Implement CreateKey Functionality #1853

euniceek · 2021-05-15T00:41:20Z

This PR implements the CreateKey functionality such that it aligns partially with the context specification and does not make any breaking changes to the API.

Like before, the Opentelemetry-python context uses strings as input to keys for context. However, changes have been made such that it now first goes through create_key to generate a unique key for every inputted key names.

Multiple calls to CreateKey with the same name SHOULD NOT return the same value unless language constraints dictate otherwise. Different languages may impose different restrictions on the expected types, so this parameter remains an implementation detail.

The following part of the specification is not met in order to avoid breaking changes in the API.

The API MUST return an opaque object representing the newly created key.

Key implementation detail:

create_key method to create a new unique key string every time it is called.
The outputted key from create_key gets inputted into get_value and set_value methods.
Added unit tests for the create_key method.
Updated b3 and jaeger propagator unit tests to work with unique keys.

Fixes #1737

Type of change

New feature (non-breaking change which adds functionality)

How Has This Been Tested?

New unit tests were added to ensure that same key name does not generate the same key.
tox was ran to ensure the new and existing unit tests all pass.

Checklist:

Followed the style guidelines of this project
Changelogs have been updated
Unit tests have been added

cc @alolita

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

codeboten

Nice! Thanks for the PR. Please update the changelog. Also are a few other places in the code where set_value is called w/ a string directly, those should probably be updated as well.

CHANGELOG.md

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

lzchen · 2021-05-17T18:44:03Z

I believe the set_value(key...)) changes need to be made in places in the contrib repo as well.

euniceek · 2021-05-17T19:27:03Z

I believe the set_value(key...)) changes need to be made in places in the contrib repo as well.

Okay will do!
Just to confirm, does this mean creating a separate PR on the python contrib repo with the relevant changes?

codeboten · 2021-05-17T23:42:19Z

I believe the set_value(key...)) changes need to be made in places in the contrib repo as well.

Okay will do!
Just to confirm, does this mean creating a separate PR on the python contrib repo with the relevant changes?

Correct. In order for the CI to pass, you'll need to ensure the git SHA is updated in the PR as per the steps in the contributing doc: https://github.com/open-telemetry/opentelemetry-python/blob/main/CONTRIBUTING.md#how-to-send-pull-requests

tox.ini

codeboten

The changes look great! Please fix the lint issues and we should be good to merge soon. @owais please review

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

aabmass · 2021-06-10T17:04:14Z

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

