-
Notifications
You must be signed in to change notification settings - Fork 857
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
Propagate context #572
Propagate context #572
Conversation
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.
this change looks really good
a couple suggestions below
// TODO I think all this should happen on exit, not on enter. | ||
// After response was handled by user provided handler. |
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 think it makes sense to end the span here, at the beginning of onCompleted
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.
My line of thought was the following: in comes a request, when a response is ready AsyncCompletionHandler
will be called. I think we want to end the request processing span after that handler has completed its job. So that if it fails or takes long time, that would be recorded in the captured span. No?
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 think the CLIENT
span timing and success are supposed to match the HTTP timing / success as closely as possible
i don't see this called out in the spec, but there's some related discussion here: open-telemetry/opentelemetry-specification#591 (comment)
.../src/main/java/io/opentelemetry/auto/instrumentation/grizzly/client/ClientRequestAdvice.java
Outdated
Show resolved
Hide resolved
...main/java/io/opentelemetry/auto/instrumentation/playws/v1_0/PlayWSClientInstrumentation.java
Outdated
Show resolved
Hide resolved
...ava8/io/opentelemetry/auto/instrumentation/springwebflux/server/DispatcherHandlerAdvice.java
Outdated
Show resolved
Hide resolved
...ava8/io/opentelemetry/auto/instrumentation/springwebflux/server/DispatcherHandlerAdvice.java
Outdated
Show resolved
Hide resolved
...n/java8/io/opentelemetry/auto/instrumentation/springwebflux/server/HandlerAdapterAdvice.java
Show resolved
Hide resolved
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.
Approach LGTM, definitely nicer to propagate context than span
...1.0/src/main/java/io/opentelemetry/auto/instrumentation/playws/v1_0/AsyncHandlerWrapper.java
Outdated
Show resolved
Hide resolved
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.
💯
Partially addresses #507 .
First cut on propagating full Context instead of just Span. Two major things are not done yet:
io.opentelemetry.auto.bootstrap.instrumentation.decorator.HttpServerTracer#attachSpanToRequest
should be redone to something likepropagateContext
. But as new context is not created insidestartSpan
method, thatpropagateContext
should be moved, made public etc...io.opentelemetry.auto.bootstrap.instrumentation.decorator.HttpServerDecorator#SPAN_ATTRIBUTE
. I have a temptation to live it be. The wholeHttpServerDecorator
is deprecated and is to be replaced byHttpServerTracer
anyway.Probably something else is missing