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 tapir path matching within pekko instrumentation #13386

Merged
merged 5 commits into from
Mar 7, 2025

Conversation

masonedmison
Copy link
Contributor

@masonedmison masonedmison commented Feb 24, 2025

@masonedmison masonedmison requested a review from a team as a code owner February 24, 2025 16:46
@masonedmison masonedmison marked this pull request as draft February 24, 2025 16:46
@masonedmison masonedmison force-pushed the add-tapir-instrumentation branch 14 times, most recently from 2ee8ccc to 5491d18 Compare February 26, 2025 15:22
@masonedmison masonedmison marked this pull request as ready for review February 26, 2025 15:24
@masonedmison masonedmison force-pushed the add-tapir-instrumentation branch from 5491d18 to 18f27dc Compare February 26, 2025 15:26
@masonedmison
Copy link
Contributor Author

@laurit Would you be able to look at this for me 🙂?

@laurit
Copy link
Contributor

laurit commented Feb 28, 2025

@masonedmison I think with your current approach pekko-http instrumentation will fail to apply when tapir is not available during runtime. You can try this our by changing the tapir dependency to be compileOnly + testCompileOnly. This is because we have tooling that detects which classes are used by the instrumentation and when those classes are missing instrumentation is not applied. It is probably easiest to treat it as completely separate instrumentation. An alternative would be to have it in the same module as pekko but have it in a different InstrumentationModule but then to get muzzle working you may need to use excludeInstrumentationName so that you could have separate pass blocks for pekko and tapir.

@masonedmison
Copy link
Contributor Author

masonedmison commented Feb 28, 2025

@masonedmison I think with your current approach pekko-http instrumentation will fail to apply when tapir is not available during runtime. You can try this our by changing the tapir dependency to be compileOnly + testCompileOnly. This is because we have tooling that detects which classes are used by the instrumentation and when those classes are missing instrumentation is not applied. It is probably easiest to treat it as completely separate instrumentation. An alternative would be to have it in the same module as pekko but have it in a different InstrumentationModule but then to get muzzle working you may need to use excludeInstrumentationName so that you could have separate pass blocks for pekko and tapir.

@laurit Very good catch--sorry I missed this! I'm pursuing the second approach first (just pushed up a commit): I've created a separate InstrumentationModule, and I've added the excludeInstrumentationName statements to the muzzle check blocks--with these in place running the muzzle check works. However, I've noticed that when running a simple pekko HTTP simple program (without tapir on the class path) I see a slew of "Missing Class..." on the logs, e.g.,

[error] [otel.javaagent 2025-02-28 15:50:51:020 -0600] [main] WARN io.opentelemetry.javaagent.tooling.instrumentation.MuzzleMatcher - Instrumentation skipped, mismatched references were found: tapir-pekko-http [class io.opentelemetry.javaagent.instrumentation.pekkohttp.v1_0.server.route.TapirPekkoHttpServerRouteInstrumentationModule] on jdk.internal.loader.ClassLoaders$AppClassLoader@251a69d7
[error] [otel.javaagent 2025-02-28 15:50:51:021 -0600] [main] WARN io.opentelemetry.javaagent.tooling.instrumentation.MuzzleMatcher - -- io.opentelemetry.javaagent.instrumentation.pekkohttp.v1_0.server.route.RouteWrapper$Finalizer:47 Missing class sttp.tapir.EndpointInput$Query
[error] [otel.javaagent 2025-02-28 15:50:51:021 -0600] [main] WARN io.opentelemetry.javaagent.tooling.instrumentation.MuzzleMatcher - -- io.opentelemetry.javaagent.instrumentation.pekkohttp.v1_0.server.route.TapirPathInstrumentation$ApplyAdvice:42 Missing class sttp.tapir.server.ServerEndpoint
[error] [otel.javaagent 2025-02-28 15:50:51:021 -0600] [main] WARN io.opentelemetry.javaagent.tooling.instrumentation.MuzzleMatcher - -- io.opentelemetry.javaagent.instrumentation.pekkohttp.v1_0.server.route.RouteWrapper$Finalizer:44 Missing class sttp.tapir.EndpointInput$PathCapture

Looking at the doc string for MuzzleMatcher this looks to be expected, but I just want to make sure.

@masonedmison masonedmison force-pushed the add-tapir-instrumentation branch 3 times, most recently from 24ee821 to aa4c1b8 Compare March 1, 2025 00:18
@masonedmison masonedmison force-pushed the add-tapir-instrumentation branch from aa4c1b8 to e122ec4 Compare March 1, 2025 00:26
@masonedmison
Copy link
Contributor Author

@laurit sorry to be a bother, but would you have a chance to look at my changes following your last comment?

@laurit laurit added this to the v2.14.0 milestone Mar 7, 2025
@masonedmison
Copy link
Contributor Author

@laurit Thank you very much for helping me get this together. I really appreciate it. I see that this is slated to go out in the 2.14.0 release (which is great!); do you know when this release will happen?

@laurit
Copy link
Contributor

laurit commented Mar 7, 2025

do you know when this release will happen?

second half of next week, see https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/RELEASING.md#release-cadence

@trask trask merged commit ffeb80e into open-telemetry:main Mar 7, 2025
85 checks passed
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