Add create context key to contrib #502

euniceek · 2021-05-21T22:18:03Z

Description

This PR makes the corresponding changes from otel-python open-telemetry/opentelemetry-python#1372 in the contrib to implement create_key functionality as noted here.

How Has This Been Tested?

tox was ran to ensure the new and existing unit tests all pass.

lzchen · 2021-05-24T16:53:47Z

CHANGELOG.md

@@ -5,7 +5,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

 ## [Unreleased](https://github.com/open-telemetry/opentelemetry-python/compare/v1.2.0-0.21b0...HEAD)
-
+- Added support for CreateKey functionality.


Nit: Please include this in the 'Added' section.

lzchen · 2021-05-24T16:54:12Z

exporter/opentelemetry-exporter-datadog/src/opentelemetry/exporter/datadog/spanprocessor.py

 from opentelemetry.sdk.trace import Span, SpanProcessor
 from opentelemetry.sdk.trace.export import SpanExporter
 from opentelemetry.trace import INVALID_TRACE_ID
 from opentelemetry.util._time import _time_ns

 logger = logging.getLogger(__name__)
+EXPORT_KEY = create_key("suppress_instrumentation")


Suggested change

EXPORT_KEY = create_key("suppress_instrumentation")

_SUPPRESS_INSTRUMENTATION_KEY= create_key("suppress_instrumentation")

codeboten

Just wanted to confirm my understanding here, the suppress_instrumentation context key was being utilized previously by the instrumentations to check if somewhere the SDK had set it. But from the looks here, this change updates the code to create separate keys for each instrumentation.

I believe this mean that this change breaks the functionality we currently have where the span process can set the suppress_instrumentation key before export to prevent the exporter from generating spans. https://github.com/open-telemetry/opentelemetry-python/blob/77c9867dcb0f3179c1b395729aae6aec2c3133a0/opentelemetry-sdk/src/opentelemetry/sdk/trace/export/__init__.py#L89

Can you confirm? If so, we should figure out the right way to move forward to not break this functionality.

euniceek · 2021-05-25T17:45:37Z

Just wanted to confirm my understanding here, the suppress_instrumentation context key was being utilized previously by the instrumentations to check if somewhere the SDK had set it. But from the looks here, this change updates the code to create separate keys for each instrumentation.

I believe this mean that this change breaks the functionality we currently have where the span process can set the suppress_instrumentation key before export to prevent the exporter from generating spans. https://github.com/open-telemetry/opentelemetry-python/blob/77c9867dcb0f3179c1b395729aae6aec2c3133a0/opentelemetry-sdk/src/opentelemetry/sdk/trace/export/__init__.py#L89

Can you confirm? If so, we should figure out the right way to move forward to not break this functionality.

Sorry I didn't know the full context behind the suppress_instrumentation before making the changes. Yes your understanding is correct in that a new key is created for each instrumentation. This was because now with the create_key functionality creates a unique key name, which is now not simply suppress_instrumentation.

What I can do is since in the otel-python PR, I am creating a unique suppress_instrumentation key, I could export this key to each instrumentation that uses it instead of calling create_key each time. (I will also rename this from EXPORT_KEY to _SUPPRESS_INSTRUMENTATION_KEY.)
https://github.com/open-telemetry/opentelemetry-python/pull/1853/files#diff-ccdfba02d42525f2fee85e7369c94b7c4976ec22079248bbddf610cd11435739R41

Let me know if you think this is alright!

ocelotl · 2021-05-26T00:02:48Z

Just wanted to confirm my understanding here, the suppress_instrumentation context key was being utilized previously by the instrumentations to check if somewhere the SDK had set it. But from the looks here, this change updates the code to create separate keys for each instrumentation.
I believe this mean that this change breaks the functionality we currently have where the span process can set the suppress_instrumentation key before export to prevent the exporter from generating spans. https://github.com/open-telemetry/opentelemetry-python/blob/77c9867dcb0f3179c1b395729aae6aec2c3133a0/opentelemetry-sdk/src/opentelemetry/sdk/trace/export/__init__.py#L89
Can you confirm? If so, we should figure out the right way to move forward to not break this functionality.

Sorry I didn't know the full context behind the suppress_instrumentation context key before making the changes. Yes your understanding is correct in that a new key is created for each instrumentation. This was because now with the create_key functionality creates a unique key name, which is now not simply suppress_instrumentation.

What I can do is since in the otel-python PR, I am creating a unique suppress_instrumentation key, I could export this key to each instrumentation that uses it instead of calling create_key each time. (I will also rename this from EXPORT_KEY to _SUPPRESS_INSTRUMENTATION_KEY.)
https://github.com/open-telemetry/opentelemetry-python/pull/1853/files#diff-ccdfba02d42525f2fee85e7369c94b7c4976ec22079248bbddf610cd11435739R41

