From a0b4c4f0f92eb065f484f6ec392ed335b2d767e6 Mon Sep 17 00:00:00 2001 From: Hasier Date: Thu, 10 Nov 2022 11:48:50 +0000 Subject: [PATCH 1/5] Fix generating ASGI keys --- .../src/opentelemetry/instrumentation/asgi/__init__.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py b/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py index 608aeade7f..46bad6672c 100644 --- a/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py @@ -260,7 +260,8 @@ def get( return decoded def keys(self, carrier: dict) -> typing.List[str]: - return [_key.decode("utf8") for (_key, _value) in carrier] + headers = carrier.get("headers") or [] + return [_key.decode("utf8") for (_key, _value) in headers] asgi_getter = ASGIGetter() From afcf3c5c558c575887ee4bdeb9bef9f8b7197995 Mon Sep 17 00:00:00 2001 From: Hasier Date: Thu, 10 Nov 2022 12:04:46 +0000 Subject: [PATCH 2/5] Test fix generating ASGI keys --- .../tests/test_asgi_custom_headers.py | 75 +++++++++++++++++++ 1 file changed, 75 insertions(+) diff --git a/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_custom_headers.py b/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_custom_headers.py index 2d50d0704f..f5701f6cc4 100644 --- a/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_custom_headers.py +++ b/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_custom_headers.py @@ -1,6 +1,18 @@ +import typing from unittest import mock import opentelemetry.instrumentation.asgi as otel_asgi +from opentelemetry import trace +from opentelemetry.context import Context +from opentelemetry.propagate import get_global_textmap, set_global_textmap +from opentelemetry.propagators.textmap import ( + CarrierT, + Getter, + Setter, + TextMapPropagator, + default_getter, + default_setter, +) from opentelemetry.test.asgitestutil import AsgiTestBase from opentelemetry.test.test_base import TestBase from opentelemetry.trace import SpanKind @@ -13,6 +25,60 @@ from .test_asgi_middleware import simple_asgi +class MockTextMapPropagator(TextMapPropagator): + """Mock propagator for testing purposes using both getter `get` and `all`.""" + + TRACE_ID_KEY = "mock-traceid" + SPAN_ID_KEY = "mock-spanid" + + def extract( + self, + carrier: CarrierT, + context: typing.Optional[Context] = None, + getter: Getter = default_getter, + ) -> Context: + if context is None: + context = Context() + + trace_id_list = getter.get(carrier, self.TRACE_ID_KEY) + span_id_list = getter.get(carrier, self.SPAN_ID_KEY) + carrier_keys = getter.keys(carrier) + + if not trace_id_list or not span_id_list: + assert not any(key in carrier_keys for key in self.fields) + return context + + assert all(key in carrier_keys for key in self.fields) + return trace.set_span_in_context( + trace.NonRecordingSpan( + trace.SpanContext( + trace_id=int(trace_id_list[0]), + span_id=int(span_id_list[0]), + is_remote=True, + ) + ), + context, + ) + + def inject( + self, + carrier: CarrierT, + context: typing.Optional[Context] = None, + setter: Setter = default_setter, + ) -> None: + span = trace.get_current_span(context) + setter.set( + carrier, self.TRACE_ID_KEY, str(span.get_span_context().trace_id) + ) + setter.set( + carrier, self.SPAN_ID_KEY, str(span.get_span_context().span_id) + ) + + @property + def fields(self): + return {self.TRACE_ID_KEY, self.SPAN_ID_KEY} + + async def http_app_with_custom_headers(scope, receive, send): message = await receive() assert scope["type"] == "http" @@ -34,6 +100,8 @@ async def http_app_with_custom_headers(scope, receive, send): b"my-custom-regex-value-3,my-custom-regex-value-4", ), (b"my-secret-header", b"my-secret-value"), + (MockTextMapPropagator.TRACE_ID_KEY.encode(), b"1"), + (MockTextMapPropagator.SPAN_ID_KEY.encode(), b"2"), ], } ) @@ -60,6 +128,8 @@ async def websocket_app_with_custom_headers(scope, receive, send): b"my-custom-regex-value-3,my-custom-regex-value-4", ), (b"my-secret-header", b"my-secret-value"), + (MockTextMapPropagator.TRACE_ID_KEY.encode(), b"1"), + (MockTextMapPropagator.SPAN_ID_KEY.encode(), b"2"), ], } ) @@ -88,6 +158,11 @@ def setUp(self): self.app = otel_asgi.OpenTelemetryMiddleware( simple_asgi, tracer_provider=self.tracer_provider ) + self.previous_propagator = get_global_textmap() + set_global_textmap(MockTextMapPropagator()) + + def tearDown(self): + set_global_textmap(self.previous_propagator) def test_http_custom_request_headers_in_span_attributes(self): self.scope["headers"].extend( From e4dbc51f2c2d5d26af4dbb04b26e3a3a44277ac4 Mon Sep 17 00:00:00 2001 From: Hasier Date: Thu, 10 Nov 2022 13:12:14 +0000 Subject: [PATCH 3/5] Update CHANGELOG.md --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 45163e7889..bb4de30076 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ([#1424](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1424)) - `opentelemetry-instrumentation-dbapi` Fix the check for the connection already being instrumented in instrument_connection(). ([#1424](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1424)) +- `opentelemetry-instrumentation-asgi` Fix keys() in class ASGIGetter to correctly fetch values from carrier headers. + ([#1435](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1435)) ## Version 1.14.0/0.35b0 (2022-11-03) From 59803954420260ae9970ad3727f44da1341f71ef Mon Sep 17 00:00:00 2001 From: Hasier Date: Wed, 11 Jan 2023 10:33:45 +0000 Subject: [PATCH 4/5] Revert "Test fix generating ASGI keys" This reverts commit afcf3c5c558c575887ee4bdeb9bef9f8b7197995. --- .../tests/test_asgi_custom_headers.py | 75 ------------------- 1 file changed, 75 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_custom_headers.py b/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_custom_headers.py index f5701f6cc4..2d50d0704f 100644 --- a/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_custom_headers.py +++ b/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_custom_headers.py @@ -1,18 +1,6 @@ -import typing from unittest import mock import opentelemetry.instrumentation.asgi as otel_asgi -from opentelemetry import trace -from opentelemetry.context import Context -from opentelemetry.propagate import get_global_textmap, set_global_textmap -from opentelemetry.propagators.textmap import ( - CarrierT, - Getter, - Setter, - TextMapPropagator, - default_getter, - default_setter, -) from opentelemetry.test.asgitestutil import AsgiTestBase from opentelemetry.test.test_base import TestBase from opentelemetry.trace import SpanKind @@ -25,60 +13,6 @@ from .test_asgi_middleware import simple_asgi -class MockTextMapPropagator(TextMapPropagator): - """Mock propagator for testing purposes using both getter `get` and `all`.""" - - TRACE_ID_KEY = "mock-traceid" - SPAN_ID_KEY = "mock-spanid" - - def extract( - self, - carrier: CarrierT, - context: typing.Optional[Context] = None, - getter: Getter = default_getter, - ) -> Context: - if context is None: - context = Context() - - trace_id_list = getter.get(carrier, self.TRACE_ID_KEY) - span_id_list = getter.get(carrier, self.SPAN_ID_KEY) - carrier_keys = getter.keys(carrier) - - if not trace_id_list or not span_id_list: - assert not any(key in carrier_keys for key in self.fields) - return context - - assert all(key in carrier_keys for key in self.fields) - return trace.set_span_in_context( - trace.NonRecordingSpan( - trace.SpanContext( - trace_id=int(trace_id_list[0]), - span_id=int(span_id_list[0]), - is_remote=True, - ) - ), - context, - ) - - def inject( - self, - carrier: CarrierT, - context: typing.Optional[Context] = None, - setter: Setter = default_setter, - ) -> None: - span = trace.get_current_span(context) - setter.set( - carrier, self.TRACE_ID_KEY, str(span.get_span_context().trace_id) - ) - setter.set( - carrier, self.SPAN_ID_KEY, str(span.get_span_context().span_id) - ) - - @property - def fields(self): - return {self.TRACE_ID_KEY, self.SPAN_ID_KEY} - - async def http_app_with_custom_headers(scope, receive, send): message = await receive() assert scope["type"] == "http" @@ -100,8 +34,6 @@ async def http_app_with_custom_headers(scope, receive, send): b"my-custom-regex-value-3,my-custom-regex-value-4", ), (b"my-secret-header", b"my-secret-value"), - (MockTextMapPropagator.TRACE_ID_KEY.encode(), b"1"), - (MockTextMapPropagator.SPAN_ID_KEY.encode(), b"2"), ], } ) @@ -128,8 +60,6 @@ async def websocket_app_with_custom_headers(scope, receive, send): b"my-custom-regex-value-3,my-custom-regex-value-4", ), (b"my-secret-header", b"my-secret-value"), - (MockTextMapPropagator.TRACE_ID_KEY.encode(), b"1"), - (MockTextMapPropagator.SPAN_ID_KEY.encode(), b"2"), ], } ) @@ -158,11 +88,6 @@ def setUp(self): self.app = otel_asgi.OpenTelemetryMiddleware( simple_asgi, tracer_provider=self.tracer_provider ) - self.previous_propagator = get_global_textmap() - set_global_textmap(MockTextMapPropagator()) - - def tearDown(self): - set_global_textmap(self.previous_propagator) def test_http_custom_request_headers_in_span_attributes(self): self.scope["headers"].extend( From c219bb0fd9884f7a7720a1192f480cef42379812 Mon Sep 17 00:00:00 2001 From: Hasier Date: Wed, 11 Jan 2023 10:32:48 +0000 Subject: [PATCH 5/5] Add getter keys() tests --- .../tests/test_getter.py | 25 +++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-asgi/tests/test_getter.py b/instrumentation/opentelemetry-instrumentation-asgi/tests/test_getter.py index 454162d715..26bb652b50 100644 --- a/instrumentation/opentelemetry-instrumentation-asgi/tests/test_getter.py +++ b/instrumentation/opentelemetry-instrumentation-asgi/tests/test_getter.py @@ -18,12 +18,18 @@ class TestASGIGetter(TestCase): - def test_get_none(self): + def test_get_none_empty_carrier(self): getter = ASGIGetter() carrier = {} val = getter.get(carrier, "test") self.assertIsNone(val) + def test_get_none_empty_headers(self): + getter = ASGIGetter() + carrier = {"headers": []} + val = getter.get(carrier, "test") + self.assertIsNone(val) + def test_get_(self): getter = ASGIGetter() carrier = {"headers": [(b"test-key", b"val")]} @@ -44,7 +50,22 @@ def test_get_(self): "Should be case insensitive", ) - def test_keys(self): + def test_keys_empty_carrier(self): getter = ASGIGetter() keys = getter.keys({}) self.assertEqual(keys, []) + + def test_keys_empty_headers(self): + getter = ASGIGetter() + keys = getter.keys({"headers": []}) + self.assertEqual(keys, []) + + def test_keys(self): + getter = ASGIGetter() + carrier = {"headers": [(b"test-key", b"val")]} + expected_val = ["test-key"] + self.assertEqual( + getter.keys(carrier), + expected_val, + "Should be equal", + )