Add CorrelationContext and stub Context API #379

codeboten · 2020-01-26T23:48:54Z

This code will rely on the Context Propagation work to be fully implemented, but breaking the work into separate PRs to make the review process a bit easier. This implements part of otep #66, this is tracked in the spec here: open-telemetry/opentelemetry-specification#420

To note in this PR:

DistributedContext-specific propagation has been removed, as this will be implemented in the Propagation API.
Context specific changes (with_current) will be implemented as part of the Context API changes
The ContextAPI object implemented here is just a temporary class, will be fully implemented with the rest of the Context Prop. changes

Signed-off-by: Alex Boten [email protected]

This code will rely on the Context Propagation work to be fully implemented, but breaking the work into separate PRs to make the review process a bit easier. Signed-off-by: Alex Boten <[email protected]>

Oberon00 · 2020-01-28T11:44:54Z

opentelemetry-api/src/opentelemetry/correlationcontext/__init__.py

+        # pylint: disable=no-self-use
+        return context.set_value(CORRELATION_CONTEXT_KEY, {})
+
+    def _copy(self) -> typing.Dict[str, typing.Any]:


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. ~~Alternatively, maybe this is the point where we should really use the contextvars backport (see #101).~~ (OK, contextvars backport is only for 3.6) The cool thing about contextvars is that it has an immutable context API, just like the OTEP's design.

Oberon00 · 2020-01-28T11:47:16Z

opentelemetry-api/src/opentelemetry/context/__init__.py

@@ -150,3 +152,20 @@ def __repr__(self):
    from .thread_local_context import ThreadLocalRuntimeContext

    Context = ThreadLocalRuntimeContext()
+
+
+class ContextAPI:


See https://github.com/open-telemetry/opentelemetry-python/pull/379/files#r371754360. We might want to stay closer to contextvars.

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.

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.

c24t

I think I need to understand the motivation behind multiple contexts to understand the design of the correlation context API here. I'll review again after taking another pass at the spec.

c24t · 2020-01-28T19:27:19Z

opentelemetry-api/src/opentelemetry/correlationcontext/__init__.py

+
+    def get_correlations(self) -> typing.Dict:
+        """
+        Returns the entries in this CorrelationContext. The order of entries is not significant. Based


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?

c24t · 2020-01-28T19:46:18Z

opentelemetry-api/src/opentelemetry/correlationcontext/__init__.py

+
+    def get_correlation(
+        self, context: ContextAPI, name: str
+    ) -> typing.Optional[typing.Any]:


Nit: this is the same type without the Optional (unless Any is a placeholder in this PR for some real type in the future).

c24t · 2020-01-28T21:09:31Z

opentelemetry-api/src/opentelemetry/context/__init__.py

@@ -150,3 +152,20 @@ def __repr__(self):
    from .thread_local_context import ThreadLocalRuntimeContext

    Context = ThreadLocalRuntimeContext()
+
+
+class ContextAPI:


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.

c24t · 2020-01-28T21:47:07Z

opentelemetry-api/src/opentelemetry/correlationcontext/__init__.py

+        new_correlation_context = self._copy()
+        new_correlation_context[name] = value
+        return context.set_value(
+            CORRELATION_CONTEXT_KEY, new_correlation_context
+        )


If I understand this, a CorrelationContextManager is a snapshot of the correlation context data of a ContextAPI, with manager-style helper methods for setting that data.

But since this isn't associated with a context instance, this makes for a pretty weird API! It's surprising that set_correlation and remove_correlation don't just set/remove values from the context or this snapshot, they replace the entire context with values from the time the CorrelationContextManager was created.

c24t · 2020-01-28T21:54:12Z

opentelemetry-api/src/opentelemetry/correlationcontext/__init__.py

+            The value of the entry matching the name
+        """
+        # pylint: disable=no-self-use
+        correlation_context = context.get_value(CORRELATION_CONTEXT_KEY)


Another surprising consequence of the snapshot/context disconnect is that get_correlations and get_correlation do such different things, e.g. that ccm.get_correlations()[key] and ccm.get_correlation(key, context) only get the same value if context's values match the ones used to create ccm.

codeboten · 2020-02-03T17:33:24Z

Closing this PR. Will be opening a PR for the Context API changes only shortly.

* feat(plugin-https): patch https requests closes open-telemetry#375 add tests Signed-off-by: Olivier Albertini <[email protected]> * docs(plugin-https): add jaeger image Signed-off-by: Olivier Albertini <[email protected]> * fix: add mayurkale22 recommendations Signed-off-by: Olivier Albertini <[email protected]> * fix: add markwolff recommendations Signed-off-by: Olivier Albertini <[email protected]> * fix: file name utils * fix: add danielkhan and bg451 recommendations Signed-off-by: Olivier Albertini <[email protected]>

codeboten requested a review from a team January 26, 2020 23:48

codeboten force-pushed the rename-dctx branch 2 times, most recently from 0a48d08 to bd5afae Compare January 27, 2020 00:13

Rename DistributedContext to CorrelationContext

69f6671

This code will rely on the Context Propagation work to be fully implemented, but breaking the work into separate PRs to make the review process a bit easier. Signed-off-by: Alex Boten <[email protected]>

codeboten force-pushed the rename-dctx branch from bd5afae to 69f6671 Compare January 27, 2020 00:28

codeboten changed the title ~~Rename DistributedContext to CorrelationContext~~ Add CorrelationContext and stub Context API Jan 27, 2020

Oberon00 reviewed Jan 28, 2020

View reviewed changes

codeboten changed the base branch from master to codeboten/context January 28, 2020 16:26

c24t reviewed Jan 28, 2020

View reviewed changes

c24t added the needs reviewers PRs with this label are ready for review and needs people to review to move forward. label Jan 30, 2020

codeboten closed this Feb 3, 2020

codeboten deleted the rename-dctx branch October 9, 2020 18:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CorrelationContext and stub Context API #379

Add CorrelationContext and stub Context API #379

codeboten commented Jan 26, 2020

Oberon00 Jan 28, 2020 •

edited

Loading

Oberon00 Jan 28, 2020

codeboten Jan 28, 2020

c24t Jan 28, 2020

c24t left a comment

c24t Jan 28, 2020

c24t Jan 28, 2020

c24t Jan 28, 2020

c24t Jan 28, 2020

c24t Jan 28, 2020

codeboten commented Feb 3, 2020

Add CorrelationContext and stub Context API #379

Add CorrelationContext and stub Context API #379

Conversation

codeboten commented Jan 26, 2020

Oberon00 Jan 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c24t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeboten commented Feb 3, 2020

Oberon00 Jan 28, 2020 •

edited

Loading