Add initial DistributedContext implementation. #92

a-feld · 2019-08-16T22:04:23Z

This PR fixes issue #91 by attempting to implement the distributed context api specification.

The work is based somewhat on the opentelemetry-ruby implementation.

This PR hasn't yet implemented the propagator APIs for DistributedContext. The intent is that when this PR is ready for review a stub will have been implemented for propagators in distributed context.

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

opentelemetry-sdk/src/opentelemetry/sdk/distributedcontext/__init__.py

reyang

I've left comments on the parsing logic.
We might want to refer to the draft CorrelationContext spec.

toumorokoshi

IMO the one change needed is to adhere to ABCs for unimplemented APIs.

As an aside, I think #89 and this PR are going to conflict a bit around the context singleton specifically. I think options are:

we merge this PR in and unifying propagator formatters #89 might refactor this context var depending on how that turns out.
we remove the distributedcontext singleton specifically (keep the classes which all look awesome) and have unifying propagator formatters #89 handle adding the appropriate context var.

I'm okay with either. looks great!

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

toumorokoshi · 2019-08-17T04:42:37Z

opentelemetry-sdk/src/opentelemetry/sdk/distributedcontext/__init__.py

+
+from opentelemetry import distributedcontext as dctx_api
+
+_CURRENT_DISTRIBUTEDCONTEXT_CV = contextvars.ContextVar(


I think we should use the existing context object that exists in the API: https://github.com/open-telemetry/opentelemetry-python/blob/master/opentelemetry-api/src/opentelemetry/context/__init__.py

There will be a need to supplement raw contextvars with other mechanisms to support python 3.4 up to 3.7

toumorokoshi · 2019-08-17T04:42:55Z

opentelemetry-sdk/src/opentelemetry/sdk/distributedcontext/__init__.py

+    def get_current_context(self) -> typing.Optional[DistributedContext]:
+        return self._cv.get(default=None)
+
+    @contextmanager


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

carlosalberto · 2019-08-21T21:15:25Z

I'm wondering if we could keep EntryKey and EntryValue as strings, as we have been simplifying things in other parts of the API (in such case, validation would need to happen in other parts, such as when Entry is created).

a-feld · 2019-08-21T21:20:26Z

I'm wondering if we could keep EntryKey and EntryValue as strings, as we have been simplifying things in other parts of the API (in such case, validation would need to happen in other parts, such as when Entry is created).

@carlosalberto That's basically what's happening 😄

EntryKey and EntryValue are effectively strings. The __new__ will return the validated string, avoiding allocation while allowing the interpreter to keep optimizations like string interning.

The benefit of the current approach is that the type checker can ensure that only validated strings are passed into the API while maintaining the optimizations provided by using raw str types.

carlosalberto · 2019-08-21T22:05:39Z

@a-feld Oh yes! but I was talking of even removing this part - for example, we don't have such class for SpanId, but we simply use ints all over the place. Something to consider (maybe @c24t has some input next week ;) )

(although the checking is indeed very nice!)

reyang · 2019-08-27T00:35:08Z

opentelemetry-api/src/opentelemetry/distributedcontext/propagation/httptextformat.py

+
+        PROPAGATOR = HTTPTextFormat()
+
+


minor: remove extra line

reyang

LGTM.

Oberon00

There are a few small things I noticed that should be fixed before merging this.

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

Oberon00 · 2019-08-27T10:41:15Z

IMO the one change needed is to adhere to ABCs for unimplemented APIs.

-- @toumorokoshi in #92 (review)

I'm still not so sure about that policy (did we officially decide on it?). But I'm sure we decided that the API package must be usable as no-op implementation without loading any implementation from elsewhere.

mauriciovasquezbernal

Just some details I found when reading this PR.

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

mauriciovasquezbernal · 2019-08-27T14:03:12Z

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

+    UNLIMITED_PROPAGATION = -1
+
+    def __init__(self, entry_ttl: int) -> None:
+        self.entry_ttl = entry_ttl


Given that currently only NO_PROPAGATION and UNLIMITED_PROPAGATION are allowed should be a check for that?

I'm not sure there's much value in doing a runtime check for these values, especially considering the intended purpose is to actually support positive integers in the future.

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

Co-Authored-By: Reiley Yang <reyang@microsoft.com>

…_.py Co-Authored-By: Christian Neumüller <christian+github@neumueller.me>

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

a-feld · 2019-08-28T17:08:31Z

@toumorokoshi Your review is pending with requested changes. I'm thinking that option 1 mentioned in the review comment (merge this PR and revisit #89 ) may be the easiest path forward. Thoughts?

Discussed in the community meeting, we can follow up in another PR / issue.

reyang · 2019-08-29T17:42:16Z

@GeorgeJoy FYI.

reyang reviewed Aug 17, 2019

View reviewed changes

opentelemetry-api/src/opentelemetry/distributedcontext/__init__.py Outdated Show resolved Hide resolved

reyang reviewed Aug 17, 2019

View reviewed changes

opentelemetry-api/src/opentelemetry/distributedcontext/__init__.py Outdated Show resolved Hide resolved

reyang reviewed Aug 17, 2019

View reviewed changes

opentelemetry-sdk/src/opentelemetry/sdk/distributedcontext/__init__.py Outdated Show resolved Hide resolved

reyang requested changes Aug 17, 2019

View reviewed changes

toumorokoshi previously requested changes Aug 17, 2019

View reviewed changes

a-feld force-pushed the distributed-context branch 2 times, most recently from 036d8ac to 701fccd Compare August 21, 2019 16:46

carlosalberto reviewed Aug 21, 2019

View reviewed changes

opentelemetry-api/src/opentelemetry/distributedcontext/__init__.py Outdated Show resolved Hide resolved

a-feld force-pushed the distributed-context branch from 374a771 to 8f7c530 Compare August 26, 2019 22:08

a-feld marked this pull request as ready for review August 26, 2019 22:48

a-feld requested review from c24t, lzchen and Oberon00 as code owners August 26, 2019 22:48

a-feld requested review from reyang and toumorokoshi August 26, 2019 22:59

a-feld force-pushed the distributed-context branch from 8e4d93f to 634e7db Compare August 26, 2019 23:49

reyang reviewed Aug 27, 2019

View reviewed changes

reyang approved these changes Aug 27, 2019

View reviewed changes

Oberon00 suggested changes Aug 27, 2019

View reviewed changes

mauriciovasquezbernal reviewed Aug 27, 2019

View reviewed changes

a-feld and others added 6 commits August 27, 2019 10:19

Add initial DistributedContext implementation.

747d100

Put length check first.

8595833

Co-Authored-By: Reiley Yang <reyang@microsoft.com>

Improve performance.

6b163db

Update distributed context documentation.

51369ab

Allow EntryKey and EntryValue to be used as str.

f3b0096

Update SDK to use context.

dc1fdeb

a-feld and others added 12 commits August 27, 2019 10:19

Fix EntryMetadata.

30c47e8

Change EntryMetadata to object type.

d1bca0e

Add api for distributed context propagation.

94decac

Add initial API testing for distributed context.

9225b00

Fix lint issues.

633e13e

Fix type errors.

face08d

Add SDK distributed context testing.

a804f5e

Change the definition of printable.

e5b52f5

Move DistributedContext implementation into api level.

135b134

Fix mypy issues.

5de7f64

Update opentelemetry-api/src/opentelemetry/distributedcontext/__init_…

23ea35d

…_.py Co-Authored-By: Christian Neumüller <christian+github@neumueller.me>

Return EntryValue from get_entry_value.

7eab1e8

a-feld force-pushed the distributed-context branch from 289066c to 7eab1e8 Compare August 27, 2019 17:19

a-feld added 4 commits August 27, 2019 10:26

Fix pylint.

6d86bd3

Remove extra lines.

fd08d80

Remove magic numbers from distributed context.

938a0b3

Fail if key is empty.

ae4f570

a-feld force-pushed the distributed-context branch from 7239e4f to ae4f570 Compare August 27, 2019 17:57

Disable len-as-condition for distributedcontext check.

5794bb8

Oberon00 approved these changes Aug 28, 2019

View reviewed changes

opentelemetry-api/src/opentelemetry/distributedcontext/__init__.py Show resolved Hide resolved

Jamim mentioned this pull request Aug 28, 2019

AsyncioScopeManager based on contextvars and supporting Tornado 6 opentracing/opentracing-python#118

Merged

reyang merged commit 12415f9 into open-telemetry:master Aug 29, 2019

a-feld deleted the distributed-context branch August 29, 2019 16:14

a-feld mentioned this pull request Aug 29, 2019

REQUEST: New membership for @a-feld open-telemetry/community#165

Closed

6 tasks

c24t mentioned this pull request Dec 3, 2019

Separate no-op from interfaces #311

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add initial DistributedContext implementation. #92

Add initial DistributedContext implementation. #92

a-feld commented Aug 16, 2019

reyang left a comment

toumorokoshi left a comment

toumorokoshi Aug 17, 2019

toumorokoshi Aug 17, 2019

carlosalberto commented Aug 21, 2019

a-feld commented Aug 21, 2019

carlosalberto commented Aug 21, 2019

reyang Aug 27, 2019

reyang left a comment

Oberon00 left a comment

Oberon00 commented Aug 27, 2019

mauriciovasquezbernal left a comment

mauriciovasquezbernal Aug 27, 2019

a-feld Aug 27, 2019

a-feld commented Aug 28, 2019 •

edited

Loading

reyang commented Aug 29, 2019


		from opentelemetry import distributedcontext as dctx_api

		_CURRENT_DISTRIBUTEDCONTEXT_CV = contextvars.ContextVar(

Add initial DistributedContext implementation. #92

Add initial DistributedContext implementation. #92

Conversation

a-feld commented Aug 16, 2019

reyang left a comment

Choose a reason for hiding this comment

toumorokoshi left a comment

Choose a reason for hiding this comment

toumorokoshi Aug 17, 2019

Choose a reason for hiding this comment

toumorokoshi Aug 17, 2019

Choose a reason for hiding this comment

carlosalberto commented Aug 21, 2019

a-feld commented Aug 21, 2019

carlosalberto commented Aug 21, 2019

reyang Aug 27, 2019

Choose a reason for hiding this comment

reyang left a comment

Choose a reason for hiding this comment

Oberon00 left a comment

Choose a reason for hiding this comment

Oberon00 commented Aug 27, 2019

mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

mauriciovasquezbernal Aug 27, 2019

Choose a reason for hiding this comment

a-feld Aug 27, 2019

Choose a reason for hiding this comment

a-feld commented Aug 28, 2019 • edited Loading

reyang commented Aug 29, 2019

a-feld commented Aug 28, 2019 •

edited

Loading