-
Notifications
You must be signed in to change notification settings - Fork 656
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
Add CorrelationContext and stub Context API #379
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,128 @@ | ||
# Copyright 2019, OpenTelemetry Authors | ||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"); | ||
# you may not use this file except in compliance with the License. | ||
# You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, software | ||
# distributed under the License is distributed on an "AS IS" BASIS, | ||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
import copy | ||
import itertools | ||
import string | ||
import typing | ||
from contextlib import contextmanager | ||
|
||
from opentelemetry.context import ContextAPI | ||
|
||
CORRELATION_CONTEXT_KEY = "correlation-context-key" | ||
|
||
|
||
class CorrelationContextManager: | ||
""" | ||
Manages access to CorrelationContext entries | ||
""" | ||
|
||
def __init__( | ||
self, entries: typing.Optional[typing.Dict[str, typing.Any]] = None | ||
) -> None: | ||
if entries: | ||
self._correlation_context = copy.deepcopy(entries) | ||
else: | ||
self._correlation_context = {} | ||
|
||
def get_correlations(self) -> typing.Dict: | ||
""" | ||
Returns the entries in this CorrelationContext. The order of entries is not significant. Based | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see the docs are copied from the spec, can you add a TODO to make these specific to python once the spec is finalized? |
||
on the languagespecification, the returned value can be either an immutable collection or an | ||
immutable iterator to the collection of entries in this CorrelationContext. | ||
|
||
Returns: | ||
An immutable iterator to the collection of entries in this CorrelationContext | ||
""" | ||
return self._correlation_context.items() | ||
|
||
def get_correlation( | ||
self, context: ContextAPI, name: str | ||
) -> typing.Optional[typing.Any]: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: this is the same type without the |
||
""" | ||
To access the value for an entry by a prior event, the Correlations API provides a | ||
function which takes a context and a name as input, and returns a value. Returns | ||
the Value associated with the given Name, or null if the given Name is not present. | ||
|
||
Args: | ||
context: The context in which to find the CorrelationContext | ||
name: The name of the entry to retrieve | ||
Returns: | ||
The value of the entry matching the name | ||
""" | ||
# pylint: disable=no-self-use | ||
correlation_context = context.get_value(CORRELATION_CONTEXT_KEY) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another surprising consequence of the snapshot/context disconnect is that |
||
if correlation_context and name in correlation_context: | ||
return correlation_context[name] | ||
return None | ||
|
||
def set_correlation( | ||
self, context: ContextAPI, name: str, value: typing.Any | ||
) -> ContextAPI: | ||
""" | ||
To record the value for an entry, the Correlations API provides a function which takes a | ||
context, a name, and a value as input. Returns an updated Context which contains the new value. | ||
|
||
Args: | ||
context: The context in which to find the CorrelationContext | ||
name: The name of the entry to set | ||
value: The value of the entry | ||
Returns: | ||
A new Context object with updated correlations | ||
""" | ||
new_correlation_context = self._copy() | ||
new_correlation_context[name] = value | ||
return context.set_value( | ||
CORRELATION_CONTEXT_KEY, new_correlation_context | ||
) | ||
Comment on lines
+84
to
+88
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I understand this, a But since this isn't associated with a context instance, this makes for a pretty weird API! It's surprising that |
||
|
||
def remove_correlation(self, context: ContextAPI, name: str) -> ContextAPI: | ||
""" | ||
To delete an entry, the Correlations API provides a function which takes a context and a name as | ||
input. Returns an updated Context which no longer contains the selected Name. | ||
|
||
Args: | ||
context: The context in which to remove the CorrelationContext | ||
name: The name of the entry to remove | ||
Returns: | ||
A new Context object with the correlation removed | ||
""" | ||
new_correlation_context = self._copy() | ||
if name in new_correlation_context: | ||
del new_correlation_context[name] | ||
return context.set_value( | ||
CORRELATION_CONTEXT_KEY, new_correlation_context | ||
) | ||
|
||
def clear_correlations(self, context: ContextAPI) -> ContextAPI: | ||
""" | ||
To avoid sending any entries to an untrusted process, the Correlation API provides a function | ||
to remove all Correlations from a context. Returns an updated Context with no correlations. | ||
|
||
Args: | ||
context: The context in which to clear the CorrelationContext | ||
Returns: | ||
A new Context object with no correlations | ||
""" | ||
# pylint: disable=no-self-use | ||
return context.set_value(CORRELATION_CONTEXT_KEY, {}) | ||
|
||
def _copy(self) -> typing.Dict[str, typing.Any]: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this will be called quite often I think we should keep performance in mind and try to use the stdlib contextvars on Python versions where this is possible (is it possible to nest contexts, i.e store a context in a context slot?). If this is not possible, we might be able to reuse ideas from https://www.python.org/dev/peps/pep-0567/#implementation (which refers to https://www.python.org/dev/peps/pep-0550/#context-variables) to achieve a constant time copy function. |
||
""" | ||
Helper function to abstract the mechanism used to copy CorrelationContext values | ||
|
||
Returns: | ||
A copy of the current entries in the CorrelationContext | ||
""" | ||
return copy.deepcopy(self._correlation_context) |
This file was deleted.
This file was deleted.
This file was deleted.
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.
See https://github.com/open-telemetry/opentelemetry-python/pull/379/files#r371754360. We might want to stay closer to contextvars.
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.
Yes, that's right. This change only implements the stub context API as a way to break apart #325 to make it easier to review. The next change in this branch will be the Context API changes, followed by the Propagation API.
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.
One consideration that might affect the design here: do we need the context API to support arbitrary "concerns", or is it safe to assume adding a new concern will require a code change here? If the former, we probably want a single dict-valued
ContextVar
with a (probably also dict-valued) key for each concern. If the latter, we could have one cv for trace context and another for correlation context.