@@ -68,6 +69,18 @@ def wrapper(  # type: ignore[misc]
    return typing.cast(_F, wrapper)  # type: ignore[misc]


+def _create_key(keyname: str) -> str:


This is just a suggestion if its easy enough to implement – the return type for the key seems like a great fit for a NewType (e.g. ContextKey = NewType('ContextKey', str)). Then we can annotate where a ContextKey is expected so its clear to users and mypy can enforce it.

Then you can add an overload for set_value(key: ContextKey, ...) and we can remove the original str key overload in the next major version.

aabmass · 2021-06-10T17:09:34Z

opentelemetry-api/src/opentelemetry/trace/propagation/__init__.py

 from opentelemetry.context.context import Context
 from opentelemetry.trace.span import INVALID_SPAN, Span

-SPAN_KEY = "current-span"


Is this not a breaking API change? cc @ocelotl

Probably this was changed to avoid the public api test cases from breaking, SPAN_KEY should remain public and the tests will pass after the corresponding label is added. @eunice98k can you please fix this?

The public API check is passing on the current commit, shouldn't it have caught this? Or does it only check for additions atm?

It only checks for additions. The script is not meant to detect breaking changes, but to warn us when a possibly unnecessarily new public symbol is being added, to avoid our API from growing without need.

ocelotl · 2021-06-10T19:13:32Z

opentelemetry-api/src/opentelemetry/trace/propagation/__init__.py

 from opentelemetry.context.context import Context
 from opentelemetry.trace.span import INVALID_SPAN, Span

-SPAN_KEY = "current-span"


Probably this was changed to avoid the public api test cases from breaking, SPAN_KEY should remain public and the tests will pass after the corresponding label is added. @eunice98k can you please fix this?

lzchen · 2021-06-11T16:07:55Z

opentelemetry-api/src/opentelemetry/trace/propagation/__init__.py

 from opentelemetry.context.context import Context
 from opentelemetry.trace.span import INVALID_SPAN, Span

-SPAN_KEY = "current-span"
+SPAN_KEY = create_key("current-span")


This is still breaking. You should leave SPAN_KEY = "current-span" add a variable _SPAN_KEY = create_key("current_span") which will be used internally.

lzchen · 2021-06-11T16:08:47Z

shim/opentelemetry-opentracing-shim/src/opentelemetry/shim/opentracing_shim/__init__.py

@@ -118,6 +125,7 @@

 ValueT = TypeVar("ValueT", int, float, bool, str)
 logger = logging.getLogger(__name__)
+SHIM_KEY = create_key("scope_shim")


Suggested change

SHIM_KEY = create_key("scope_shim")

_SHIM_KEY= create_key("scope_shim")

euniceek added 3 commits May 14, 2021 14:49

Add create key functionality

6afb10c

Update unit tests

edbe22b

Update create_key comment

1ec001d

euniceek requested review from a team, owais and srikanthccv and removed request for a team May 15, 2021 00:41

owais approved these changes May 15, 2021

View reviewed changes

owais requested changes May 15, 2021

View reviewed changes

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

Change key to reflect key name

9010487

codeboten reviewed May 17, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

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

euniceek added 2 commits May 17, 2021 09:29

Update CHANGELOG

95af93d

Update set_value inputs to key

fb86f6f

Fix lint errors

1f59f5b

This was referenced May 18, 2021

Add create context key to contrib open-o11y/opentelemetry-python-contrib#16

Closed

Add create context key to contrib open-telemetry/opentelemetry-python-contrib#502

Merged

euniceek and others added 11 commits May 21, 2021 15:34

Add create key functionality

a939327

Update unit tests

20257df

Update create_key comment

a8741ab

Change key to reflect key name

657e219

Update CHANGELOG

065a3c5

Update set_value inputs to key

669544b

Fix lint errors

43b60d3

Update contrib repo git SHA

5ceae8e

Merge branch 'open-telemetry:main' into create-context-key

c00af0a

Update contrib repo SHA

df7c79c

Merge branch 'open-telemetry:main' into create-context-key

0350096

Fix tox.ini

061f591

euniceek force-pushed the create-context-key branch from 58fff25 to 061f591 Compare June 6, 2021 00:51

euniceek added 3 commits June 5, 2021 22:08

Change instrumentation order in tox.ini

8e2d3a3

Add instrumentation to docs-requirements

c4cdffd

Update SHA

268f268

lzchen reviewed Jun 7, 2021

View reviewed changes

tox.ini Outdated Show resolved Hide resolved

euniceek and others added 4 commits June 7, 2021 11:38

Merge branch 'open-telemetry:main' into create-context-key

5dae28c

Fix public API errors

38ca88e

Update contrib sha and tox.ini

4cf1bf5

Change context import order

ba15cdc

euniceek force-pushed the create-context-key branch from 71a1645 to ba15cdc Compare June 7, 2021 20:46

lzchen approved these changes Jun 7, 2021

View reviewed changes

codeboten approved these changes Jun 10, 2021

View reviewed changes

codeboten requested a review from owais June 10, 2021 15:37

owais approved these changes Jun 10, 2021

View reviewed changes

aabmass reviewed Jun 10, 2021

View reviewed changes

Fix lint errors

092e341

ocelotl suggested changes Jun 10, 2021

View reviewed changes

euniceek force-pushed the create-context-key branch 2 times, most recently from 08542e1 to 092e341 Compare June 11, 2021 07:11

euniceek and others added 2 commits June 11, 2021 00:13

Revert public API errors

eaff5b2

Merge branch 'open-telemetry:main' into create-context-key

9a45def

lzchen reviewed Jun 11, 2021

View reviewed changes

euniceek and others added 2 commits June 11, 2021 09:26

Fix span_key and shim_key

4ed8eea

Merge branch 'open-telemetry:main' into create-context-key

7d67ff9

ocelotl approved these changes Jun 11, 2021

View reviewed changes

ocelotl added the Approve Public API check This label shows that the public symbols added or changed in a PR are strictly necessary label Jun 11, 2021

lzchen merged commit 540b8a5 into open-telemetry:main Jun 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement CreateKey Functionality #1853

Implement CreateKey Functionality #1853

euniceek commented May 15, 2021

codeboten left a comment

lzchen commented May 17, 2021

euniceek commented May 17, 2021

codeboten commented May 17, 2021

codeboten left a comment

aabmass Jun 10, 2021

aabmass Jun 10, 2021

ocelotl Jun 10, 2021

aabmass Jun 10, 2021

ocelotl Jun 10, 2021

ocelotl Jun 10, 2021

lzchen Jun 11, 2021

lzchen Jun 11, 2021

		@@ -68,6 +69,18 @@ def wrapper( # type: ignore[misc]
		return typing.cast(_F, wrapper) # type: ignore[misc]


		def _create_key(keyname: str) -> str:

	SHIM_KEY = create_key("scope_shim")
	_SHIM_KEY= create_key("scope_shim")

Implement CreateKey Functionality #1853

Implement CreateKey Functionality #1853

Conversation

euniceek commented May 15, 2021

codeboten left a comment

Choose a reason for hiding this comment

lzchen commented May 17, 2021

euniceek commented May 17, 2021

codeboten commented May 17, 2021

codeboten 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment