-
Notifications
You must be signed in to change notification settings - Fork 904
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
feat: Add OpenTelemetry instrumentation for ActiveJ HTTP server #13335
base: main
Are you sure you want to change the base?
Conversation
|
f90bed1
to
dfa49e2
Compare
…nabling distributed tracing and context propagation for ActiveJ-based HTTP servers
dfa49e2
to
b52c49d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we will want integration tests for any new instrumentation
...emetry/javaagent/instrumentation/activejhttp/ActiveJHttpServerConnectionInstrumentation.java
Outdated
Show resolved
Hide resolved
...emetry/javaagent/instrumentation/activejhttp/ActiveJHttpServerConnectionInstrumentation.java
Outdated
Show resolved
Hide resolved
…nAsWordInName) at class level
7396363
to
b241b45
Compare
@jaydeluca I have added test cases and resolved most of the workflow job failures. But, still there are couple of them are failing and not able to understand the cause after checking the raw logs as well. Could you please help me to understand what causing them to failure so that I can fix them. Thank you! |
@laurit Thanks for the link. So, all the agents should support minimum java 8? |
Thanks for the help! @laurit . Build looks good now. Please review |
...emetry/javaagent/instrumentation/activejhttp/ActivejHttpServerConnectionInstrumentation.java
Show resolved
Hide resolved
...emetry/javaagent/instrumentation/activejhttp/ActivejHttpServerConnectionInstrumentation.java
Outdated
Show resolved
Hide resolved
.../javaagent/instrumentation/activejhttp/ActivejHttpServerConnectionInstrumentationModule.java
Outdated
Show resolved
Hide resolved
...ain/java/io/opentelemetry/javaagent/instrumentation/activejhttp/ActivejHttpServerHelper.java
Outdated
Show resolved
Hide resolved
...entelemetry/javaagent/instrumentation/activejhttp/ActivejHttpServerHttpAttributesGetter.java
Outdated
Show resolved
Hide resolved
...entelemetry/javaagent/instrumentation/activejhttp/ActivejHttpServerHttpAttributesGetter.java
Outdated
Show resolved
Hide resolved
...entelemetry/javaagent/instrumentation/activejhttp/ActivejHttpServerHttpAttributesGetter.java
Outdated
Show resolved
Hide resolved
...entelemetry/javaagent/instrumentation/activejhttp/ActivejHttpServerHttpAttributesGetter.java
Outdated
Show resolved
Hide resolved
...t/java/io/opentelemetry/javaagent/instrumentation/activejhttp/ActivejRoutingServletTest.java
Outdated
Show resolved
Hide resolved
Latest dep tests try to automatically bump the |
@laurit now test-latest-deps / testLatestDeps2 is failing. Like you said in my local when I ran I don't find the error in the logs as well, so not sure what is happening here. In Raw logs as well, don't see much info and it is really difficult to navigate. Last time you shared an URL like this - https://scans.gradle.com/s/dy73bc67fptxo/tests/task/:instrumentation:activej-http-6.0:javaagent:test/details/(N%2FA)/Gradle%20Test%20Executor%2045?top-execution=1. Can you tell me how to access that in case of failure, so that it will be easy for me in future. Thanks! |
this failure looks unrelated to your PR |
it's been fixed in |
…emetry#13296) Co-authored-by: Lauri Tulmin <[email protected]>
) Co-authored-by: otelbot <[email protected]>
Co-authored-by: otelbot <[email protected]> Co-authored-by: Trask Stalnaker <[email protected]>
…-telemetry#13349) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…emconv to v1.30.0 (open-telemetry#13351) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Lauri Tulmin <[email protected]>
…en-telemetry#13354) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…ts to v4-rev20250211-2.0.0 (open-telemetry#13357) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Thanks @trask . Looks good now |
…rter-web to v3.4.3 (open-telemetry#13362) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…20.5 (open-telemetry#13365) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
@laurit all the comments are addressed. Please check again. Thanks! |
instrumenter() | ||
.end( | ||
context, | ||
httpRequest, | ||
responsePromise == null ? null : responsePromise.getResult(), | ||
throwable); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I doubt this is the right way. We want to end the span when the request processing ends. For async methods request processing ends when the promise completes not when the called method exits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@laurit I don't see a better way to do this. I even looked into some of the existing async handling and this is how it is done, here is an example from google-http-client
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the linked code in google http client only ends the span on method exit when there was an exception. Did you try what I proposed in #13335 (comment)
|
||
public class ActivejHttpServerTest extends AbstractHttpServerTest<HttpServer> { | ||
|
||
@ClassRule public static final EventloopRule eventloopRule = new EventloopRule(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
classrule is for junit4 we use junit5
@Override | ||
protected void assertHighConcurrency(int count) { | ||
// | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not implemented
Overview
This PR introduces OpenTelemetry instrumentation for the ActiveJ framework, enabling distributed tracing and context propagation for ActiveJ-based HTTP servers. The changes allow developers to monitor and debug ActiveJ applications in distributed systems by capturing trace context from incoming requests and propagating it through responses.
Key Features
traceparent
header.Changes Included
Instrumentation:
ActiveJHttpServerConnectionInstrumentation
: Instruments theserve
method of ActiveJ servlets to capture and propagate trace context.Testing:
Documentation:
Testing
traceparent
header.Benefits
Related Issues
Closes #13202
Acknowledgments
This implementation was inspired by and adapted from the ActiveJ framework codebase. Special thanks to the ActiveJ community for their excellent work and contributions.