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

How does Span gets entries from CorrelationContext/Baggage #867

Open
cijothomas opened this issue Aug 24, 2020 · 12 comments
Open

How does Span gets entries from CorrelationContext/Baggage #867

cijothomas opened this issue Aug 24, 2020 · 12 comments
Assignees
Labels
area:api Cross language API specification issue area:sdk Related to the SDK release:after-ga Not required before GA release, and not going to work on before GA spec:baggage Related to the specification/baggage directory spec:trace Related to the specification/trace directory

Comments

@cijothomas
Copy link
Member

cijothomas commented Aug 24, 2020

What are you trying to achieve?
Its not clear from the current CorrelationContext/Baggage spec how does Span gets enriched with the entries from Baggage. How/where does this happen?

  1. User writes own SpanProcessor, and in OnStart, access Baggage and decorates the Span with it.
  2. Built-in Processors should do this automatically?

Originally asked in this PR: #857 (comment)

What did you expect to see?
Expect spec to clarify this thing to ensure every language gets consistent implementation.

@cijothomas cijothomas added spec:baggage Related to the specification/baggage directory spec:trace Related to the specification/trace directory and removed spec:trace Related to the specification/trace directory labels Aug 24, 2020
@andrewhsu
Copy link
Member

andrewhsu commented Aug 25, 2020

from the spec sig mtg today, talked about this and looks like more investigation is needed before deciding whether this is a breaking change that require resolution before ga in order to triage

@andrewhsu andrewhsu added release:after-ga Not required before GA release, and not going to work on before GA and removed release:after-ga Not required before GA release, and not going to work on before GA labels Aug 25, 2020
@Oberon00
Copy link
Member

This might require #510, I believe that is the part we need to investigate before GA.

@reyang reyang added release:required-for-ga Must be resolved before GA release, or nice to have before GA priority:p2 Medium priority level labels Aug 28, 2020
@Oberon00
Copy link
Member

Oberon00 commented Sep 2, 2020

After some thinking, I think that the answer to "How does Span get entries from CorrelationContext/Baggage" is: Not at all. Why should it? The Context contains these entries, and propagators operate on Context, not on Span(Context). Unless you want to export the Baggage/CorrelationContext along with the span, which you'd probably want. Then something like #875 is required.

This cannot be solved with a span processor, since

  1. The current context when starting the span is not required to be the parent context (we could change that though!)
  2. According to the spec, the notion of a "current" context is an optional extension, so some languages will only have an explicitly passed around Context object (like Go).

@Oberon00
Copy link
Member

Oberon00 commented Sep 2, 2020

@cijothomas What are you trying to achieve? From the issue description, just saying "Span is not associated with Baggage" would answer your question with no action required. What do you want to achieve with the Baggage?

@cijothomas
Copy link
Member Author

@Oberon00 My understanding is that Baggage is used to annotate telemetry. (span/metric etc). So how does one use Baggage to annotate telemetry?
If my Baggage contains ("mykey", "myvalue") - apart from being propagated, what purpose does it serve, If I am not able to add the Baggage entry to my Span attributes?

@Oberon00
Copy link
Member

Oberon00 commented Sep 2, 2020

You can read it back in the application, given the Context, can't you?

@cijothomas
Copy link
Member Author

You can read it back in the application, given the Context, can't you?

Yes surely. So my question is, "how do I use it to annotate telemetry"?

@Oberon00
Copy link
Member

Oberon00 commented Sep 2, 2020

It seems that this is indeed impossible currently and might require breaking changes to tracing API to support. @open-telemetry/technical-committee should this be raised to P1?

@Oberon00 Oberon00 added priority:p1 Highest priority level and removed priority:p2 Medium priority level labels Sep 3, 2020
@Oberon00
Copy link
Member

Oberon00 commented Sep 3, 2020

I think if this is not P1, justification is required so I'm rising this one's priority for now.

@Oberon00 Oberon00 added spec:trace Related to the specification/trace directory area:api Cross language API specification issue area:sdk Related to the SDK bug Something isn't working labels Sep 3, 2020
@Oberon00
Copy link
Member

Oberon00 commented Sep 4, 2020

With #875 each span would have an associated parent Context from which the baggage could be read to enrich the span. However, a span can be active in multiple contexts each of which can have a different baggage. Also, Spans do not keep track of which contexts they are in, and at the time they are ended they are usually not active in any context anymore, even if they were in some contexts during their started time.

So I think all we can do is associate the baggage of the parent context. You can explicitly create a new context as parent that keeps the original parent span but just assigns new baggage. So I think #875 is the best solution for this, and also a complete solution as far as the core SDK is concerned. Exporters can then use the parent Context stored on the span to enrich the exported data. Or a SpanProcessor could do it if we extended their capabilities in that direction. #669 (comment)

@andrewhsu
Copy link
Member

From the issue triage mtg today with @carlosalberto, assigning to @bogdandrutu as he has more context on this issue and can make a final call.

@bogdandrutu
Copy link
Member

@cijothomas @Oberon00 for API I think #875 resolves the issue of being able to have access to "baggages" at the start of the Span. Once that is solved I think the rest of the issue of how SDK annotates the span using attributes or simply capturing all the baggages can be downgraded to p2 in terms of urgency. Do we all agree on that?

Personally I would suggest to have a "baggage" entry in the Span and not use "attributes", here are some reasonings:

  1. some of these entries are common between different spans in the same trace, and can be used by exporter/backend to do smart things like different ways to index, compressing them in different ways etc.
  2. there is a clear "scope" separation, "attributes" are Span scoped, "baggages" are request scoped.

I think we can discuss this as a followup after we push the required API changes.

@Oberon00 Oberon00 added priority:p2 Medium priority level and removed priority:p1 Highest priority level labels Sep 22, 2020
@andrewhsu andrewhsu added release:after-ga Not required before GA release, and not going to work on before GA and removed priority:p2 Medium priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA labels Sep 25, 2020
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 area:sdk Related to the SDK release:after-ga Not required before GA release, and not going to work on before GA spec:baggage Related to the specification/baggage directory spec:trace Related to the specification/trace directory
Projects
None yet
Development

No branches or pull requests

5 participants