Clarify where Context.Key lives, and API to insert/extract from a Context instance #1020
Conversation
There was a problem hiding this comment.
I have been brainstorming integrating trace context methods into Span itself
static Span.current()
Scope Span.makeCurrent()
static Span fromContext(Context ctx)
Context addToContext(Context ctx)I think this could be a simpler Java API where static util methods are not so idiomatic. Does the spec preclude merging pieces pieces together (Span and TraceContextUtils) in this sort of way?
|
@anuraaga please add this comment to the issue linked by this PR. You still need 2 static methods in your example, not sure if that solves that problem, but let's discuss in the issue where I have more examples and explanations. |
carlosalberto
added
release:required-for-ga
| - Set the currently active span | ||
| - Get the currently active `Span` | ||
| - Set the currently active `Span` | ||
| - Extract the `Span` from a `Context` instance |
There was a problem hiding this comment.
Should we also allow (not necessarily recommend) to just expose the key instead? With that, all the methods provided here should "be equivalent to a single parameterized method call of the Context management system".
There was a problem hiding this comment.
We discussed that, please look for history in the context PR why not to do that. So the answer is NO, https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/context/context.md#create-a-key
…text instance Signed-off-by: Bogdan Drutu <[email protected]>
Signed-off-by: Bogdan Drutu <[email protected]>
bogdandrutu
force-pushed
the
contextkey
branch
from
4d550ee to
3b9184b
Compare
Signed-off-by: Bogdan Drutu <[email protected]>
| @@ -133,19 +133,26 @@ returned `Tracer`s and actively update their configuration if it changes. | |||
| ## Tracing Context Utilities | |||
|
|
|||
| `Tracing Context Utilities` contains all operations within tracing that | |||
| modify the [`Context`](../context/context.md). | |||
| modify the [`Context`](../context/context.md). It also holds the reference to | |||
| the `Context.Key` (the name MAY be `"span_context_key"`). | |||
There was a problem hiding this comment.
It's unclear what Context.Key is. How about this?
| the `Context.Key` (the name MAY be `"span_context_key"`). | |
| the `Context` key used for storing the currently active span | |
| (usually called `Context.Key` or `span_context_key` in implementations). |
There was a problem hiding this comment.
It is not necessary to store the active Span. Is to store a span in the context. The fact that context is active or not is an orthogonal thing.
There was a problem hiding this comment.
What about the "the span active in a particular Context"?
There was a problem hiding this comment.
I guess reference to context key is too much of an implementation detail. The semantic way of saying it is, "the only way of setting a span in a context is through methods on the trace context utilities". Does that sound right?
Note, of course I'm hoping the "trace context utilities" are allowed to be merged into other concepts like TracerProvider if it makes sense for the language as per my previous comment ;)
Co-authored-by: Armin Ruech <[email protected]>
|
Update from the TC meeting: @bogdandrutu will re-do this PR to show the desired final state of the world. |
|
This is no longer needed, a different PR has been filed that fixes the entire issue, see #1063 |
Signed-off-by: Bogdan Drutu [email protected]
Updates #1019