Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Jetty Integration #320

Merged
merged 5 commits into from
May 11, 2022
Merged

Add Jetty Integration #320

merged 5 commits into from
May 11, 2022

Conversation

Mrod1598
Copy link
Contributor

@Mrod1598 Mrod1598 commented May 2, 2022

Description:
Added Support for Jetty monitoring.

Testing:
Added Integration test for Jetty and updated supported tech tests.

Documentation:
Updated list of supported Tech.
Added Jetty doc.

@@ -68,6 +68,7 @@ mutually exclusive with `otel.jmx.groovy.script`. The currently supported target

| `otel.jmx.target.system` |
|--------------------------|
| [`jetty`](./docs/target-systems/jetty.md) |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not a great system, but we have jvm on top and external libs sorted below it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

.withNetworkAliases("jetty")
.withExposedPorts(1099)
.withStartupTimeout(Duration.ofMinutes(2))
.waitingFor(Wait.forListeningPort());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an informative log statement to wait for to help w/ potential flake?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added

Copy link
Contributor

@dehaansa dehaansa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One documentation nit, otherwise looks good to me



* Name: `jetty.thread.queue.count`
* Description: The maximum amount of time a session has been active.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a copy-pasted description made it in.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@Mrod1598
Copy link
Contributor Author

@rmfitzpatrick Any chance you have some time to review this?

@rmfitzpatrick rmfitzpatrick merged commit ee223df into open-telemetry:main May 11, 2022
@Mrod1598 Mrod1598 deleted the jetty branch May 11, 2022 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants