KEMBAR78
Micrometer#observation(): avoids scopes, set parentObservation by simonbasle · Pull Request #3119 · reactor/reactor-core · GitHub
Skip to content

Micrometer#observation(): avoids scopes, set parentObservation #3119

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Jul 22, 2022
Merged

Micrometer#observation(): avoids scopes, set parentObservation #3119

merged 6 commits into from
Jul 22, 2022

Conversation

@simonbasle
Copy link
Contributor

@simonbasle simonbasle commented Jul 18, 2022 •
edited
Loading

This commit reworks the reactor-core-micrometer module in several ways:

  • removed context-propagation dependency
  • read Observation from Context
  • don't open a scope, but rather start new Observation setting old one
    as parent explicitly
  • store the new Observation in the ContextView
outdated
  • made the context-propagation dependency optional
  • added Micrometer#isContextPropagationAvailable to detect it at
    runtime
  • also added Micrometer#isTracingAvailable as a bonus
  • reworked MicrometerObservationListener to check context-propagation
    is available and fallback to exclusively use Reactor Context to deal
    with Observation hierarchy:
  • when context-propagation is there, focus on Micrometer.Observation
    ContextSnapshot key using a predicate in eg. captureUsing

@simonbasle
Rework Micrometer#observation(), context-propagation is optional (#1111)
dccdf6b 
This commit reworks the reactor-core-micrometer module in several ways:

 - made the context-propagation dependency optional
 - added `Micrometer#isContextPropagationAvailable` to detect it at
 runtime
 - also added `Micrometer#isTracingAvailable` as a bonus
 - reworked MicrometerObservationListener to check context-propagation
 is available and fallback to exclusively use Reactor Context to deal
 with Observation hierarchy:
  - read Observation from Context
  - don't open a scope, but rather start new Observation setting old one
  as parent explicitly
  - store the new Observation in the ContextView
 - when context-propagation is there, focus on Micrometer.Observation
 ContextSnapshot key using a predicate in eg. `captureUsing`
@simonbasle simonbasle added this to the 3.5.0-M5 milestone Jul 18, 2022
@simonbasle simonbasle added area/observability type/dependency-upgrade A dependency upgrade (possibly via bot) type/enhancement A general enhancement labels Jul 18, 2022
@simonbasle simonbasle self-assigned this Jul 18, 2022
rstoyanchev
rstoyanchev reviewed Jul 19, 2022
View reviewed changes
...meter/src/main/java/reactor/core/observability/micrometer/MicrometerObservationListener.java Outdated
//tap context hasn't been initialized yet, so addToContext can now use the Scope
ContextSnapshot contextSnapshot2 = ContextSnapshot.capture(this.originalContext);
this.contextWithScope = contextSnapshot2.updateContext(Context.of(this.originalContext));
if (Micrometer.isContextPropagationAvailable()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This might not always work very well. I think in some cases debuggers can introspect local variables which would cause classpath issues if context-propagation is not there. We tend to isolate such blocks of code in static, nested classes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can you elaborate? I'm not sure what and how to isolate at this level...

@simonbasle simonbasle marked this pull request as ready for review July 19, 2022 17:00
@simonbasle
Copy link
Contributor Author

following the reviews and a long discussion with @marcingrzejszczak, I've reworked this quite a bit and the MicrometerObservationListener doesn't even use the context-propagation API anymore.

We've decided that removing all Scope/openScope was the way to go, since this has potential for ThreadLocals pollution (the doFirst isn't guaranteed to run in the same thread than eg. doOnComplete, when it comes to scope opening / closing).

So the Micrometer.observation() tap listener now only concerns itself with starting an Observation and directly assigning the correct parentObservation to it, either from the ContextView or barring that from the registry.getCurrentObservation().

@simonbasle
Copy link
Contributor Author

simonbasle commented Jul 19, 2022 •
edited
Loading

side note on the usage of ObservationThreadLocalAccessor.KEY: this should be ok even if the context-propagation jar isn't available at runtime, even though that class implements an interface from said jar: the class isn't initialized as long as we only access the static constant KEY.

(at a point I had a test suite without context-propagation, and it was running fine while accessing KEY. this test suite has been rolled back however)

@simonbasle simonbasle changed the title Rework Micrometer#observation(), context-propagation is optional (#1111) Rework Micrometer#observation(), context-propagation is optional Jul 20, 2022
rstoyanchev
rstoyanchev approved these changes Jul 20, 2022
View reviewed changes
Copy link
Contributor

@rstoyanchev rstoyanchev left a comment

Choose a reason for hiding this comment

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

Looks much more straight-forward. I like that it's possible to just work with the Reactor Context as expected within a Reactor chain.

@simonbasle simonbasle changed the title Rework Micrometer#observation(), context-propagation is optional Rework Micrometer#observation() to avoid scopes/ThreadLocals Jul 21, 2022
@simonbasle simonbasle changed the title Rework Micrometer#observation() to avoid scopes/ThreadLocals Rework Micrometer#observation() to avoid scopes, set parentObservation Jul 21, 2022
@simonbasle simonbasle changed the title Rework Micrometer#observation() to avoid scopes, set parentObservation Rework Micrometer#observation(): avoid scopes, set parentObservation Jul 22, 2022
@simonbasle simonbasle changed the title Rework Micrometer#observation(): avoid scopes, set parentObservation Micrometer#observation(): avoids scopes, set parentObservation Jul 22, 2022
@simonbasle simonbasle merged commit f47fc56 into main Jul 22, 2022
@simonbasle simonbasle deleted the contextPropagationOptional2 branch July 22, 2022 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@violetagg violetagg Awaiting requested review from violetagg

3 more reviewers

@rstoyanchev rstoyanchev rstoyanchev approved these changes

@marcingrzejszczak marcingrzejszczak marcingrzejszczak approved these changes

@OlegDokuka OlegDokuka OlegDokuka approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@simonbasle simonbasle

Labels

area/observability type/dependency-upgrade A dependency upgrade (possibly via bot) type/enhancement A general enhancement

Projects

None yet

Milestone

3.5.0-M5

Development

Successfully merging this pull request may close these issues.

4 participants

@simonbasle @rstoyanchev @marcingrzejszczak @OlegDokuka