-
Notifications
You must be signed in to change notification settings - Fork 896
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
Adding Correlations API and removing DistributedContext #420
Adding Correlations API and removing DistributedContext #420
Conversation
Updating DistributedContext to CorrelationContext as per [otep-66](https://github.com/open-telemetry/oteps/blob/master/text/0066-separate-context-propagation.md#Correlations-API). Updated the following: - added correlations API details - removing Entry as a separate class in favour of name/value pair to align more closely with the w3c spec - library layout, file name and README - overview Signed-off-by: Alex Boten <[email protected]>
Signed-off-by: Alex Boten <[email protected]>
Co-Authored-By: Armin Ruech <[email protected]>
Worth noting that the Correlation Context spec has been getting more attention in recent meetings of the W3C Distributed Tracing working group. One of the things discussed has been changing the name of the header. Currently it is It may be worth noting in this specification, that we're basing it on the current version, but that it might change. |
Here's the relevant issue for @dyladan's comment: w3c/baggage#13 |
This will close #93 |
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.
Some questions about the default context, but it looks good otherwise!
- update context to be an optional parameter - removing name recommendation - updating casing on REQUIRED - updating conflict resolution example Signed-off-by: Alex Boten <[email protected]>
Signed-off-by: Alex Boten <[email protected]>
Signed-off-by: Alex Boten <[email protected]>
Signed-off-by: Alex Boten <[email protected]>
Signed-off-by: Alex Boten <[email protected]>
Hey @bogdandrutu if this looks correct to you, lets definitely approve/merge, as this is the last item prior to do a 0.3.0 release (observe a related ticket was created so we don't forget about the |
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.
LGTM.
Co-Authored-By: Reiley Yang <[email protected]>
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.
I am very confused why did we write CorrelationContext with the idea that there is no "CorrelationContext" object that is part of the context, instead we have just functions/methods that change the object in the Context.
We are not consistent with the trace API where we do expose a "Span" object, and there are discussion we had about that.
Signed-off-by: Alex Boten <[email protected]>
Signed-off-by: Alex Boten <[email protected]>
@bogdandrutu I added a few more details around the CorrelationContext, which make it more explicit that the CorrelationContext is a set of name/value pairs stored in the Context. The functions defined are interacting with the Context explicitly and implicitly with the CorrelationContext. Let me know if this change 975d4c5 makes this any clearer. |
It is not clear to me why we choose to not have all the functions interact explicitly with a "CorrelationContext" object and we want this path of interacting explicitly with Context. In my opinion this case is more natural for languages that explicitly pass Context around (like Go) but not that natural for Python where for example the ContextVar library does not work very well with this API. |
@bogdandrutu for an OO language, it would be fine to make
Does that clarify things? Or am I missing your point? |
I'd like to see |
I agree @carlosalberto, the spec should have a way of being "language neutral." The concepts, features, and requirements should be clear, but languages should be allowed to implement features using whichever language construct feels the most natural (as long as it does not violate a requirement). I think we could improve the spec on this front, but that should be separate initiative from adding the features we need for the beta launch. |
It seems to me that I am not making myself well understood, but this API has some serious problems when it is used with an implicit (thread-local) storage and not with an explicit (manually) propagated mechanism. Here are some reasons why we need an explicit object called Correlation exposed:
For an experiment you can look at https://github.com/grpc/grpc-java/blob/b7ccc0d14297f255557e67a4f0364480f4a80bcf/census/src/main/java/io/grpc/census/CensusStatsModule.java#L102 to see why a |
Merging this PR as we have enough approvals, to keep the pace and do our 0.3.0 release as expected (we are already behind schedule anyway) - we will follow up on #482 as I think it's very important to follow up and solve the |
This reverts commit abdd521.
Oh, definitely a race condition between my comment/merge and your latest comment ;) Thanks for the explanation, and specially the code example, as it will definitely help us have an even better understanding of the requirement. Observe that the current specification doesn't restrict the usage of I'm wondering if this is worth, overall, to prevent us from doing the 0.3.0 release. Next release is expected to happen in 1 week, so I guess we need to ponder all our alternatives here. @open-telemetry/specs-approvers opinions? |
My main point is that the way how this is written now will make a lot of the sigs to implement this way then they will realize that it is not good for some cases. |
I think the best thing to do is to create an issue and move the discussion there. Maybe mark this as experimental for the moment. |
Updating DistributedContext to CorrelationContext as per otep-66. Updated the following:
Signed-off-by: Alex Boten [email protected]