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 jdk.httpserver instrumentation #13243

Merged
merged 32 commits into from
Feb 14, 2025
Merged

Conversation

SentryMan
Copy link
Contributor

@SentryMan SentryMan commented Feb 7, 2025

Adds instrumentation for the jdk.httpserver module.

will resolve #13216 when I get the agent working

Copy link

linux-foundation-easycla bot commented Feb 7, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.


@Override
public String getHttpRoute(HttpExchange exchange) {
return exchange.getHttpContext().getPath();
Copy link
Contributor

Choose a reason for hiding this comment

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

Route must have a low cardinality. Often it is not possible to get route for a low level server framework. Just return null here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't follow, exchange.getHttpContext().getPath() gives you the configured path for the route, not what is currently matched

Copy link
Contributor Author

@SentryMan SentryMan Feb 8, 2025

Choose a reason for hiding this comment

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

that is, if you do

    HttpServer server = HttpServer.create(new InetSocketAddress(port), 0);
    server.createContext("/path/" ctx ->{});

and send a request to /path/something, you'll get /path/ from exchange.getHttpContext().getPath()

@SentryMan
Copy link
Contributor Author

@laurit perchance do you know why it's failing now?

@trask
Copy link
Member

trask commented Feb 7, 2025

@laurit perchance do you know why it's failing now?

I haven't followed the discussions above, but looks like its:

2025-02-07T18:19:27.2764194Z > Task :instrumentation:java-http-server:javaagent:test
2025-02-07T18:19:27.2766261Z 
2025-02-07T18:19:27.2768473Z JdkHttpServerTest > httpPipelining() FAILED

you could opt-out of the http pipelining test initially at least, look for options.setTestHttpPipelining(false)

@SentryMan
Copy link
Contributor Author

what's the best way to search these error logs, I'm having trouble locating the actual tests that are failing.

@SentryMan SentryMan marked this pull request as ready for review February 8, 2025 03:34
@SentryMan
Copy link
Contributor Author

So I add a Readme and the pipeline fails?

@trask
Copy link
Member

trask commented Feb 12, 2025

So I add a Readme and the pipeline fails?

we have been doing some compliance work in the repo recently which introduced a couple of new required PR checks, and so just need to rebase to main to fix (looks like @laurit has done that)

@laurit laurit added the test openj9 This label can be applied to PRs to trigger them to run openj9 tests label Feb 13, 2025
Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

thanks @SentryMan @laurit! one suggestion on the package name

@trask trask merged commit 8d057a2 into open-telemetry:main Feb 14, 2025
81 checks passed
@SentryMan SentryMan deleted the jdk.httpserver branch February 14, 2025 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test openj9 This label can be applied to PRs to trigger them to run openj9 tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for java's jdk.httpserver module
4 participants