-
Notifications
You must be signed in to change notification settings - Fork 844
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
Context propagation update (OTEP 66) #720
Context propagation update (OTEP 66) #720
Conversation
Codecov Report
@@ Coverage Diff @@
## master #720 +/- ##
=========================================
Coverage 84.75% 84.75%
Complexity 979 979
=========================================
Files 128 128
Lines 3541 3541
Branches 310 310
=========================================
Hits 3001 3001
Misses 413 413
Partials 127 127 Continue to review full report at Codecov.
|
cc @yurishkuro ;) |
api/src/main/java/io/opentelemetry/context/propagation/DefaultPropagators.java
Outdated
Show resolved
Hide resolved
api/src/main/java/io/opentelemetry/trace/propagation/ContextUtils.java
Outdated
Show resolved
Hide resolved
I don't love that we end up with 3 |
Agreed. I kept them with their original names (at least the |
@jkwatson Also, those |
Yes, and we can definitely iterate over details here and there that might need polishing or adjustments. |
This PR has been updated with the latest bits of master (took slightly longer than expected as I had to update our brand new B3 Propagator ;) ) cc @bogdandrutu |
api/src/test/java/io/opentelemetry/trace/DefaultTracerTest.java
Outdated
Show resolved
Hide resolved
context_prop/src/main/java/io/opentelemetry/context/propagation/DefaultContextPropagators.java
Show resolved
Hide resolved
Co-Authored-By: Giovanni Liva <[email protected]>
Ping @bogdandrutu Would you mind sharing the bits around what we need to change? I can work on that so we can get this PR updated before 0.3.0 (hopefully) ;) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good to me! Thanks for the work @carlosalberto!
I have only issue with the API for setting the default propagators.
I would prefer to have an API like the one for registering SpanProcessor where the user can add to the list of existing ones. Having a setter for the whole list might be disruptive since a module registers, say the B3 propagator, and another module overrides this with the HTTP Text propagator. I know that we recommend to have the configuration only in a single place, but I would like to avoid such a case. WDYT?
Hey @thisthat thanks for the review!
I remember talking about this when writing the prototype, but it was discussed in the Specification (by Yuri, IIRC) that ideally you want to set the propagators only ONCE in your code, and that would be in the user code, not from the instrumentation side, hence having it exposed as a For this specific case you mention, if you feel it's worth considering or (even better) have a test scenario, I suggest filling an issue in the Specification, as this could impact other languages 😄 |
api/src/main/java/io/opentelemetry/correlationcontext/CorrelationsContextUtils.java
Outdated
Show resolved
Hide resolved
api/src/main/java/io/opentelemetry/trace/TracingContextUtils.java
Outdated
Show resolved
Hide resolved
api/src/main/java/io/opentelemetry/trace/TracingContextUtils.java
Outdated
Show resolved
Hide resolved
api/src/test/java/io/opentelemetry/trace/DefaultTracerTest.java
Outdated
Show resolved
Hide resolved
context_prop/src/main/java/io/opentelemetry/context/propagation/ContextPropagators.java
Outdated
Show resolved
Hide resolved
context_prop/src/main/java/io/opentelemetry/context/propagation/DefaultContextPropagators.java
Show resolved
Hide resolved
@bogdandrutu Updated to use only In the Specification it is mentioned that we ought to support both SpanContext spanContext = extractImpl(...);
return TracingContextUtils.withSpan(DefaultSpan.create(spanContext), ctx); ... which is fine for now IMHO (and this pattern is only used 4 times in the entire code base, aside from tests, so we are cool). |
16b8020
to
88fdcbb
Compare
@bogdandrutu Reverted the |
@carlosalberto same for Correlation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Final small comments.
api/src/main/java/io/opentelemetry/correlationcontext/CorrelationsContextUtils.java
Outdated
Show resolved
Hide resolved
Please make sure we close all the comments:
|
@bogdandrutu Ooops, was moving files around and guess the Correlation one didn't make it somehow. Fixing now. |
@bogdandrutu Filled the new issues, and updated the code (one of them was not required anymore, FYI) |
* Initial Propagators refactor. * Add tests for the new propagators changes. * Make the SDK compile/pass. * Make the OT shim compile/pass. * Make contrib components compile/pass. * Improvement over Span/SpanContext handling in Context. * Add Span.setParent(Context) overload. * Do not provide default values for Span/SpanContext keys in Context. * Improve the Context's active state handling. * Rename DistributedContext to CorrelationContext. * Improve names for correlationcontext's ContextUtils methods. * Don't provide an automatic default for current CorrelationContext. * Improve the client-server example after the recent changes. * Adds CorrelationContext.Builder.setParent(Context) overload. * s/be/become. * Fix javadoc. * No need to use diamond. * Simply import withScopedContext(). * Fix the API/SDK build. * Remove the builder from the Propagators interface. * Fix name. * Use Collections.emptyList() directly. * Rename Propagators to ContextPropagators. * Move context/ members in api/ to context_prop/ * Add check/tests for null scoped Contexts. * Rename ContextUtils classes to better alternatives. * Update the context* util classes. * Make the code compile after the latest master merge. * Cache the fields() in our default composite propagator. * Remove the overloads of setParent(Context) for now. * Use DefaultSpan for the tests instead of calling getTracerProvider() * Fix the sdk testbed artifact build. * Make the B3 propagator comply with the new propagator API. * Simplify the HttpTraceContextTest tests. * Simplify the ContextUtils* classes handling of default values. * Minor nit. * Update api/src/main/java/io/opentelemetry/OpenTelemetry.java Co-Authored-By: Giovanni Liva <[email protected]> * Annotate ContextPropagators with ThreadSafe instead of Immutable. * Do not use the fully qualified ContextUtils identifier. * Remove SpanContext support from TracingContextUtils. * Rever to using non-defaulted key for TracingContextUtils. * Revert the default keys for CorrelationsContextUtils. Co-authored-by: Giovanni Liva <[email protected]>
This PR expects to be a real take on open-telemetry/oteps#66 which changes how context propagation takes place.
Summary
The OpenTelemetry API/SDK becomes aware of the notion of
Context
. At this moment we are usingio.grpc.Context
directly, as there was a fear that we would end up wrapping objects too much if we had had our own abstraction on top of it (open to discussion). However, most of the API remains untouched, as it will useContext
implicitly.DistributedContext
has been renamed toCorrelationContext
(Rename DistributedContext to CorrelationContext #757), and has kept all its semantics (including its getters and setters).Binary
format has been removed for now (Remove Binary format and related members. #891 ), as it's not possible to, given a byte buffer, trivially extract all the contained data (SpanContext
,CorrelationContext
). The plan is to add it in the near future.The overloads of
setParent(Context)
for both spans and correlation context have been removed from this PR, in order to make it smaller (will follow up with them next).Context usage
An instance of
Context
will contain all the different values (Span
,CorrelationContext
, etc) under the scenes - usually,Context.current()
. The OTel api will use it behind the scenes.Propagators
Propagators, the responsible objects for injecting/extracting interprocess context, work directly with such
Context
(by fetching and storing their objects in it):Injection
Extraction
Technical detail:
Extract()
used to returnnon-null
values, which is not expected now (propagators can storenull
in their concern's slot inContext
). This simplifies checks and allows having no-op propagators that don't need to be set default values for each concern.Technical detail 2: For the same reason, we don't provide
Context.Key
with default values, in order to simplify checking whether an actual value is stored inContext
.Global Propagators
The default, global
OpenTelemetry.getPropagators()
contains tracing'sHttpTraceContext
(and should later include the propagator forCorrelationContext
) - this is done to keep the current invariant of doing propagation even for theDefaultTracer
, and also to provide sane defaults.Initialization (done at start up time)
Library instrumentation
Technical detail: A previous approach was to use SPI to load a
Propagators
implementation, which would have aaddHttpTextFormat()
method which would add it similarly to howSpanProcessor
instances are added to an already createdTracer
(using a volatile array with a lock-on-write setter to handle the propagators). This would complicate a little bit the propagation, and would make it mutable, but it's definitely an option.Active Span/SpanContext handling
Given a
Context
,Span
has higher priority thanSpanContext
. It is expected that, upon a new request, an emptyContext
will be available prior toExtract()
, which will make this invariant work. This is subject to change depending on further requirements.Closing notes
There are a few potential improvement areas (such as having overloads for
HttpTextFormat
methods usingContext.current()
directly), but I'd like to keep those things as future sugar, if possible (unless they are really fundamental, one way or another).