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

Replace trac(k)ing abstract with more generic context tracking #1495

Merged
merged 2 commits into from
Jan 28, 2019

Conversation

bsideup
Copy link
Contributor

@bsideup bsideup commented Jan 28, 2019

After #1492, there is no need for a special Marker abstraction and the Tracing story can be simplified to "Context tracking".

Copy link
Member

@simonbasle simonbasle left a comment

Choose a reason for hiding this comment

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

needs small doc improvements, but great to be able to remove unnecessary interface ;)

* Example implementation:
* <pre>{@code
* if (!shouldTrack()) return context;
* Marker parent = context.getOrDefault(Marker.class, null);
Copy link
Member

Choose a reason for hiding this comment

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

snippet still references Marker

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's an example, Marker is something provided by the implementation

* {@link ContextTracker} implementors receive this callback before
* the {@link reactor.core.scheduler.Scheduler}s execute scheduled {@link Runnable}s.
* <p>
* Consider implementing it if your tracking relies on {@link ThreadLocal}s.
Copy link
Member

Choose a reason for hiding this comment

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

maybe flesh out this sentence: the method would extract key-value pairs from Context and put them in appropriate ThreadLocal variables, then return a Disposable that clears said threadlocals. Maybe ThreadLocal is not the right example either (MDC ?).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most tracers use ThreadLocal to store the spans (as well as MDC's implementation), hence the mention :)

But I like the idea of guiding how to implement it, will add 👍

@codecov-io
Copy link

Codecov Report

Merging #1495 into tracing will decrease coverage by 0.06%.
The diff coverage is 74.28%.

Impacted file tree graph

@@              Coverage Diff              @@
##             tracing    #1495      +/-   ##
=============================================
- Coverage       83.9%   83.84%   -0.07%     
+ Complexity      3932     3918      -14     
=============================================
  Files            363      363              
  Lines          30198    29955     -243     
  Branches        5601     5527      -74     
=============================================
- Hits           25339    25117     -222     
+ Misses          3208     3192      -16     
+ Partials        1651     1646       -5
Impacted Files Coverage Δ Complexity Δ
...in/java/reactor/core/publisher/ContextTracker.java 0% <0%> (ø) 0 <0> (?)
...isher/ContextTrackingExecutorServiceDecorator.java 45.94% <55.55%> (ø) 2 <1> (?)
...actor/core/publisher/ContextTrackingPublisher.java 73.68% <83.33%> (ø) 4 <4> (?)
...re/src/main/java/reactor/core/publisher/Hooks.java 92% <84.61%> (ø) 49 <2> (ø) ⬇️
...e/src/main/java/reactor/core/publisher/Traces.java 66.66% <0%> (-6.16%) 25% <0%> (-8%)
...ain/java/reactor/core/publisher/MonoCacheTime.java 87.07% <0%> (-3.41%) 18% <0%> (-1%)
...a/reactor/core/scheduler/ReactorThreadFactory.java 85.71% <0%> (-1.25%) 6% <0%> (ø)
...ain/java/reactor/core/publisher/FluxUsingWhen.java 96.41% <0%> (-0.99%) 8% <0%> (-8%)
...c/main/java/reactor/core/publisher/RingBuffer.java 70.56% <0%> (-0.95%) 24% <0%> (ø)
...main/java/reactor/core/publisher/FluxGenerate.java 69.32% <0%> (-0.62%) 7% <0%> (ø)
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d11fa50...24a80f0. Read the comment docs.

@bsideup bsideup merged commit acc2b0d into tracing Jan 28, 2019
@reactor reactor deleted a comment from reactorbot Jan 28, 2019
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.

None yet

4 participants