Fix HTTP instrumentation not being suppressed #1116
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
7 tasks
lzchen
reviewed
Jun 13, 2022
| @@ -188,6 +189,18 @@ def test_suppress_instrumentation(self): | |||
|
|
|||
| self.assert_span(num_spans=0) | |||
|
|
|||
| def test_suppress_http_instrumentation(self): | |||
There was a problem hiding this comment.
Is there a test that tests the scenario of being instrumented with multiple http instrumentations?
There was a problem hiding this comment.
Testing a scenario with multiple http instrumentations would require testing across multiple packages at the same time (for example, requests and urllib3 to replicate the problem raised in issue #930) in the form of integration tests and not unit tests, which I think would be out of scope in this PR.
lzchen
approved these changes
Jun 14, 2022
ocelotl
reviewed
Jun 17, 2022
| @@ -87,6 +87,7 @@ def response_hook(span, service_name, operation_name, result): | |||
| from wrapt import wrap_function_wrapper | |||
|
|
|||
| from opentelemetry import context as context_api | |||
| from opentelemetry.context import _SUPPRESS_HTTP_INSTRUMENTATION_KEY | |||
There was a problem hiding this comment.
This needs a FIXME to remind us to fix the importing of a private attribute when the location of the _SUPPRESS_HTTP_INSTRUMENTATION_KEY is defined.
There was a problem hiding this comment.
Added, thanks for the feedback!
sk-
added a commit
to sk-/opentelemetry-python-contrib
that referenced
this pull request
Sep 10, 2022
This PR adds the target information for metrics reported by instrumentation/asgi. Unfortunately, there's no ASGI standard to reliably get this information, and I was only able to get it for FastAPI. I also tried to get the info with Sanic and Starlette (encode/starlette#685), but there's nothing in the scope allowing to recreate the route. Besides the included unit tests, the logic was tested using the following app: ```python import io import fastapi app = fastapi.FastAPI() def dump_scope(scope): b = io.StringIO() print(scope, file=b) return b.getvalue() @app.get("/test/{id}") def test(id: str, req: fastapi.Request): print(req.scope) return {"target": _collect_target_attribute(req.scope), "scope": dump_scope(req.scope)} sub_app = fastapi.FastAPI() @sub_app.get("/test/{id}") def sub_test(id: str, req: fastapi.Request): print(req.scope) return {"target": _collect_target_attribute(req.scope), "scope": dump_scope(req.scope)} app.mount("/sub", sub_app) ``` Partially fixes open-telemetry#1116 Note to reviewers: I tried to touch as less as possible, so that we don;t require a refactor before this change. However, we could consider changing `collect_request_attributes` so that it returns both a trace attributes and a metrics attributes. Wihout that change we cannot add the `HTTP_TARGET` attribute to the list of metric atttributes, because it will be present but with high cardinality.
7 tasks
sk-
added a commit
to sk-/opentelemetry-python-contrib
that referenced
this pull request
Sep 10, 2022
This PR adds the target information for metrics reported by instrumentation/asgi. Unfortunately, there's no ASGI standard to reliably get this information, and I was only able to get it for FastAPI. I also tried to get the info with Sanic and Starlette (encode/starlette#685), but there's nothing in the scope allowing to recreate the route. Besides the included unit tests, the logic was tested using the following app: ```python import io import fastapi app = fastapi.FastAPI() def dump_scope(scope): b = io.StringIO() print(scope, file=b) return b.getvalue() @app.get("/test/{id}") def test(id: str, req: fastapi.Request): print(req.scope) return {"target": _collect_target_attribute(req.scope), "scope": dump_scope(req.scope)} sub_app = fastapi.FastAPI() @sub_app.get("/test/{id}") def sub_test(id: str, req: fastapi.Request): print(req.scope) return {"target": _collect_target_attribute(req.scope), "scope": dump_scope(req.scope)} app.mount("/sub", sub_app) ``` Partially fixes open-telemetry#1116 Note to reviewers: I tried to touch as less as possible, so that we don;t require a refactor before this change. However, we could consider changing `collect_request_attributes` so that it returns both a trace attributes and a metrics attributes. Wihout that change we cannot add the `HTTP_TARGET` attribute to the list of metric atttributes, because it will be present but with high cardinality.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
This change aims to fix the issue outlined in #930 where urllib3 downstream spans are not being suppressed with a requests upstream, causing an extra span to be created.
The root cause of this bug lies in the create_key() method in opentelemetry context which adds a unique
uuidto the end of each key created. Since a new_SUPPRESS_HTTP_INSTRUMENTATIONkey is being created in the instrumentation for each library, this results in a different key being created by each instrumentation instead of the creation of a single key that suppresses HTTP instrumentation so proceeding libraries know not to instrument further.This fix involves creating the
_SUPPRESS_HTTP_INSTRUMENTATIONonce in opentelemetry context to avoid the bug that causes a new suppress http instrumentation key to be created by each instrumentation. By having only 1 key that is accessible to all instrumentations, its true value can be passed on from one instrumentation to the next without being set to None every time the instrumentation of a new library begins. This does not conflict with the context spec since the_SUPPRESS_HTTP_INSTRUMENTATION_KEYis meant to be shared, and creating a unique version of it in each library instrumentation is not the correct implementation of this concept.Fixes #930
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.