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.

Sign up for GitHub

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

Jump to bottom

Proposal to rename distributed context and add support for baggage. #36

Closed
wants to merge 1 commit into from
Closed

Proposal to rename distributed context and add support for baggage. #36

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,5 @@

# Text formatting and spelling tools.
.tools

.DS_Store
59 changes: 59 additions & 0 deletions text/0000-rename-distributed-ctx.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
# Rename distributed context into tags and add support for Baggage.

**Status:** `proposed`

Rename Distributed-context to a less confusing term and restrict its use for observability.


## Motivation

Context Propagation is generalized to carry observability and non-observability contexts. This
includes SpanContext, Distributed-context, Authentication-context, etc. Distributed-context
carries key-value pairs for observability. The name Distributed-context is
very confusing. Hence it should be changed to TagMap (or other better suggestion).
That is the first motivation.

The second motivation is the restrict use of Distributed-context to observability. As it is
written currently it does not specifically call out for using it only for non-observability purpose.
However, it can be used to support 'Baggage' functionality for non-observability purpose as it is
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how is "non-observability" defined? I would imagine here that if Baggage refers to the key-values pairs that carry with the trace, then those would contribute to observability since they help define the context of the request.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

non-observability should be defined as data that needs to propagate downstream along with the request for usage specific to application, testing, etc. It is not tied to whether the request is being traced/monitored or not.

Copy link
Member

@toumorokoshi toumorokoshi Sep 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it! might be good to add that definition somewhere, unless there is a definition floating in the spec or other document.

OpenTracing.
There are few reasons to have different mechanism to address observability and
non-observability use cases.
1. For observability, key-value pairs SHOULD be write-only from application perspective. SDKs can read them
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is write-only desirable? I figured distributedcontext entries would be something I could pipe into metrics as I see fit.

For example, if I used this to propagate my originating service, There's cases I would want metrics to also have the originating service, as I may want to split my metric by that dimension.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SDK can read it so it can be applied to metrics.

Copy link
Member

@toumorokoshi toumorokoshi Sep 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you say the sdk, are you referring to the exporters?
In the case of metrics, I think a lot of the dimensions will be specified and passed in during the invocation (e.g. during the creation of a TimeSeries (currently terminology) or Handler (new RFC-based terminology)). In that world, DistributedContext would need to be readable by the user:

# create the main counter
request_count = Counter()

# in the request, get the relevant time series and add the value.
time_series = request_count.get_time_series({"service": DistributedContextSingleton.get("service"))
time_series.add(1)

Although it is possible for the exporter to have access to the values, there's no easy mechanism to tell the exporter which labels matter.

This behavior is also valuable because metrics will pre-aggregate, so they need values to determine the aggregation before the measurement even reaches the exporter.

for example to apply them to measurements at the time of recording. For non-observability,
key-value pairs SHOULD be writable/readable for application.
2. For non-observability, it may require that the delivery is guaranteed and pairs are not dropped.
3. Semantically two are different and mechanically it could be different as well.

## Explanation

We do following to
- Rename distributed-context to TagMap.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also rename DistributedContext.Entry to Tag, key and value to TagKey, TagValue.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure. My intention is to restore TagMap as defined in OC

- It is strictly used for observability.
- It is a write-only object from application and framework-integrations perspective.
- It is readable by SDKs.

- Add Baggage support. Equivalent of what is there in [OpenTracing](https://opentracing.io/docs/overview/tags-logs-baggage/#baggage-items).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is unclear when to put an item in Baggage versus TagMap.
Also, if something started as an item in TagMap, later there are needs to also put it Baggage, do we move the item or we duplicate the item?

A meta question - do we believe that OpenTelemetry should provide such general purpose Baggage so people can use it for things such as "authentication context"?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is unclear when to put an item in Baggage versus TagMap.

As I read this OTEP, that is quite clear: An application puts a value in the TagMap if it wants to have spans, metrics, etc tagged with that value. The baggage on the other hand is used when the value should not be attached to the telemetry data but if the application itself (or another service invoked by it) wants to read the value back, for application-specific purposes.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, if something started as an item in TagMap, later there are needs to also put it Baggage, do we move the item or we duplicate the item?

No. Two are independent. Yes, there is duplication as mentioned in Trade-offs section but it is not worth to optimize it.

A meta question - do we believe that OpenTelemetry should provide such general purpose Baggage so people can use it for things such as "authentication context"?

I agree that from observability perspective, 'Baggage' doesn't belong in OpenTelemetry. However, from POV of opentracing compatibility we need to provide some solution. It could very well be in another repo/contrib package.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. Two are independent. Yes, there is duplication as mentioned in Trade-offs section but it is not worth to optimize it.

I see, now it is clear to me. Thanks.

I agree that from observability perspective, 'Baggage' doesn't belong in OpenTelemetry. However, from POV of opentracing compatibility we need to provide some solution. It could very well be in another repo/contrib package.

Makes sense. If this for compatibility, do we encourage more people to adopt it, or we suggest people to move away from it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Baggage is just context propagation. Context propagation is not a part of OpenTelemetry, it's the layer on top of which OpenTelemetry is built.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would vote for not including a non-telemetry related concept directly into opentelemetry. If Baggage is needed for opentracing having it be in contrib makes sense.

Will SaaS vendors need to use this as well? authentication or identification seems like a common concept.

- It is more of a convenient functionality to provide compatibility with Open Tracing.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- It is more of a convenient functionality to provide compatibility with Open Tracing.
- It is more of a convenient functionality to provide compatibility with OpenTracing.

- It is propagated downstream but may require separate propagator (different from one used for
SpanContext and TagMap
- It is readable by application. **Open question**: should it be mutable or immutable?

## Trade-offs and mitigations

There may be cases where a key-value pair is propagated as TagMap for observability and as a Baggage for
application specific use. AB testing is one example of such use case. There is a duplication here at
call site where a pair is created and also at propagation. Solving this issue is not worth having
semantic confusion with dual purpose.


## References

[OpenTracing Baggage](https://opentracing.io/docs/overview/tags-logs-baggage/#baggage-items)

[OpenCensus Tag](https://github.com/census-instrumentation/opencensus-specs/blob/master/tags/TagMap.md)

## Open questions