Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Proposal to separate context propagation from observability #42
Proposal to separate context propagation from observability #42
Changes from 1 commit
dff8df9
5ad7d1c
1dc3c7b
58248e6
68cb0ba
3dc6a76
459435e
c9c64f4
c3c7c24
4588096
2d80dae
7a73210
0d8e41b
aad5605
4a930eb
e1ef61f
f949435
1cb155e
7b9e861
07eb397
0ebeb6c
147d6b0
72d4651
1472197
18a37d4
7ea1834
7317747
43ba8fd
d7d6f1c
3a817a2
3381e0f
c15a107
310e8d5
f59fc27
153b9aa
f70855a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 find this pic very confusing, sorry. There was an ascii art in one of the earlier tickets.
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.
what is an observation in this context? A single metric measurement comes to mind.
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.
This describes
In my understanding, Observe is a stand-in for e.g. starting, ending Spans, but also any metric recording.
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 guess it's still not something that immediately clicks for me. Some clarification on that, if it's not around in some other document, would be great.
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.
Thanks for the feedback, @toumorokoshi. @Oberon00 is correct, this definition means to say the details of every function in the observability system are not relevant, only that in addition to "doing what they do," they:
These are the only two ways that the various observability APIs are tied together. I will try to make this more clear in the proposal; please let me know if you agree in principle.
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.
Would it also be beneficial to have a
hoplimit
on baggage items? For example, once supported, we may want a certain baggage value only to be propagated to the downstream service and no further - or only available within the service (NO_PROPAGATION
) - but it is not to be treated as a label.Although I see the explicit methods below for achieving the same - having the hoplimit also available when setting the baggage value enables an implicit decision to be defined by the baggage creator.
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've been in general skeptical about the TTL notion, especially "single-hop" whose semantics are completely vague given that requests may pass through proxies that the caller would not know about.
There is always the underlying in-memory Context object used as a storage for all kinds of contexts. I can always add a value to that context explicitly, and retrieve it explicitly if my application needs it in-process. I don't think there's a need to provide any additional functionality here (although it's worth calling out that such capability must exist in the Context, whereas it was missing from OpenTracing Java, for example).
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.
Yeah, I agree that single hop TTL is questionable...
@objectiser I agree that if we come up with a TTL scheme beyond just
NO_PROPAGATION
andUNLIMITED_PROPAGATION
we should add it to baggage. I only left it off becauseNO_PROPAGATION
is equivalent to just using the context API directly, and that did not leave any options other thanUNLIMITED_PROPAGATION
. So it wasn't clear to me why anyone would need baggage for that purpose.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.
With the text that follows, I expected a
RegisterPropagatator
API here. Somehow this should reference the actual registration function described later.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.
So, the propagation layer provides the RegisterPropagator function. Applications provide, and applications provide the Propagators to be registered.
Is there a better way to explain this in the GetPropagator description? Right now it says "To register with the propagation system, the [BLANK] API provides a set of propagation functions for every propagation type."
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've removed the Registry concept, hopefully this is clearer.
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.
Was this meant?
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.
might be worth expanded on what Automated Context Management is like in practice. I would imagine A ThreadLocal is an example since context is not available outside of the thread it was set or modified in?
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.
👍 In general, this OTEP is very abstract, some (non-normative) examples would be helpful.
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.
Yeah, I struggle with this part. IMHO, explaining what OpenTelemetry looks like without automation is the goal of the spec, as it describes what instructions actually need to be executed in order to implement this system.
Some implementations may be able to leverage the runtime to execute these instructions without the end user having to write some or all of the code... but I'm not sure what the best way to express that would be. We can list some example runtimes, like java and thread locals, to give readers a hint, but it would be nice if there was a useful way to describe what we mean while still using the same syntax and concepts described in the spec.
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.
This section is confusing two different layers from the Context Propagation Layers design doc: the Context and In-Process Propagation, hence the problem. In languages like Go the In-Process Propagation layer is not relevant, because the context is passed explicitly. This could apply to other frameworks when people don't want to rely on thread-locals. That has nothing to do with the Context type itself that simply stores values.
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 don't think the section is confusing them. It just defines a point where they interact. The context type used here is not necessarily the same the in-process layer uses, it could translate between them.
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.
So what happens when multiple propagators are registered for the same type? I think chained propagators may be cumbersome to provide on top of an API that allows only one propagator, since it requires cooperation between everyone who wants to add a propagator to the chain.
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 have seen code that does this, by using a stack-alike
Propagator
that tries to inject/extract from a list of propagators (order is important) - for extraction, it returns with the first successful attempt, and for injection it simply tries to inject all formats. And yes, we should have a test case for this scenario, so we know where work fine.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.
So, here is the nuance: there is basic chaining provided at the API level by RegisterPropagator. All propagators are run for every type, in the order in which they are registered. This is where the cooperation happens between independent applications.
More complex propagation behavior usually ends up being specific to each application. So, the OTel SDK can provide the kind of fallback W3C -> B3 chaining for observability, described here. That sort of behavior is presented as a single, complex propagator, from the point of view of the Propagation API, since the fallback behavior is internal to a single application. Does that make sense?
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.
Curious, is this just for fun?
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.
Haha, well I wanted to address what I perceive to be a common question, along the lines of "why is this context parameter everywhere? Is it because this is a golang project? Is it required that every language must pass context this way?"
But if the humor gets in the way of learning, I can change it.
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 like it. You can also stress that it's not just a Go thing - in the old versions of Node there was no CLS (or it was extremely inefficient) and explicitly passing the context was how the propagation was achieved.
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.
Good call, I will emphasize that this issue exists in multiple languages.
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.
ahem... Context Propagation Layers https://docs.google.com/document/d/1UxrEYOaQlF_E4gtiPoFmcZ4YKKe1GxohvCvQDuwvD1I/edit