From eed93aa453b0df3989355189d6ec3ce88835064d Mon Sep 17 00:00:00 2001 From: Martin Morset Date: Mon, 13 Nov 2023 14:31:05 +0100 Subject: [PATCH 1/7] feat: Respect "suppress_instrumentation" in redis instrumentor --- .../instrumentation/redis/__init__.py | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/instrumentation/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/__init__.py b/instrumentation/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/__init__.py index 1d61e8cfd3..9618ab29d1 100644 --- a/instrumentation/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/__init__.py @@ -96,7 +96,8 @@ def response_hook(span, instance, response): import redis from wrapt import wrap_function_wrapper -from opentelemetry import trace +from opentelemetry import context, trace +from opentelemetry.context import _SUPPRESS_INSTRUMENTATION_KEY from opentelemetry.instrumentation.instrumentor import BaseInstrumentor from opentelemetry.instrumentation.redis.package import _instruments from opentelemetry.instrumentation.redis.util import ( @@ -182,6 +183,11 @@ def _traced_execute_command(func, instance, args, kwargs): query = _format_command_args(args) name = _build_span_name(instance, args) + if context.get_value( + _SUPPRESS_INSTRUMENTATION_KEY + ): + return func(*args, **kwargs) + with tracer.start_as_current_span( name, kind=trace.SpanKind.CLIENT ) as span: @@ -197,6 +203,12 @@ def _traced_execute_command(func, instance, args, kwargs): return response def _traced_execute_pipeline(func, instance, args, kwargs): + + if context.get_value( + _SUPPRESS_INSTRUMENTATION_KEY + ): + return func(*args, **kwargs) + ( command_stack, resource, From f60a669b7654606536f259ebdeb8c3234d0ff061 Mon Sep 17 00:00:00 2001 From: Martin Morset Date: Wed, 15 Nov 2023 13:48:57 +0100 Subject: [PATCH 2/7] test: Suppressing instrumentation works as expected --- .../tests/test_redis.py | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/instrumentation/opentelemetry-instrumentation-redis/tests/test_redis.py b/instrumentation/opentelemetry-instrumentation-redis/tests/test_redis.py index 11e56ad953..5cf045b1e2 100644 --- a/instrumentation/opentelemetry-instrumentation-redis/tests/test_redis.py +++ b/instrumentation/opentelemetry-instrumentation-redis/tests/test_redis.py @@ -17,8 +17,9 @@ import redis import redis.asyncio -from opentelemetry import trace +from opentelemetry import context, trace from opentelemetry.instrumentation.redis import RedisInstrumentor +from opentelemetry.instrumentation.utils import _SUPPRESS_INSTRUMENTATION_KEY from opentelemetry.test.test_base import TestBase from opentelemetry.trace import SpanKind @@ -78,6 +79,17 @@ def test_not_recording(self): self.assertFalse(mock_span.set_attribute.called) self.assertFalse(mock_span.set_status.called) + def test_suppress_instrumentation_no_span(self): + redis_client = redis.Redis() + + token = context.attach(context.set_value(_SUPPRESS_INSTRUMENTATION_KEY, True)) + with mock.patch.object(redis_client, "connection"): + redis_client.ping() + context.detach(token) + spans = self.memory_exporter.get_finished_spans() + + self.assertEqual(len(spans), 0) + def test_instrument_uninstrument(self): redis_client = redis.Redis() From 1ff4c0be005ad95bdba3375eb8ca1c02295b22c1 Mon Sep 17 00:00:00 2001 From: Martin Morset Date: Thu, 29 Feb 2024 13:23:55 +0100 Subject: [PATCH 3/7] address PR feedback --- .../instrumentation/redis/__init__.py | 25 ++++++++------- .../tests/test_redis.py | 31 ++++++++++++++++--- 2 files changed, 41 insertions(+), 15 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/__init__.py b/instrumentation/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/__init__.py index 9618ab29d1..a313949953 100644 --- a/instrumentation/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/__init__.py @@ -96,8 +96,7 @@ def response_hook(span, instance, response): import redis from wrapt import wrap_function_wrapper -from opentelemetry import context, trace -from opentelemetry.context import _SUPPRESS_INSTRUMENTATION_KEY +from opentelemetry import trace from opentelemetry.instrumentation.instrumentor import BaseInstrumentor from opentelemetry.instrumentation.redis.package import _instruments from opentelemetry.instrumentation.redis.util import ( @@ -105,7 +104,7 @@ def response_hook(span, instance, response): _format_command_args, ) from opentelemetry.instrumentation.redis.version import __version__ -from opentelemetry.instrumentation.utils import unwrap +from opentelemetry.instrumentation.utils import unwrap, is_instrumentation_enabled from opentelemetry.semconv.trace import SpanAttributes from opentelemetry.trace import Span @@ -180,14 +179,12 @@ def _instrument( response_hook: _ResponseHookT = None, ): def _traced_execute_command(func, instance, args, kwargs): - query = _format_command_args(args) - name = _build_span_name(instance, args) - if context.get_value( - _SUPPRESS_INSTRUMENTATION_KEY - ): + if not is_instrumentation_enabled(): return func(*args, **kwargs) + query = _format_command_args(args) + name = _build_span_name(instance, args) with tracer.start_as_current_span( name, kind=trace.SpanKind.CLIENT ) as span: @@ -204,9 +201,7 @@ def _traced_execute_command(func, instance, args, kwargs): def _traced_execute_pipeline(func, instance, args, kwargs): - if context.get_value( - _SUPPRESS_INSTRUMENTATION_KEY - ): + if not is_instrumentation_enabled(): return func(*args, **kwargs) ( @@ -260,6 +255,10 @@ def _traced_execute_pipeline(func, instance, args, kwargs): ) async def _async_traced_execute_command(func, instance, args, kwargs): + + if not is_instrumentation_enabled(): + return func(*args, **kwargs) + query = _format_command_args(args) name = _build_span_name(instance, args) @@ -278,6 +277,10 @@ async def _async_traced_execute_command(func, instance, args, kwargs): return response async def _async_traced_execute_pipeline(func, instance, args, kwargs): + + if not is_instrumentation_enabled(): + return func(*args, **kwargs) + ( command_stack, resource, diff --git a/instrumentation/opentelemetry-instrumentation-redis/tests/test_redis.py b/instrumentation/opentelemetry-instrumentation-redis/tests/test_redis.py index a41d1ea0f8..e8633d30ca 100644 --- a/instrumentation/opentelemetry-instrumentation-redis/tests/test_redis.py +++ b/instrumentation/opentelemetry-instrumentation-redis/tests/test_redis.py @@ -20,7 +20,7 @@ from opentelemetry import context, trace from opentelemetry.instrumentation.redis import RedisInstrumentor -from opentelemetry.instrumentation.utils import _SUPPRESS_INSTRUMENTATION_KEY +from opentelemetry.instrumentation.utils import suppress_instrumentation from opentelemetry.test.test_base import TestBase from opentelemetry.trace import SpanKind @@ -65,12 +65,35 @@ def test_not_recording(self): def test_suppress_instrumentation_no_span(self): redis_client = redis.Redis() - token = context.attach(context.set_value(_SUPPRESS_INSTRUMENTATION_KEY, True)) with mock.patch.object(redis_client, "connection"): - redis_client.ping() - context.detach(token) + redis_client.get("key") + spans = self.memory_exporter.get_finished_spans() + + self.assertEqual(len(spans), 1) + self.memory_exporter.clear() + + with suppress_instrumentation(): + with mock.patch.object(redis_client, "connection"): + redis_client.ping() + spans = self.memory_exporter.get_finished_spans() + + self.assertEqual(len(spans), 0) + + def test_suppress_async_instrumentation_no_span(self): + redis_client = redis.Redis() + + with mock.patch.object(redis_client, "connection", AsyncMock()): + redis_client.get("key") spans = self.memory_exporter.get_finished_spans() + self.assertEqual(len(spans), 1) + self.memory_exporter.clear() + + with suppress_instrumentation(): + with mock.patch.object(redis_client, "connection", AsyncMock()): + redis_client.ping() + spans = self.memory_exporter.get_finished_spans() + self.assertEqual(len(spans), 0) def test_instrument_uninstrument(self): From 20b551e37b258d11bb3c0451b4ea5afe664fa084 Mon Sep 17 00:00:00 2001 From: Martin Morset Date: Thu, 29 Feb 2024 13:25:53 +0100 Subject: [PATCH 4/7] linter --- .../opentelemetry/instrumentation/redis/__init__.py | 13 ++++++++----- .../tests/test_redis.py | 4 ++-- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/__init__.py b/instrumentation/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/__init__.py index a313949953..be437af45c 100644 --- a/instrumentation/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/__init__.py @@ -104,7 +104,10 @@ def response_hook(span, instance, response): _format_command_args, ) from opentelemetry.instrumentation.redis.version import __version__ -from opentelemetry.instrumentation.utils import unwrap, is_instrumentation_enabled +from opentelemetry.instrumentation.utils import ( + is_instrumentation_enabled, + unwrap, +) from opentelemetry.semconv.trace import SpanAttributes from opentelemetry.trace import Span @@ -255,10 +258,10 @@ def _traced_execute_pipeline(func, instance, args, kwargs): ) async def _async_traced_execute_command(func, instance, args, kwargs): - + if not is_instrumentation_enabled(): return func(*args, **kwargs) - + query = _format_command_args(args) name = _build_span_name(instance, args) @@ -277,10 +280,10 @@ async def _async_traced_execute_command(func, instance, args, kwargs): return response async def _async_traced_execute_pipeline(func, instance, args, kwargs): - + if not is_instrumentation_enabled(): return func(*args, **kwargs) - + ( command_stack, resource, diff --git a/instrumentation/opentelemetry-instrumentation-redis/tests/test_redis.py b/instrumentation/opentelemetry-instrumentation-redis/tests/test_redis.py index e8633d30ca..ed7d697aac 100644 --- a/instrumentation/opentelemetry-instrumentation-redis/tests/test_redis.py +++ b/instrumentation/opentelemetry-instrumentation-redis/tests/test_redis.py @@ -76,7 +76,7 @@ def test_suppress_instrumentation_no_span(self): with mock.patch.object(redis_client, "connection"): redis_client.ping() spans = self.memory_exporter.get_finished_spans() - + self.assertEqual(len(spans), 0) def test_suppress_async_instrumentation_no_span(self): @@ -93,7 +93,7 @@ def test_suppress_async_instrumentation_no_span(self): with mock.patch.object(redis_client, "connection", AsyncMock()): redis_client.ping() spans = self.memory_exporter.get_finished_spans() - + self.assertEqual(len(spans), 0) def test_instrument_uninstrument(self): From 1744eba3c513ed5542411150bd8819015f3b3901 Mon Sep 17 00:00:00 2001 From: Martin Morset Date: Thu, 29 Feb 2024 13:46:11 +0100 Subject: [PATCH 5/7] remove unused import --- .../opentelemetry-instrumentation-redis/tests/test_redis.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/instrumentation/opentelemetry-instrumentation-redis/tests/test_redis.py b/instrumentation/opentelemetry-instrumentation-redis/tests/test_redis.py index ed7d697aac..c6fdd7e069 100644 --- a/instrumentation/opentelemetry-instrumentation-redis/tests/test_redis.py +++ b/instrumentation/opentelemetry-instrumentation-redis/tests/test_redis.py @@ -18,7 +18,7 @@ import redis import redis.asyncio -from opentelemetry import context, trace +from opentelemetry import trace from opentelemetry.instrumentation.redis import RedisInstrumentor from opentelemetry.instrumentation.utils import suppress_instrumentation from opentelemetry.test.test_base import TestBase From 8d8eb6f842fe2e0a2ff472fdc8f93f43f555c1a5 Mon Sep 17 00:00:00 2001 From: Martin Morset Date: Thu, 29 Feb 2024 13:53:33 +0100 Subject: [PATCH 6/7] fix some asyncio mess --- .../tests/test_redis.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-redis/tests/test_redis.py b/instrumentation/opentelemetry-instrumentation-redis/tests/test_redis.py index c6fdd7e069..68d46f9c43 100644 --- a/instrumentation/opentelemetry-instrumentation-redis/tests/test_redis.py +++ b/instrumentation/opentelemetry-instrumentation-redis/tests/test_redis.py @@ -74,16 +74,16 @@ def test_suppress_instrumentation_no_span(self): with suppress_instrumentation(): with mock.patch.object(redis_client, "connection"): - redis_client.ping() + redis_client.get("key") spans = self.memory_exporter.get_finished_spans() self.assertEqual(len(spans), 0) def test_suppress_async_instrumentation_no_span(self): - redis_client = redis.Redis() + redis_client = redis.asyncio.Redis() with mock.patch.object(redis_client, "connection", AsyncMock()): - redis_client.get("key") + asyncio.run(redis_client.get("key")) spans = self.memory_exporter.get_finished_spans() self.assertEqual(len(spans), 1) @@ -91,7 +91,7 @@ def test_suppress_async_instrumentation_no_span(self): with suppress_instrumentation(): with mock.patch.object(redis_client, "connection", AsyncMock()): - redis_client.ping() + asyncio.run(redis_client.get("key")) spans = self.memory_exporter.get_finished_spans() self.assertEqual(len(spans), 0) From 201470f4df16ec8f4d9f1ac297de46115aa9b711 Mon Sep 17 00:00:00 2001 From: Martin Morset Date: Thu, 29 Feb 2024 13:54:34 +0100 Subject: [PATCH 7/7] some missing await --- .../src/opentelemetry/instrumentation/redis/__init__.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/__init__.py b/instrumentation/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/__init__.py index be437af45c..14b2ce7d2f 100644 --- a/instrumentation/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/__init__.py @@ -260,7 +260,7 @@ def _traced_execute_pipeline(func, instance, args, kwargs): async def _async_traced_execute_command(func, instance, args, kwargs): if not is_instrumentation_enabled(): - return func(*args, **kwargs) + return await func(*args, **kwargs) query = _format_command_args(args) name = _build_span_name(instance, args) @@ -282,7 +282,7 @@ async def _async_traced_execute_command(func, instance, args, kwargs): async def _async_traced_execute_pipeline(func, instance, args, kwargs): if not is_instrumentation_enabled(): - return func(*args, **kwargs) + return await func(*args, **kwargs) ( command_stack,