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

Reactor instrumentation: do not make root context current #6593

Merged
merged 6 commits into from
Sep 14, 2022

Conversation

lmolkova
Copy link
Contributor

@lmolkova lmolkova commented Sep 12, 2022

In some cases, reactor's fluxes can be created and subscribed for at startup, and live till application dies.
TracingSubscriber is created at subscription time and captures root context as current, but this context is likely irrelevant to the rest of the Flux lifetime.

E.g. the receiver will read and process messages from the queue:

client = QueueReceveierBuilder().build();
client
  .receiveMessages()
  .subscribe(msg -> System::out::println);

Now imagine we do something like this:

  Flux<Object> publish = Flux.create(sink -> {
    Span s = tracer.spanBuilder("inner").startSpan();
    try (Scope scope = s.makeCurrent()) {
      // this will invoke the subscriber callback below synchronously
      // and we'd expect that inner span should be active
      sink.next(42); 
    } finally {
      s.end();
    }
  });

  publish
      .take(1)
      .subscribe(n -> {
        // this code is invoked synchronously from sink.next
        // but in between there was a TracingSubscriber that made root context active
        // so without this change, this assert fails
        assertThat(Span.current().getSpanContext().isValid()).isTrue();
      });

This change only prevents the root context from becoming current. But if subscribe happens in scope of some span, there is no reliable way to know if it's the correct one. If not, applications can work it around by

  • subscribe to long-running fluxes outside of any scope
  • adding root trace context, to reactor context to avoid tracing subscriber to propagate anything as done in doesNotOverrideInnerCurrentSpansWithThereIsOuterCurrent test

@lmolkova lmolkova requested a review from a team September 12, 2022 17:23
@trask
Copy link
Member

trask commented Sep 13, 2022

@lmolkova heads up, I merged main into your branch to fix CI

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, thx!

@trask trask added this to the v1.18.0 milestone Sep 13, 2022
@trask trask merged commit 97bc4a4 into open-telemetry:main Sep 14, 2022
LironKS pushed a commit to helios/opentelemetry-java-instrumentation that referenced this pull request Oct 23, 2022
…etry#6593)

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
LironKS pushed a commit to helios/opentelemetry-java-instrumentation that referenced this pull request Oct 31, 2022
…etry#6593)

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
LironKS pushed a commit to helios/opentelemetry-java-instrumentation that referenced this pull request Dec 4, 2022
…etry#6593)

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
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