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

Use new reactor contextWrite when available (from reactor 3.4.0) #7538

Merged
merged 11 commits into from
Jan 12, 2023
Merged

Use new reactor contextWrite when available (from reactor 3.4.0) #7538

merged 11 commits into from
Jan 12, 2023

Conversation

trask
Copy link
Member

@trask trask commented Jan 10, 2023

Related to #7107 and #7202

Support WebFlux 6.

Supporting reactor 3.5 seems pretty straightforward, the subscriberContext() was deprecated in 3.4 in favor of contextWrite(). In 3.5, subscriberContext() was removed.

This PR doesn't bump latestDepTestLibrary to 3.5 yet because there are a couple of tests that succeed in 3.4 using contextWrite() but fail in 3.5 using contextWrite().

My proposal is to review/merge this PR, and then I can ping our resident reactor experts to see if they have thoughts on the failing tests in 3.5.

@trask trask marked this pull request as ready for review January 11, 2023 02:28
@trask trask requested a review from a team January 11, 2023 02:28
@@ -56,6 +67,7 @@ public List<String> requestHeader(ClientRequest request, String name) {
}

@Override
@Nullable
public Integer statusCode(
Copy link
Member

Choose a reason for hiding this comment

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

I encountered the same issue in #7551 -- and decided not to use the rawStatusCode() method, it's already deprecated and a target for removal.

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 kept rawStatusCode() for version range [5.1,6.0), but added new path for 6.0

interim =
(Mono<String>)
MONO_CONTEXT_WRITE_METHOD.invoke(
interim, new StoreOpenTelemetryContext(Context.current().with(span)));
Copy link
Member

Choose a reason for hiding this comment

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

In the previous version of this code, wasn't Context.current().with(span) executed as a part of the lambda? I mean it was lazy before, now it's eager

@mateuszrzeszutek
Copy link
Member

This PR doesn't bump latestDepTestLibrary to 3.5 yet because there are a couple of tests that succeed in 3.4 using contextWrite() but fail in 3.5 using contextWrite().

That's the part where I gave up when I was trying to update the instrumentation 🙈
Still, I had no idea that Webflux did not really need it -- good for us!

@github-actions github-actions bot requested a review from theletterf January 11, 2023 22:12
@trask trask merged commit 09b63d2 into open-telemetry:main Jan 12, 2023
@trask trask deleted the project-reactor-3.5 branch January 12, 2023 04:26
mateuszrzeszutek pushed a commit that referenced this pull request Jan 25, 2023
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.

2 participants