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
Adding Context API #395
Adding Context API #395
Changes from 46 commits
1500bec
3bdf1c8
e144871
569878e
55c817a
746e591
b53bdea
76a8c03
3fe5a23
1f8d67f
4bdd27c
b28fb3f
51af769
530c21a
16eb703
ddd81a5
b935bf5
15805a4
68b98e5
3a2b0ef
62c76ec
5dca7cc
7203e72
e43f168
f81a673
ba65845
ed187bf
2b8c69f
1a0217f
212b14b
dbfeb98
2235bc3
5dc9fd5
02c7c4c
ae2cbc4
6f99621
01e0054
ebcf0bd
ddf3c8d
6f9780b
49b6abd
4c5083a
5a8a9fb
d5da10c
7995ac3
dbbdefb
37f1555
81f06a9
f29111a
057c22c
5376e60
e7d0286
0cee462
f8032c1
02b8f71
727ae50
c9c66f3
f25992f
91d2ebc
335678e
619fc1b
1f9bd76
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.
If this is supposed to represent the current context, then we cant just use a global variable. We need a context-local variable (or at least a thread local variable). But this is already what the context inside does, so it seems we have mixed up "current Context" and "configured context manager".
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.
Maybe my interpretation of what
set_current
is intended to achieve is wrong. My understanding is thatset_current
is used here to update the global context which can then be referenced other places in the application with updated values rather than updating the context within a context manager. This I think can be achieved with the global here, though I might be missing something.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.
@Oberon00 , what do you mean with "configured context manager"?
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 think we have to distinguish four things:
get_value
returns).class
or object. E.g. a thread of execution, the context of a async task, a greenlet, etc. The context manager defines what an execution context is and ensures that there is a 1:1 mapping from logical execution contexts to context objects.EDIT:
set_context
should then set the context object in the current logical execution context.EDIT2: As a concrete example, consider setting a value in a context object and then passing it (e.g. via a queue) to another, already running, thread. In that case, the other thread must be able to read that value back. This is not currently the case, as our "Context" is in fact a context manager and will return the return value of
get_value(key, context)
is potentially different when called from different logical execution contexts, even for identicalcontext
arguments.EDIT3: I asked the spec folks: open-telemetry/opentelemetry-specification#424 (comment)
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.
Some clarifications:
This is currently represented by the
Context
interface which is implemented by eitherContextVarsContext
orThreadLocalContext
depending on the version of PythonThis is implemented via the
get_current
&set_current
methodsThere 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 was about to comment the same. Regarding point 1, here is the class that represents the concept of a single context object. It is implemented here, here and here.
Regarding your first EDIT, there is nothing named
set_context
. Did you meanset_current
?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.
@codeboten @ocelotl: No, ContextVarsContext and ThradLocalContext are not a contexts in the OTEP 66 sense. E.g. ThreadLocalContext is a mapping
(thread, key) -> value
instead of justkey -> value
. That's what I wanted to clarify in my EDIT2.For example, in the OTEP,
Propagator.extract
returns a context. Which return type would you use given the types in this PR, so that you can give the context to another thread or execution context and it still keeps its 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 think @Oberon00 has a good point here, and this will not work as expected.
Using his context queue example:
This works as expected, but only incidentally. If the call to
set_current
happens in one task in between theset_current
andget_current
calls in another, we get some weird behavior. Running effectively the same code, but manipulating the call order with anEvent
shows this:This prints:
Task 1 gets the context from the
set_current
call in task 2, but doesn't get its context variable values becauseset
calls are scoped to the task! You could show roughly the same thing with threads and threadlocals.(Sorry for the long example here)
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.
@ocelotl Yes, sorry I meant
set_current
.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.
Thank for the clarifications. I've updated the
Context
class to be an immutable object. The portion of theContext
class that was used to wrap threadlocal or contextvars implementation details is nowRuntimeContext
.I still need to update the PR to not use a global _CONTEXT variable, but I think this change is at least a step in the right direction.
This file was deleted.