Let me know if you think this is alright!

Yes, this makes sense 👍

codeboten · 2021-05-26T17:30:22Z

Just wanted to confirm my understanding here, the suppress_instrumentation context key was being utilized previously by the instrumentations to check if somewhere the SDK had set it. But from the looks here, this change updates the code to create separate keys for each instrumentation.
I believe this mean that this change breaks the functionality we currently have where the span process can set the suppress_instrumentation key before export to prevent the exporter from generating spans. https://github.com/open-telemetry/opentelemetry-python/blob/77c9867dcb0f3179c1b395729aae6aec2c3133a0/opentelemetry-sdk/src/opentelemetry/sdk/trace/export/__init__.py#L89
Can you confirm? If so, we should figure out the right way to move forward to not break this functionality.

Sorry I didn't know the full context behind the suppress_instrumentation before making the changes. Yes your understanding is correct in that a new key is created for each instrumentation. This was because now with the create_key functionality creates a unique key name, which is now not simply suppress_instrumentation.

What I can do is since in the otel-python PR, I am creating a unique suppress_instrumentation key, I could export this key to each instrumentation that uses it instead of calling create_key each time. (I will also rename this from EXPORT_KEY to _SUPPRESS_INSTRUMENTATION_KEY.)
https://github.com/open-telemetry/opentelemetry-python/pull/1853/files#diff-ccdfba02d42525f2fee85e7369c94b7c4976ec22079248bbddf610cd11435739R41

Let me know if you think this is alright!

The problem with exporting the key from the SDK is that the instrumentations will then depend on the SDK, which I don't believe they do today. This should likely be part of the API instead, this was discussed in the specification here: open-telemetry/opentelemetry-specification#530

I'll add this to the agenda to discuss it at Thursday's SIG meeting.

ocelotl · 2021-05-26T23:27:03Z

Just wanted to confirm my understanding here, the suppress_instrumentation context key was being utilized previously by the instrumentations to check if somewhere the SDK had set it. But from the looks here, this change updates the code to create separate keys for each instrumentation.
I believe this mean that this change breaks the functionality we currently have where the span process can set the suppress_instrumentation key before export to prevent the exporter from generating spans. https://github.com/open-telemetry/opentelemetry-python/blob/77c9867dcb0f3179c1b395729aae6aec2c3133a0/opentelemetry-sdk/src/opentelemetry/sdk/trace/export/__init__.py#L89
Can you confirm? If so, we should figure out the right way to move forward to not break this functionality.

Sorry I didn't know the full context behind the suppress_instrumentation before making the changes. Yes your understanding is correct in that a new key is created for each instrumentation. This was because now with the create_key functionality creates a unique key name, which is now not simply suppress_instrumentation.
What I can do is since in the otel-python PR, I am creating a unique suppress_instrumentation key, I could export this key to each instrumentation that uses it instead of calling create_key each time. (I will also rename this from EXPORT_KEY to _SUPPRESS_INSTRUMENTATION_KEY.)
https://github.com/open-telemetry/opentelemetry-python/pull/1853/files#diff-ccdfba02d42525f2fee85e7369c94b7c4976ec22079248bbddf610cd11435739R41
Let me know if you think this is alright!

The problem with exporting the key from the SDK is that the instrumentations will then depend on the SDK, which I don't believe they do today. This should likely be part of the API instead, this was discussed in the specification here: open-telemetry/opentelemetry-specification#530

I'll add this to the agenda to discuss it at Thursday's SIG meeting.

Hm, yeah, good point, @codeboten. I think this is the PR that added the suppress instrumentation key to the API.

Co-authored-by: Eddy Lin <[email protected]>

codeboten

Nice, looks great!

lzchen · 2021-06-02T16:31:56Z

opentelemetry-instrumentation/src/opentelemetry/instrumentation/utils.py

 from opentelemetry.trace import StatusCode

+_SUPPRESS_INSTRUMENTATION_KEY = create_key("suppress_instrumentation")


Can we leave a comment saying that this is a temporary solution, and we are waiting on a decision to how we are handling supressing instrumentations?

ocelotl

Just please add the FIXME, it makes it easier to find stuff that is to be fixed later.

opentelemetry-instrumentation/src/opentelemetry/instrumentation/utils.py

euniceek marked this pull request as ready for review May 21, 2021 22:26

euniceek requested review from a team, aabmass and lzchen and removed request for a team May 21, 2021 22:26

lzchen reviewed May 24, 2021

View reviewed changes

lzchen approved these changes May 24, 2021

View reviewed changes

codeboten suggested changes May 25, 2021

