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

Properly define CorrelationContext as an optional interface. #482

Open
carlosalberto opened this issue Feb 21, 2020 · 4 comments
Open

Properly define CorrelationContext as an optional interface. #482

carlosalberto opened this issue Feb 21, 2020 · 4 comments
Assignees
Labels
area:api Cross language API specification issue priority:p2 Medium priority level release:allowed-for-ga Editorial changes that can still be added before GA since they don't require action by SIGs spec:baggage Related to the specification/baggage directory
Milestone

Comments

@carlosalberto
Copy link
Contributor

From #420

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.

This is a very important distinction that we want to make so languages keep a certain uniformity, and I do agree this the statement above, so we must add a note or a section that fully clarifies this one.

@carlosalberto
Copy link
Contributor Author

Added it to the (incoming) milestone 0.4.0.

@carlosalberto carlosalberto self-assigned this Feb 21, 2020
@carlosalberto
Copy link
Contributor Author

Copying/pasting a comment from @bogdandrutu on why we should recommend implementations to implement this (from #420):

Here are some reasons why we need an explicit object called Correlation exposed:

  • Current API makes impossible to move one item from a Context object to another. For example in case of a batch operation if there are 5 operations batched and I want to extract the Correlation from one object I need to extract all elements in a dictionary, then iterate over them and manually add 1 by 1.
  • It makes a clear separation between the Context functionality and the Correlation functionality.
  • Allow users to manually propagate individual items in the Context.
  • Consistency with trace where we expose the Span object.
  • Consistency with languages that requires to have an object defined like Python if you want to use ContextVar API.
  • To avoid creating long Context chains (if implemented using a reference to the parent instead of copying all the elements), we can build instrumentation that creates a Context object once an adds all the items (like Span + CorrelationContext).

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 TagContext was needed in order to implement the tags propagation in gRPC.

@bogdandrutu bogdandrutu modified the milestones: v0.4, v0.5 Apr 28, 2020
@andrewhsu
Copy link
Member

From the SIG mtg today, sounds like this one would be best to be bumped to the next milestone.

@carlosalberto carlosalberto modified the milestones: v0.5, v0.6 Jun 9, 2020
@carlosalberto carlosalberto added spec:baggage Related to the specification/baggage directory area:api Cross language API specification issue labels Jun 30, 2020
@reyang reyang added the release:required-for-ga Must be resolved before GA release, or nice to have before GA label Jul 1, 2020
@carlosalberto carlosalberto added the priority:p2 Medium priority level label Jul 24, 2020
@andrewhsu andrewhsu added release:allowed-for-ga Editorial changes that can still be added before GA since they don't require action by SIGs and removed release:required-for-ga Must be resolved before GA release, or nice to have before GA labels Sep 25, 2020
@andrewhsu
Copy link
Member

from the issue triage mtg today with TC, allowing changes related to this issue for GA if editorial changes only

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:api Cross language API specification issue priority:p2 Medium priority level release:allowed-for-ga Editorial changes that can still be added before GA since they don't require action by SIGs spec:baggage Related to the specification/baggage directory
Projects
None yet
Development

No branches or pull requests

4 participants