API: Tags vs. Baggage - Githubissues

SergeyKanzhelev commented 5 years ago

from https://docs.google.com/document/d/1WhmNrl7ZGBTYAyo23OjTW7IXj2G4AtsTcwv3Zieg2tk/edit#

OT: Tied to the Span, and part of SpanContext (which is immutable).

OC: Exist independently, can be propagated without a Span. Possible approaches: Have Baggage/Tags decoupled from the Span/SpanContext and have it stand independently (NOTE: this seems to be the path we will take). Advantage: Simplicity, decoupling. Disadvantage: Would need to provide a backwards layer for OT-compliant Tracers using the current approach. Have Baggage/Tags moved to SpanContext.

SergeyKanzhelev commented 5 years ago

questions:

Common name
Do we have "Get" on baggage
Is Baggage on spans or separate API?

bogdandrutu commented 5 years ago

Everyone agrees having baggage/tags separate from the SpanContext.

bogdandrutu commented 5 years ago

Decision: Tags will support get by key, (maybe an iterator).

bogdandrutu commented 5 years ago

Sergey: Proposed the default propagation to be local in the process.

bogdandrutu commented 5 years ago

Decision: Everything limited (key, value), see OpenCensus as a starting point.

bogdandrutu commented 5 years ago

Decision: Rename to baggage for the moment, may be revisited.

SergeyKanzhelev commented 5 years ago

So we can keep the Tags name if we will make local TTL as a default

tedsuo commented 5 years ago

@pavolloffay @carlosalberto @yurishkuro there has been a lot of push back against calling this Baggage, and we are considering sticking with Tags as the name for this. By making the default propagation context local, this should not be too bad.

yurishkuro commented 5 years ago

there has been a lot of push back ... baggage ...

Can we make the push back public and with arguments?

Local-only tags do not overlap with baggage functionality, so I don't think it addresses the decision here.

Tentative name for baggage in w3c is correlation context, I'd be fine to go with this name as an abstraction, eg in Go it could be

ctx = corrctx.Set(ctx, key, value)

tedsuo commented 5 years ago

Let's make sure there's alignment:

Tags is a good name for metrics-related labeling
But Baggage has more use cases than Tags.

yurishkuro commented 5 years ago

My proposal was to keep Tags as an API layer for managing metrics-related labeling, but the inter-process context propagation mechanism should have more general name, preferably "correlation context" to be in sync with https://github.com/w3c/correlation-context

Here are a few of examples of correlation context that don't make sense to be called tags:

tenancy of the request at Uber is encoded as a json blob with many different attributes
pivot tracing
fault injection instructions for lineage-driven chaos engineering

tedsuo commented 5 years ago

Conclusion

API: We want only one package, which includes getters, and supports both use cases (tagging metrics and user baggage).

Naming: This will stay Tags for now because we don't have time to pick a name before Monday. But the name is still under discussion, Tags is only a placeholder for now.

yurishkuro commented 5 years ago

Corresponding discussion in Specification repo: https://github.com/open-telemetry/opentelemetry-specification/issues/9

yurishkuro commented 5 years ago

It seems we agreed more than once that "tags" is very misleading name. I suggested "correlation context" earlier https://github.com/open-telemetry/opentelemetry-java/issues/11#issuecomment-484604871, even though I have not seen any stated arguments against "baggage" (except for some thumbs-down, which I don't consider an "argument").

bogdandrutu commented 5 years ago

Correlation context sounds like a long name, I would like to see a proposal that includes the name of:

Package - current tags
The map of elements - current TagMap
The element - current Tag

It will be good to have all of these, for "correlation context" I have a hard time finding these names.

tedsuo commented 5 years ago

Discussion 6/4

we want a name that is clear
we don't want "metaphorical names" like "mailman" or "baggage"
Tags is too metrics-specific, and a confusing re-mapping of an OpenTracing term.
The View object is where observability comes in. It is a metrics-specfic application of this more general propagation mechanism. So this package should not have a metrics-specfic name.
This is related to in-process context propagation, so they should have "twin" names.

So, we're choosing DistributedContext as the name for this. We recommend changing the w3c proposal from CorrelationContext to DistributedContext for clarity.

Package - dctx
Map of elements - DistributedContext
The elements - Attribute

Because we need to get a first draft of Java (and the spec) out the door, we'd prefer to not litigate this issue any further, at least not as a blocker for the first draft.

bogdandrutu commented 5 years ago

What about ObservabilityContext, TelemetryContext?

yurishkuro commented 5 years ago

@bogdandrutu I think the main idea is that DistributedContext (correlation context) is a general mechanism, not specific to telemetry. The needs of telemetry can be limited to views that read data from general context.

tedsuo commented 5 years ago

Update:

changed the name of Attribute to Entry, as their signature is different from Span Attributes.
clarified package name.

New name:

Package - dctx or distributedcontext, depending on the language norms.
Map of elements - DistributedContext
The elements - Entry

jmacd commented 5 years ago

My objections to the name "DistributedContext" are:

(1) It looks like it does not contain a SpanContext. It's not a complete distributed context without a span context included. If we include the SpanContext, or establish a 1:1 correspondence between a structured tag value and a span context, then it would be at least accurate.

For example, if we adopted structured values (https://github.com/open-telemetry/opentelemetry-specification/issues/76), then a SpanContext could be included in a DistrubutedContext as:

"span.context": {
  "@type": "opentelemetry.org/v1/span_context",
  "span_id": "................",
  "trace_id": "................"
}

It feels likely add to confusion if something called DistributedContext isn't actually the complete distributed context. If we make it a container for a map of keys to entries plus a SpanContext, or establish the equivalence shown above, then it's a sensible name, to me.

My second objection is just that it's a terribly long name. TagMap is far more descriptive of the actual function performed by this class, as it stands.

bogdandrutu commented 5 years ago

We do rename this for the moment, but we want to make it clear that this is an initial proposal and we are open to discuss a better name in the specs repo and specs SIG meetings.

We are doing the renaming to close the initial API proposal milestone.

flodetan commented 1 year ago

hi @bogdandrutu @SergeyKanzhelev I have some information propagated through baggage, I want to put baggage to Span attribute to make information be visible in jaeger UI. How can i implement this feature ?

PS : I already tried in SimpleSpanProcessor.onStart() but baggage is empty, and SimpleSpanProcessor.onEnd() but no context provided. Also tried in w3cTraceContextPropagator and w3cBaggagePropagator , but it doesn't provide dataobject to set baggage.

open-telemetry / opentelemetry-java

API: Tags vs. Baggage #11

Conclusion

Discussion 6/4