View reviewed changes

euniceek force-pushed the context-key-contrib branch from 5ec1cb5 to 588cffb Compare May 26, 2021 05:24

lzchen mentioned this pull request May 26, 2021

Implement CreateKey Functionality open-telemetry/opentelemetry-python#1853

Merged

4 tasks

euniceek and others added 12 commits May 30, 2021 23:20

Add context key in contrib repo

eb4e9c6

Fix lint errors

18d99ba

Apply suggestions from code review

5e07eb3

Fix orders of test

391ded9

Co-authored-by: Eddy Lin <[email protected]>

Update changelog

96dd6fe

Fix orders

2b60bed

Update changelog link

6bd41c7

Update changelog to be in added section

dceeec3

Change export key name

84d1586

Refactor suppress instrumentation key

01dacae

Remove create key in span processor

075bdd0

Move suppress key to instrumentation package

f05b9f5

euniceek force-pushed the context-key-contrib branch 3 times, most recently from 2d8d41a to f05b9f5 Compare May 31, 2021 06:52

Fix lint error

b4c0f0e

euniceek force-pushed the context-key-contrib branch from e1b84bb to b4c0f0e Compare May 31, 2021 17:46

euniceek and others added 3 commits May 31, 2021 11:06

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

4128fa7

Update changelog

196684b

Fix import errors

212dcb9

codeboten approved these changes Jun 2, 2021

View reviewed changes

lzchen reviewed Jun 2, 2021

View reviewed changes

lzchen approved these changes Jun 2, 2021

View reviewed changes

euniceek added 2 commits June 2, 2021 20:13

Merge remote-tracking branch 'origin/master' into context-key-contrib

f6cfd54

Add comment on suppress instrumentation key

7f6b893

euniceek force-pushed the context-key-contrib branch from bc24d57 to 7f6b893 Compare June 3, 2021 03:39

ocelotl approved these changes Jun 3, 2021

View reviewed changes

opentelemetry-instrumentation/src/opentelemetry/instrumentation/utils.py Outdated Show resolved Hide resolved

Update SHA

11b5ece

euniceek force-pushed the context-key-contrib branch from 59205e3 to 11b5ece Compare June 6, 2021 07:22

euniceek and others added 5 commits June 6, 2021 00:25

Merge remote-tracking branch 'origin/master' into context-key-contrib

6ea87b6

Add FIXME comment

1604150

Change instrumentation order in tox.ini

bd9108e

Change instrumentation order in tox.ini

73c67be

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

3c8330b

euniceek force-pushed the context-key-contrib branch from 2ae3a5d to 3c8330b Compare June 11, 2021 06:12

euniceek added 2 commits June 10, 2021 23:13

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

82c4f60

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

88191c4

lzchen merged commit 4be32b5 into open-telemetry:main Jun 11, 2021

LikeTheSalad mentioned this pull request Oct 6, 2023

Prevent infinite traces when automatically instrumenting OkHttp open-telemetry/opentelemetry-java#5886

Closed

LikeTheSalad mentioned this pull request Oct 16, 2023

Identifying otel http calls open-telemetry/opentelemetry-java#5918

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add create context key to contrib #502

Add create context key to contrib #502

euniceek commented May 21, 2021

lzchen May 24, 2021

lzchen May 24, 2021 •

edited

Loading

codeboten left a comment

euniceek commented May 25, 2021 •

edited

Loading

ocelotl commented May 26, 2021

codeboten commented May 26, 2021

ocelotl commented May 26, 2021

codeboten left a comment

lzchen Jun 2, 2021

ocelotl left a comment

	EXPORT_KEY = create_key("suppress_instrumentation")
	_SUPPRESS_INSTRUMENTATION_KEY= create_key("suppress_instrumentation")

		from opentelemetry.trace import StatusCode

		_SUPPRESS_INSTRUMENTATION_KEY = create_key("suppress_instrumentation")

Add create context key to contrib #502

Add create context key to contrib #502

Conversation

euniceek commented May 21, 2021

Description

How Has This Been Tested?

lzchen May 24, 2021

Choose a reason for hiding this comment

lzchen May 24, 2021 • edited Loading

Choose a reason for hiding this comment

codeboten left a comment

Choose a reason for hiding this comment

euniceek commented May 25, 2021 • edited Loading

ocelotl commented May 26, 2021

codeboten commented May 26, 2021

ocelotl commented May 26, 2021

codeboten left a comment

Choose a reason for hiding this comment

lzchen Jun 2, 2021

Choose a reason for hiding this comment

ocelotl left a comment

Choose a reason for hiding this comment

lzchen May 24, 2021 •

edited

Loading

euniceek commented May 25, 2021 •

edited

Loading