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

Implement a dedicated reactor-netty 1.0 instrumentation #4662

Merged

Conversation

mateuszrzeszutek
Copy link
Member

This PR introduces a completely new reactor-netty 1.0 instrumentation; one that does not rely on netty client instrumentation to produce telemetry. The HTTP client span is now the parent of RESOLVE/CONNECT/SSL spans, as I suggested in #4617:

In case of "normal"/higher-level HTTP clients I think it'd make perfect sense for them to be nested under the HTTP CLIENT span.

(Now I only need to change the kind of those nested spans from INTERNAL to CLIENT...)

CC @lmolkova

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.

nice 🎉

Comment on lines +77 to +82
if (propagatedContext.useClientContext) {
context = contextView.getOrDefault(ReactorContextKeys.CLIENT_CONTEXT_KEY, null);
}
if (context == null) {
context = contextView.getOrDefault(ReactorContextKeys.CLIENT_PARENT_CONTEXT_KEY, null);
}
Copy link
Member

Choose a reason for hiding this comment

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

is the fallback in case of useClientContext important? (vs straight if/else here)

Copy link
Member Author

Choose a reason for hiding this comment

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

It is -- when reactor-netty span is suppressed and client context is not accessible from the reactor's ContextView we still want to execute the doOn*() callbacks with the parent context in scope.

Comment on lines +163 to +164
// connection is actually an instance of HttpClientOperations - a package private class that
// implements both Connection and HttpClientResponse
Copy link
Member

Choose a reason for hiding this comment

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

thx for these comments ❤️

Comment on lines 98 to 99
// needs to be called after original method to prevent StackOverflowError
callDepth.decrementAndGet();
Copy link
Member

Choose a reason for hiding this comment

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

can you move the callDepth update to beginning of these methods? I'm just paranoid about resetting thread local state 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

I have to decrement the count after the original response*() method is called, that's why they're the last thing I'm calling. I've wrapped those invocations in try-finally blocks - now the counter will always be decremented.

Copy link
Member

Choose a reason for hiding this comment

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

oh, i completely missed the skipOn and what was happening here 👍


Context context = instrumenter().start(parentContext, config);
contextHolder.context = context;
return ContextPropagationOperator.runWithContext(mono, context)
Copy link
Contributor

Choose a reason for hiding this comment

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

nice! it looks like pure context API is still useful even without extra convenience we discussed to start spans

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it turned out to be super useful this time 🚀

@trask trask merged commit 9235719 into open-telemetry:main Nov 18, 2021
@mateuszrzeszutek mateuszrzeszutek deleted the reactor-netty-instrumentation branch November 18, 2021 18:29
RashmiRam pushed a commit to RashmiRam/opentelemetry-auto-instr-java that referenced this pull request May 23, 2022
…ry#4662)

* Implement a dedicated reactor-netty 1.0 instrumentation

* Apply suggestions from code review

Co-authored-by: Trask Stalnaker <[email protected]>

* code review comments

* code review comments

* code review comments

Co-authored-by: Trask Stalnaker <[email protected]>
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