From 776f9d464361ae06ccf663e9b91ae7269882c800 Mon Sep 17 00:00:00 2001 From: Shalev Roda <65566801+shalevr@users.noreply.github.com> Date: Tue, 13 Jun 2023 11:07:45 +0300 Subject: [PATCH 01/29] Fix celery docker tests (#1841) --- .../tests/celery/test_celery_functional.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/opentelemetry-docker-tests/tests/celery/test_celery_functional.py b/tests/opentelemetry-docker-tests/tests/celery/test_celery_functional.py index 1a86154ffc..f284272c5d 100644 --- a/tests/opentelemetry-docker-tests/tests/celery/test_celery_functional.py +++ b/tests/opentelemetry-docker-tests/tests/celery/test_celery_functional.py @@ -303,8 +303,8 @@ def fn_exception(): span = spans[0] - assert span.status.is_ok is True - assert span.status.status_code == StatusCode.UNSET + assert span.status.is_ok is False + assert span.status.status_code == StatusCode.ERROR assert span.name == "run/test_celery_functional.fn_exception" assert span.attributes.get("celery.action") == "run" assert span.attributes.get("celery.state") == "FAILURE" @@ -443,8 +443,8 @@ def run(self): span = spans[0] - assert span.status.is_ok is True - assert span.status.status_code == StatusCode.UNSET + assert span.status.is_ok is False + assert span.status.status_code == StatusCode.ERROR assert span.name == "run/test_celery_functional.BaseTask" assert span.attributes.get("celery.action") == "run" assert span.attributes.get("celery.state") == "FAILURE" From 26c673e7c9364a00b94f2868360162f8849e4640 Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Tue, 13 Jun 2023 10:40:41 +0200 Subject: [PATCH 02/29] Use HTTP mock server for aiohttp tests (#1849) Fixes #1842 --- .../pyproject.toml | 1 + .../tests/test_aiohttp_client_integration.py | 33 ++++++++++++------- 2 files changed, 23 insertions(+), 11 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-aiohttp-client/pyproject.toml b/instrumentation/opentelemetry-instrumentation-aiohttp-client/pyproject.toml index acdc1a2a4d..ea23325caa 100644 --- a/instrumentation/opentelemetry-instrumentation-aiohttp-client/pyproject.toml +++ b/instrumentation/opentelemetry-instrumentation-aiohttp-client/pyproject.toml @@ -38,6 +38,7 @@ instruments = [ ] test = [ "opentelemetry-instrumentation-aiohttp-client[instruments]", + "http-server-mock" ] [project.entry-points.opentelemetry_instrumentor] diff --git a/instrumentation/opentelemetry-instrumentation-aiohttp-client/tests/test_aiohttp_client_integration.py b/instrumentation/opentelemetry-instrumentation-aiohttp-client/tests/test_aiohttp_client_integration.py index d9f76f0239..7c3d7b634d 100644 --- a/instrumentation/opentelemetry-instrumentation-aiohttp-client/tests/test_aiohttp_client_integration.py +++ b/instrumentation/opentelemetry-instrumentation-aiohttp-client/tests/test_aiohttp_client_integration.py @@ -23,6 +23,7 @@ import aiohttp import aiohttp.test_utils import yarl +from http_server_mock import HttpServerMock from pkg_resources import iter_entry_points from opentelemetry import context @@ -313,18 +314,26 @@ async def request_handler(request): def test_credential_removal(self): trace_configs = [aiohttp_client.create_trace_config()] - url = "http://username:password@httpbin.org/status/200" - with self.subTest(url=url): + app = HttpServerMock("test_credential_removal") - async def do_request(url): - async with aiohttp.ClientSession( - trace_configs=trace_configs, - ) as session: - async with session.get(url): - pass + @app.route("/status/200") + def index(): + return "hello" - loop = asyncio.get_event_loop() - loop.run_until_complete(do_request(url)) + url = "http://username:password@localhost:5000/status/200" + + with app.run("localhost", 5000): + with self.subTest(url=url): + + async def do_request(url): + async with aiohttp.ClientSession( + trace_configs=trace_configs, + ) as session: + async with session.get(url): + pass + + loop = asyncio.get_event_loop() + loop.run_until_complete(do_request(url)) self.assert_spans( [ @@ -333,7 +342,9 @@ async def do_request(url): (StatusCode.UNSET, None), { SpanAttributes.HTTP_METHOD: "GET", - SpanAttributes.HTTP_URL: "http://httpbin.org/status/200", + SpanAttributes.HTTP_URL: ( + "http://localhost:5000/status/200" + ), SpanAttributes.HTTP_STATUS_CODE: int(HTTPStatus.OK), }, ) From bcf770d079dc7b76aa5260ebf30f1620ffe51408 Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Tue, 13 Jun 2023 12:01:02 +0200 Subject: [PATCH 03/29] Use HTTP mock server for tornado tests (#1855) * Use HTTP mock server for tornado tests Fixes #1681 * Fix lint --- .../tests/pyramid_base_test.py | 6 +- .../pyproject.toml | 1 + .../tests/test_instrumentation.py | 56 ++++++++++--------- 3 files changed, 36 insertions(+), 27 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-pyramid/tests/pyramid_base_test.py b/instrumentation/opentelemetry-instrumentation-pyramid/tests/pyramid_base_test.py index f5dd9fd7d7..c6b9faa196 100644 --- a/instrumentation/opentelemetry-instrumentation-pyramid/tests/pyramid_base_test.py +++ b/instrumentation/opentelemetry-instrumentation-pyramid/tests/pyramid_base_test.py @@ -15,7 +15,11 @@ import pyramid.httpexceptions as exc from pyramid.response import Response from werkzeug.test import Client -from werkzeug.wrappers import BaseResponse + +# opentelemetry-instrumentation-pyramid uses werkzeug==0.16.1 which has +# werkzeug.wrappers.BaseResponse. This is not the case for newer versions of +# werkzeug like the one lint uses. +from werkzeug.wrappers import BaseResponse # pylint: disable=no-name-in-module class InstrumentationTest: diff --git a/instrumentation/opentelemetry-instrumentation-tornado/pyproject.toml b/instrumentation/opentelemetry-instrumentation-tornado/pyproject.toml index a16554af74..c0553eb6c0 100644 --- a/instrumentation/opentelemetry-instrumentation-tornado/pyproject.toml +++ b/instrumentation/opentelemetry-instrumentation-tornado/pyproject.toml @@ -37,6 +37,7 @@ instruments = [ test = [ "opentelemetry-instrumentation-tornado[instruments]", "opentelemetry-test-utils == 0.40b0.dev", + "http-server-mock" ] [project.entry-points.opentelemetry_instrumentor] diff --git a/instrumentation/opentelemetry-instrumentation-tornado/tests/test_instrumentation.py b/instrumentation/opentelemetry-instrumentation-tornado/tests/test_instrumentation.py index 9fb3608572..c875a331ef 100644 --- a/instrumentation/opentelemetry-instrumentation-tornado/tests/test_instrumentation.py +++ b/instrumentation/opentelemetry-instrumentation-tornado/tests/test_instrumentation.py @@ -15,6 +15,7 @@ from unittest.mock import Mock, patch +from http_server_mock import HttpServerMock from tornado.testing import AsyncHTTPTestCase from opentelemetry import trace @@ -494,32 +495,35 @@ def test_response_headers(self): self.memory_exporter.clear() set_global_response_propagator(orig) - # todo(srikanthccv): fix this test - # this test is making request to real httpbin.org/status/200 which - # is not a good idea as it can fail due to availability of the - # service. - # def test_credential_removal(self): - # response = self.fetch( - # "http://username:password@httpbin.org/status/200" - # ) - # self.assertEqual(response.code, 200) - - # spans = self.sorted_spans(self.memory_exporter.get_finished_spans()) - # self.assertEqual(len(spans), 1) - # client = spans[0] - - # self.assertEqual(client.name, "GET") - # self.assertEqual(client.kind, SpanKind.CLIENT) - # self.assertSpanHasAttributes( - # client, - # { - # SpanAttributes.HTTP_URL: "http://httpbin.org/status/200", - # SpanAttributes.HTTP_METHOD: "GET", - # SpanAttributes.HTTP_STATUS_CODE: 200, - # }, - # ) - - # self.memory_exporter.clear() + def test_credential_removal(self): + app = HttpServerMock("test_credential_removal") + + @app.route("/status/200") + def index(): + return "hello" + + with app.run("localhost", 5000): + response = self.fetch( + "http://username:password@localhost:5000/status/200" + ) + self.assertEqual(response.code, 200) + + spans = self.sorted_spans(self.memory_exporter.get_finished_spans()) + self.assertEqual(len(spans), 1) + client = spans[0] + + self.assertEqual(client.name, "GET") + self.assertEqual(client.kind, SpanKind.CLIENT) + self.assertSpanHasAttributes( + client, + { + SpanAttributes.HTTP_URL: "http://localhost:5000/status/200", + SpanAttributes.HTTP_METHOD: "GET", + SpanAttributes.HTTP_STATUS_CODE: 200, + }, + ) + + self.memory_exporter.clear() class TestTornadoInstrumentationWithXHeaders(TornadoTest): From fc547877d3db5f9039a842eeb05ad6246e396b67 Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Tue, 13 Jun 2023 12:30:52 +0200 Subject: [PATCH 04/29] Remove use of httpbin (#1854) --- .../tests/test_asgi_middleware.py | 4 +-- .../README.rst | 6 ++-- .../instrumentation/httpx/__init__.py | 6 ++-- .../tests/test_httpx_integration.py | 4 +-- .../tests/test_requests_integration.py | 8 ++--- .../tests/test_metrics_instrumentation.py | 4 +-- .../tests/test_urllib_integration.py | 18 +++++------ .../tests/test_urllib3_integration.py | 28 ++++++++--------- .../tests/test_urllib3_metrics.py | 30 +++++++++---------- .../tests/test_wsgi_middleware.py | 4 +-- 10 files changed, 56 insertions(+), 56 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py b/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py index bfa5720f99..7cbae79c6d 100644 --- a/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py +++ b/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py @@ -705,11 +705,11 @@ def test_response_attributes_invalid_status_code(self): self.assertEqual(self.span.set_status.call_count, 1) def test_credential_removal(self): - self.scope["server"] = ("username:password@httpbin.org", 80) + self.scope["server"] = ("username:password@mock", 80) self.scope["path"] = "/status/200" attrs = otel_asgi.collect_request_attributes(self.scope) self.assertEqual( - attrs[SpanAttributes.HTTP_URL], "http://httpbin.org/status/200" + attrs[SpanAttributes.HTTP_URL], "http://mock/status/200" ) def test_collect_target_attribute_missing(self): diff --git a/instrumentation/opentelemetry-instrumentation-httpx/README.rst b/instrumentation/opentelemetry-instrumentation-httpx/README.rst index ffa86cb4bc..1e03eb128e 100644 --- a/instrumentation/opentelemetry-instrumentation-httpx/README.rst +++ b/instrumentation/opentelemetry-instrumentation-httpx/README.rst @@ -30,7 +30,7 @@ When using the instrumentor, all clients will automatically trace requests. import httpx from opentelemetry.instrumentation.httpx import HTTPXClientInstrumentor - url = "https://httpbin.org/get" + url = "https://some.url/get" HTTPXClientInstrumentor().instrument() with httpx.Client() as client: @@ -51,7 +51,7 @@ use the `instrument_client` method. import httpx from opentelemetry.instrumentation.httpx import HTTPXClientInstrumentor - url = "https://httpbin.org/get" + url = "https://some.url/get" with httpx.Client(transport=telemetry_transport) as client: HTTPXClientInstrumentor.instrument_client(client) @@ -96,7 +96,7 @@ If you don't want to use the instrumentor class, you can use the transport class SyncOpenTelemetryTransport, ) - url = "https://httpbin.org/get" + url = "https://some.url/get" transport = httpx.HTTPTransport() telemetry_transport = SyncOpenTelemetryTransport(transport) diff --git a/instrumentation/opentelemetry-instrumentation-httpx/src/opentelemetry/instrumentation/httpx/__init__.py b/instrumentation/opentelemetry-instrumentation-httpx/src/opentelemetry/instrumentation/httpx/__init__.py index b603cbcdd6..736e6c3d32 100644 --- a/instrumentation/opentelemetry-instrumentation-httpx/src/opentelemetry/instrumentation/httpx/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-httpx/src/opentelemetry/instrumentation/httpx/__init__.py @@ -25,7 +25,7 @@ import httpx from opentelemetry.instrumentation.httpx import HTTPXClientInstrumentor - url = "https://httpbin.org/get" + url = "https://some.url/get" HTTPXClientInstrumentor().instrument() with httpx.Client() as client: @@ -46,7 +46,7 @@ import httpx from opentelemetry.instrumentation.httpx import HTTPXClientInstrumentor - url = "https://httpbin.org/get" + url = "https://some.url/get" with httpx.Client(transport=telemetry_transport) as client: HTTPXClientInstrumentor.instrument_client(client) @@ -91,7 +91,7 @@ SyncOpenTelemetryTransport, ) - url = "https://httpbin.org/get" + url = "https://some.url/get" transport = httpx.HTTPTransport() telemetry_transport = SyncOpenTelemetryTransport(transport) diff --git a/instrumentation/opentelemetry-instrumentation-httpx/tests/test_httpx_integration.py b/instrumentation/opentelemetry-instrumentation-httpx/tests/test_httpx_integration.py index 3cac4c45a7..c0d3705dca 100644 --- a/instrumentation/opentelemetry-instrumentation-httpx/tests/test_httpx_integration.py +++ b/instrumentation/opentelemetry-instrumentation-httpx/tests/test_httpx_integration.py @@ -97,7 +97,7 @@ class BaseTestCases: class BaseTest(TestBase, metaclass=abc.ABCMeta): # pylint: disable=no-member - URL = "http://httpbin.org/status/200" + URL = "http://mock/status/200" response_hook = staticmethod(_response_hook) request_hook = staticmethod(_request_hook) no_update_request_hook = staticmethod(_no_update_request_hook) @@ -165,7 +165,7 @@ def test_basic_multiple(self): self.assert_span(num_spans=2) def test_not_foundbasic(self): - url_404 = "http://httpbin.org/status/404" + url_404 = "http://mock/status/404" with respx.mock: respx.get(url_404).mock(httpx.Response(404)) diff --git a/instrumentation/opentelemetry-instrumentation-requests/tests/test_requests_integration.py b/instrumentation/opentelemetry-instrumentation-requests/tests/test_requests_integration.py index 8d6ee7c04d..6e6a68cb8f 100644 --- a/instrumentation/opentelemetry-instrumentation-requests/tests/test_requests_integration.py +++ b/instrumentation/opentelemetry-instrumentation-requests/tests/test_requests_integration.py @@ -63,7 +63,7 @@ class RequestsIntegrationTestBase(abc.ABC): # pylint: disable=no-member # pylint: disable=too-many-public-methods - URL = "http://httpbin.org/status/200" + URL = "http://mock/status/200" # pylint: disable=invalid-name def setUp(self): @@ -152,7 +152,7 @@ def response_hook(span, request_obj, response): self.assertEqual(span.attributes["response_hook_attr"], "value") def test_excluded_urls_explicit(self): - url_404 = "http://httpbin.org/status/404" + url_404 = "http://mock/status/404" httpretty.register_uri( httpretty.GET, url_404, @@ -194,7 +194,7 @@ def name_callback(method, url): self.assertEqual(span.name, "HTTP GET") def test_not_foundbasic(self): - url_404 = "http://httpbin.org/status/404" + url_404 = "http://mock/status/404" httpretty.register_uri( httpretty.GET, url_404, @@ -460,7 +460,7 @@ def perform_request(url: str, session: requests.Session = None): return session.get(url) def test_credential_removal(self): - new_url = "http://username:password@httpbin.org/status/200" + new_url = "http://username:password@mock/status/200" self.perform_request(new_url) span = self.assert_span() diff --git a/instrumentation/opentelemetry-instrumentation-urllib/tests/test_metrics_instrumentation.py b/instrumentation/opentelemetry-instrumentation-urllib/tests/test_metrics_instrumentation.py index c9417fc67b..f56aa4f97d 100644 --- a/instrumentation/opentelemetry-instrumentation-urllib/tests/test_metrics_instrumentation.py +++ b/instrumentation/opentelemetry-instrumentation-urllib/tests/test_metrics_instrumentation.py @@ -27,8 +27,8 @@ class TestUrllibMetricsInstrumentation(TestBase): - URL = "http://httpbin.org/status/200" - URL_POST = "http://httpbin.org/post" + URL = "http://mock/status/200" + URL_POST = "http://mock/post" def setUp(self): super().setUp() diff --git a/instrumentation/opentelemetry-instrumentation-urllib/tests/test_urllib_integration.py b/instrumentation/opentelemetry-instrumentation-urllib/tests/test_urllib_integration.py index 9937d42176..35e9e1f1c7 100644 --- a/instrumentation/opentelemetry-instrumentation-urllib/tests/test_urllib_integration.py +++ b/instrumentation/opentelemetry-instrumentation-urllib/tests/test_urllib_integration.py @@ -46,9 +46,9 @@ class RequestsIntegrationTestBase(abc.ABC): # pylint: disable=no-member - URL = "http://httpbin.org/status/200" - URL_TIMEOUT = "http://httpbin.org/timeout/0" - URL_EXCEPTION = "http://httpbin.org/exception/0" + URL = "http://mock/status/200" + URL_TIMEOUT = "http://mock/timeout/0" + URL_EXCEPTION = "http://mock/exception/0" # pylint: disable=invalid-name def setUp(self): @@ -83,7 +83,7 @@ def setUp(self): ) httpretty.register_uri( httpretty.GET, - "http://httpbin.org/status/500", + "http://mock/status/500", status=500, ) @@ -142,7 +142,7 @@ def test_basic(self): ) def test_excluded_urls_explicit(self): - url_201 = "http://httpbin.org/status/201" + url_201 = "http://mock/status/201" httpretty.register_uri( httpretty.GET, url_201, @@ -172,7 +172,7 @@ def test_excluded_urls_from_env(self): self.assert_span(num_spans=1) def test_not_foundbasic(self): - url_404 = "http://httpbin.org/status/404/" + url_404 = "http://mock/status/404/" httpretty.register_uri( httpretty.GET, url_404, @@ -336,14 +336,14 @@ def test_custom_tracer_provider(self): def test_requests_exception_with_response(self, *_, **__): with self.assertRaises(HTTPError): - self.perform_request("http://httpbin.org/status/500") + self.perform_request("http://mock/status/500") span = self.assert_span() self.assertEqual( dict(span.attributes), { SpanAttributes.HTTP_METHOD: "GET", - SpanAttributes.HTTP_URL: "http://httpbin.org/status/500", + SpanAttributes.HTTP_URL: "http://mock/status/500", SpanAttributes.HTTP_STATUS_CODE: 500, }, ) @@ -365,7 +365,7 @@ def test_requests_timeout_exception(self, *_, **__): self.assertEqual(span.status.status_code, StatusCode.ERROR) def test_credential_removal(self): - url = "http://username:password@httpbin.org/status/200" + url = "http://username:password@mock/status/200" with self.assertRaises(Exception): self.perform_request(url) diff --git a/instrumentation/opentelemetry-instrumentation-urllib3/tests/test_urllib3_integration.py b/instrumentation/opentelemetry-instrumentation-urllib3/tests/test_urllib3_integration.py index ae59d57c51..315cec1112 100644 --- a/instrumentation/opentelemetry-instrumentation-urllib3/tests/test_urllib3_integration.py +++ b/instrumentation/opentelemetry-instrumentation-urllib3/tests/test_urllib3_integration.py @@ -35,8 +35,8 @@ class TestURLLib3Instrumentor(TestBase): - HTTP_URL = "http://httpbin.org/status/200" - HTTPS_URL = "https://httpbin.org/status/200" + HTTP_URL = "http://mock/status/200" + HTTPS_URL = "https://mock/status/200" def setUp(self): super().setUp() @@ -123,7 +123,7 @@ def test_basic_http_success(self): self.assert_success_span(response, self.HTTP_URL) def test_basic_http_success_using_connection_pool(self): - pool = urllib3.HTTPConnectionPool("httpbin.org") + pool = urllib3.HTTPConnectionPool("mock") response = pool.request("GET", "/status/200") self.assert_success_span(response, self.HTTP_URL) @@ -133,13 +133,13 @@ def test_basic_https_success(self): self.assert_success_span(response, self.HTTPS_URL) def test_basic_https_success_using_connection_pool(self): - pool = urllib3.HTTPSConnectionPool("httpbin.org") + pool = urllib3.HTTPSConnectionPool("mock") response = pool.request("GET", "/status/200") self.assert_success_span(response, self.HTTPS_URL) def test_basic_not_found(self): - url_404 = "http://httpbin.org/status/404" + url_404 = "http://mock/status/404" httpretty.register_uri(httpretty.GET, url_404, status=404) response = self.perform_request(url_404) @@ -152,30 +152,30 @@ def test_basic_not_found(self): self.assertIs(trace.status.StatusCode.ERROR, span.status.status_code) def test_basic_http_non_default_port(self): - url = "http://httpbin.org:666/status/200" + url = "http://mock:666/status/200" httpretty.register_uri(httpretty.GET, url, body="Hello!") response = self.perform_request(url) self.assert_success_span(response, url) def test_basic_http_absolute_url(self): - url = "http://httpbin.org:666/status/200" + url = "http://mock:666/status/200" httpretty.register_uri(httpretty.GET, url, body="Hello!") - pool = urllib3.HTTPConnectionPool("httpbin.org", port=666) + pool = urllib3.HTTPConnectionPool("mock", port=666) response = pool.request("GET", url) self.assert_success_span(response, url) def test_url_open_explicit_arg_parameters(self): - url = "http://httpbin.org:666/status/200" + url = "http://mock:666/status/200" httpretty.register_uri(httpretty.GET, url, body="Hello!") - pool = urllib3.HTTPConnectionPool("httpbin.org", port=666) + pool = urllib3.HTTPConnectionPool("mock", port=666) response = pool.urlopen(method="GET", url="/status/200") self.assert_success_span(response, url) def test_excluded_urls_explicit(self): - url_201 = "http://httpbin.org/status/201" + url_201 = "http://mock/status/201" httpretty.register_uri( httpretty.GET, url_201, @@ -301,7 +301,7 @@ def url_filter(url): self.assert_success_span(response, self.HTTP_URL) def test_credential_removal(self): - url = "http://username:password@httpbin.org/status/200" + url = "http://username:password@mock/status/200" response = self.perform_request(url) self.assert_success_span(response, self.HTTP_URL) @@ -339,7 +339,7 @@ def request_hook(span, request, headers, body): headers = {"header1": "value1", "header2": "value2"} body = "param1=1¶m2=2" - pool = urllib3.HTTPConnectionPool("httpbin.org") + pool = urllib3.HTTPConnectionPool("mock") response = pool.request( "POST", "/status/200", body=body, headers=headers ) @@ -366,7 +366,7 @@ def request_hook(span, request, headers, body): body = "param1=1¶m2=2" - pool = urllib3.HTTPConnectionPool("httpbin.org") + pool = urllib3.HTTPConnectionPool("mock") response = pool.urlopen("POST", "/status/200", body) self.assertEqual(b"Hello!", response.data) diff --git a/instrumentation/opentelemetry-instrumentation-urllib3/tests/test_urllib3_metrics.py b/instrumentation/opentelemetry-instrumentation-urllib3/tests/test_urllib3_metrics.py index ca691ebd47..6bf61a9fd8 100644 --- a/instrumentation/opentelemetry-instrumentation-urllib3/tests/test_urllib3_metrics.py +++ b/instrumentation/opentelemetry-instrumentation-urllib3/tests/test_urllib3_metrics.py @@ -26,7 +26,7 @@ class TestURLLib3InstrumentorMetric(HttpTestBase, TestBase): - HTTP_URL = "http://httpbin.org/status/200" + HTTP_URL = "http://mock/status/200" def setUp(self): super().setUp() @@ -68,11 +68,11 @@ def test_basic_metrics(self): min_data_point=client_duration_estimated, attributes={ "http.flavor": "1.1", - "http.host": "httpbin.org", + "http.host": "mock", "http.method": "GET", "http.scheme": "http", "http.status_code": 200, - "net.peer.name": "httpbin.org", + "net.peer.name": "mock", "net.peer.port": 80, }, ) @@ -91,11 +91,11 @@ def test_basic_metrics(self): min_data_point=0, attributes={ "http.flavor": "1.1", - "http.host": "httpbin.org", + "http.host": "mock", "http.method": "GET", "http.scheme": "http", "http.status_code": 200, - "net.peer.name": "httpbin.org", + "net.peer.name": "mock", "net.peer.port": 80, }, ) @@ -116,11 +116,11 @@ def test_basic_metrics(self): min_data_point=expected_size, attributes={ "http.flavor": "1.1", - "http.host": "httpbin.org", + "http.host": "mock", "http.method": "GET", "http.scheme": "http", "http.status_code": 200, - "net.peer.name": "httpbin.org", + "net.peer.name": "mock", "net.peer.port": 80, }, ) @@ -144,11 +144,11 @@ def test_str_request_body_size_metrics(self): min_data_point=6, attributes={ "http.flavor": "1.1", - "http.host": "httpbin.org", + "http.host": "mock", "http.method": "POST", "http.scheme": "http", "http.status_code": 200, - "net.peer.name": "httpbin.org", + "net.peer.name": "mock", "net.peer.port": 80, }, ) @@ -172,11 +172,11 @@ def test_bytes_request_body_size_metrics(self): min_data_point=6, attributes={ "http.flavor": "1.1", - "http.host": "httpbin.org", + "http.host": "mock", "http.method": "POST", "http.scheme": "http", "http.status_code": 200, - "net.peer.name": "httpbin.org", + "net.peer.name": "mock", "net.peer.port": 80, }, ) @@ -201,11 +201,11 @@ def test_fields_request_body_size_metrics(self): min_data_point=expected_value, attributes={ "http.flavor": "1.1", - "http.host": "httpbin.org", + "http.host": "mock", "http.method": "POST", "http.scheme": "http", "http.status_code": 200, - "net.peer.name": "httpbin.org", + "net.peer.name": "mock", "net.peer.port": 80, }, ) @@ -229,11 +229,11 @@ def test_bytesio_request_body_size_metrics(self): min_data_point=6, attributes={ "http.flavor": "1.1", - "http.host": "httpbin.org", + "http.host": "mock", "http.method": "POST", "http.scheme": "http", "http.status_code": 200, - "net.peer.name": "httpbin.org", + "net.peer.name": "mock", "net.peer.port": 80, }, ) diff --git a/instrumentation/opentelemetry-instrumentation-wsgi/tests/test_wsgi_middleware.py b/instrumentation/opentelemetry-instrumentation-wsgi/tests/test_wsgi_middleware.py index ffe2982052..926124caf3 100644 --- a/instrumentation/opentelemetry-instrumentation-wsgi/tests/test_wsgi_middleware.py +++ b/instrumentation/opentelemetry-instrumentation-wsgi/tests/test_wsgi_middleware.py @@ -437,10 +437,10 @@ def test_response_attributes(self): self.span.set_attribute.assert_has_calls(expected, any_order=True) def test_credential_removal(self): - self.environ["HTTP_HOST"] = "username:password@httpbin.com" + self.environ["HTTP_HOST"] = "username:password@mock" self.environ["PATH_INFO"] = "/status/200" expected = { - SpanAttributes.HTTP_URL: "http://httpbin.com/status/200", + SpanAttributes.HTTP_URL: "http://mock/status/200", SpanAttributes.NET_HOST_PORT: 80, } self.assertGreaterEqual( From 4637912418a0d23b32b0f280f8c84009a4141504 Mon Sep 17 00:00:00 2001 From: Matthew Grossman Date: Tue, 13 Jun 2023 04:23:48 -0700 Subject: [PATCH 05/29] Use `request_ctx` to determine whether or not `_teardown_request` should end flask span (#1692) Co-authored-by: Shalev Roda <65566801+shalevr@users.noreply.github.com> Co-authored-by: Diego Hurtado --- CHANGELOG.md | 2 + dev-requirements.txt | 2 +- .../pyproject.toml | 3 +- .../instrumentation/flask/__init__.py | 36 ++++++++++---- .../tests/base_test.py | 18 ++++++- .../tests/test_copy_context.py | 48 +++++++++++++++++++ tox.ini | 14 +++--- 7 files changed, 104 insertions(+), 19 deletions(-) create mode 100644 instrumentation/opentelemetry-instrumentation-flask/tests/test_copy_context.py diff --git a/CHANGELOG.md b/CHANGELOG.md index ee92bdab7a..ca0c1db9b9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,6 +44,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ([#1738](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1738)) - Fix `None does not implement middleware` error when there are no middlewares registered ([#1766](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1766)) +- Fix Flask instrumentation to only close the span if it was created by the same request context. + ([#1692](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1692)) ## Version 1.17.0/0.38b0 (2023-03-22) diff --git a/dev-requirements.txt b/dev-requirements.txt index a8efb950dd..8973fb9476 100644 --- a/dev-requirements.txt +++ b/dev-requirements.txt @@ -14,7 +14,7 @@ bleach==4.1.0 # transient dependency for readme-renderer grpcio-tools==1.29.0 mypy-protobuf>=1.23 protobuf~=3.13 -markupsafe==2.0.1 +markupsafe>=2.0.1 codespell==2.1.0 requests==2.28.1 ruamel.yaml==0.17.21 diff --git a/instrumentation/opentelemetry-instrumentation-flask/pyproject.toml b/instrumentation/opentelemetry-instrumentation-flask/pyproject.toml index 2e6b9d9646..885ca8965a 100644 --- a/instrumentation/opentelemetry-instrumentation-flask/pyproject.toml +++ b/instrumentation/opentelemetry-instrumentation-flask/pyproject.toml @@ -30,6 +30,7 @@ dependencies = [ "opentelemetry-instrumentation-wsgi == 0.40b0.dev", "opentelemetry-semantic-conventions == 0.40b0.dev", "opentelemetry-util-http == 0.40b0.dev", + "packaging >= 21.0", ] [project.optional-dependencies] @@ -38,7 +39,7 @@ instruments = [ ] test = [ "opentelemetry-instrumentation-flask[instruments]", - "markupsafe==2.0.1", + "markupsafe==2.1.2", "opentelemetry-test-utils == 0.40b0.dev", ] diff --git a/instrumentation/opentelemetry-instrumentation-flask/src/opentelemetry/instrumentation/flask/__init__.py b/instrumentation/opentelemetry-instrumentation-flask/src/opentelemetry/instrumentation/flask/__init__.py index fd3c40aab3..73c2f4fe2d 100644 --- a/instrumentation/opentelemetry-instrumentation-flask/src/opentelemetry/instrumentation/flask/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-flask/src/opentelemetry/instrumentation/flask/__init__.py @@ -238,13 +238,14 @@ def response_hook(span: Span, status: str, response_headers: List): API --- """ +import weakref from logging import getLogger -from threading import get_ident from time import time_ns from timeit import default_timer from typing import Collection import flask +from packaging import version as package_version import opentelemetry.instrumentation.wsgi as otel_wsgi from opentelemetry import context, trace @@ -265,11 +266,21 @@ def response_hook(span: Span, status: str, response_headers: List): _ENVIRON_STARTTIME_KEY = "opentelemetry-flask.starttime_key" _ENVIRON_SPAN_KEY = "opentelemetry-flask.span_key" _ENVIRON_ACTIVATION_KEY = "opentelemetry-flask.activation_key" -_ENVIRON_THREAD_ID_KEY = "opentelemetry-flask.thread_id_key" +_ENVIRON_REQCTX_REF_KEY = "opentelemetry-flask.reqctx_ref_key" _ENVIRON_TOKEN = "opentelemetry-flask.token" _excluded_urls_from_env = get_excluded_urls("FLASK") +if package_version.parse(flask.__version__) >= package_version.parse("2.2.0"): + + def _request_ctx_ref() -> weakref.ReferenceType: + return weakref.ref(flask.globals.request_ctx._get_current_object()) + +else: + + def _request_ctx_ref() -> weakref.ReferenceType: + return weakref.ref(flask._request_ctx_stack.top) + def get_default_span_name(): try: @@ -399,7 +410,7 @@ def _before_request(): activation = trace.use_span(span, end_on_exit=True) activation.__enter__() # pylint: disable=E1101 flask_request_environ[_ENVIRON_ACTIVATION_KEY] = activation - flask_request_environ[_ENVIRON_THREAD_ID_KEY] = get_ident() + flask_request_environ[_ENVIRON_REQCTX_REF_KEY] = _request_ctx_ref() flask_request_environ[_ENVIRON_SPAN_KEY] = span flask_request_environ[_ENVIRON_TOKEN] = token @@ -439,17 +450,22 @@ def _teardown_request(exc): return activation = flask.request.environ.get(_ENVIRON_ACTIVATION_KEY) - thread_id = flask.request.environ.get(_ENVIRON_THREAD_ID_KEY) - if not activation or thread_id != get_ident(): + + original_reqctx_ref = flask.request.environ.get( + _ENVIRON_REQCTX_REF_KEY + ) + current_reqctx_ref = _request_ctx_ref() + if not activation or original_reqctx_ref != current_reqctx_ref: # This request didn't start a span, maybe because it was created in # a way that doesn't run `before_request`, like when it is created # with `app.test_request_context`. # - # Similarly, check the thread_id against the current thread to ensure - # tear down only happens on the original thread. This situation can - # arise if the original thread handling the request spawn children - # threads and then uses something like copy_current_request_context - # to copy the request context. + # Similarly, check that the request_ctx that created the span + # matches the current request_ctx, and only tear down if they match. + # This situation can arise if the original request_ctx handling + # the request calls functions that push new request_ctx's, + # like any decorated with `flask.copy_current_request_context`. + return if exc is None: activation.__exit__(None, None, None) diff --git a/instrumentation/opentelemetry-instrumentation-flask/tests/base_test.py b/instrumentation/opentelemetry-instrumentation-flask/tests/base_test.py index a9cc4e55f7..6117521bb9 100644 --- a/instrumentation/opentelemetry-instrumentation-flask/tests/base_test.py +++ b/instrumentation/opentelemetry-instrumentation-flask/tests/base_test.py @@ -19,7 +19,7 @@ from werkzeug.test import Client from werkzeug.wrappers import Response -from opentelemetry import context +from opentelemetry import context, trace class InstrumentationTest: @@ -37,6 +37,21 @@ def _sqlcommenter_endpoint(): ) return sqlcommenter_flask_values + @staticmethod + def _copy_context_endpoint(): + @flask.copy_current_request_context + def _extract_header(): + return flask.request.headers["x-req"] + + # Despite `_extract_header` copying the request context, + # calling it shouldn't detach the parent Flask span's contextvar + request_header = _extract_header() + + return { + "span_name": trace.get_current_span().name, + "request_header": request_header, + } + @staticmethod def _multithreaded_endpoint(count): def do_random_stuff(): @@ -84,6 +99,7 @@ def excluded2_endpoint(): self.app.route("/hello/")(self._hello_endpoint) self.app.route("/sqlcommenter")(self._sqlcommenter_endpoint) self.app.route("/multithreaded")(self._multithreaded_endpoint) + self.app.route("/copy_context")(self._copy_context_endpoint) self.app.route("/excluded/")(self._hello_endpoint) self.app.route("/excluded")(excluded_endpoint) self.app.route("/excluded2")(excluded2_endpoint) diff --git a/instrumentation/opentelemetry-instrumentation-flask/tests/test_copy_context.py b/instrumentation/opentelemetry-instrumentation-flask/tests/test_copy_context.py new file mode 100644 index 0000000000..96268de5e7 --- /dev/null +++ b/instrumentation/opentelemetry-instrumentation-flask/tests/test_copy_context.py @@ -0,0 +1,48 @@ +# Copyright The OpenTelemetry Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +import flask +from werkzeug.test import Client +from werkzeug.wrappers import Response + +from opentelemetry.instrumentation.flask import FlaskInstrumentor +from opentelemetry.test.wsgitestutil import WsgiTestBase + +from .base_test import InstrumentationTest + + +class TestCopyContext(InstrumentationTest, WsgiTestBase): + def setUp(self): + super().setUp() + FlaskInstrumentor().instrument() + self.app = flask.Flask(__name__) + self._common_initialization() + + def tearDown(self): + super().tearDown() + with self.disable_logging(): + FlaskInstrumentor().uninstrument() + + def test_copycontext(self): + """Test that instrumentation tear down does not blow up + when the request calls functions where the context has been + copied via `flask.copy_current_request_context` + """ + self.app = flask.Flask(__name__) + self.app.route("/copy_context")(self._copy_context_endpoint) + client = Client(self.app, Response) + resp = client.get("/copy_context", headers={"x-req": "a-header"}) + + self.assertEqual(200, resp.status_code) + self.assertEqual("/copy_context", resp.json["span_name"]) + self.assertEqual("a-header", resp.json["request_header"]) diff --git a/tox.ini b/tox.ini index be821e9adf..32656d7214 100644 --- a/tox.ini +++ b/tox.ini @@ -84,8 +84,8 @@ envlist = pypy3-test-instrumentation-fastapi ; opentelemetry-instrumentation-flask - py3{7,8,9,10,11}-test-instrumentation-flask - pypy3-test-instrumentation-flask + py3{7,8,9,10,11}-test-instrumentation-flask{213,220} + pypy3-test-instrumentation-flask{213,220} ; opentelemetry-instrumentation-urllib py3{7,8,9,10,11}-test-instrumentation-urllib @@ -258,6 +258,8 @@ deps = falcon1: falcon ==1.4.1 falcon2: falcon >=2.0.0,<3.0.0 falcon3: falcon >=3.0.0,<4.0.0 + flask213: Flask ==2.1.3 + flask220: Flask >=2.2.0 grpc: pytest-asyncio sqlalchemy11: sqlalchemy>=1.1,<1.2 sqlalchemy14: aiosqlite @@ -304,7 +306,7 @@ changedir = test-instrumentation-elasticsearch{2,5,6}: instrumentation/opentelemetry-instrumentation-elasticsearch/tests test-instrumentation-falcon{1,2,3}: instrumentation/opentelemetry-instrumentation-falcon/tests test-instrumentation-fastapi: instrumentation/opentelemetry-instrumentation-fastapi/tests - test-instrumentation-flask: instrumentation/opentelemetry-instrumentation-flask/tests + test-instrumentation-flask{213,220}: instrumentation/opentelemetry-instrumentation-flask/tests test-instrumentation-urllib: instrumentation/opentelemetry-instrumentation-urllib/tests test-instrumentation-urllib3: instrumentation/opentelemetry-instrumentation-urllib3/tests test-instrumentation-grpc: instrumentation/opentelemetry-instrumentation-grpc/tests @@ -365,8 +367,8 @@ commands_pre = grpc: pip install {toxinidir}/instrumentation/opentelemetry-instrumentation-grpc[test] - falcon{1,2,3},flask,django{1,2,3,4},pyramid,tornado,starlette,fastapi,aiohttp,asgi,requests,urllib,urllib3,wsgi: pip install {toxinidir}/util/opentelemetry-util-http[test] - wsgi,falcon{1,2,3},flask,django{1,2,3,4},pyramid: pip install {toxinidir}/instrumentation/opentelemetry-instrumentation-wsgi[test] + falcon{1,2,3},flask{213,220},django{1,2,3,4},pyramid,tornado,starlette,fastapi,aiohttp,asgi,requests,urllib,urllib3,wsgi: pip install {toxinidir}/util/opentelemetry-util-http[test] + wsgi,falcon{1,2,3},flask{213,220},django{1,2,3,4},pyramid: pip install {toxinidir}/instrumentation/opentelemetry-instrumentation-wsgi[test] asgi,django{3,4},starlette,fastapi: pip install {toxinidir}/instrumentation/opentelemetry-instrumentation-asgi[test] asyncpg: pip install {toxinidir}/instrumentation/opentelemetry-instrumentation-asyncpg[test] @@ -380,7 +382,7 @@ commands_pre = falcon{1,2,3}: pip install {toxinidir}/instrumentation/opentelemetry-instrumentation-falcon[test] - flask: pip install {toxinidir}/instrumentation/opentelemetry-instrumentation-flask[test] + flask{213,220}: pip install {toxinidir}/instrumentation/opentelemetry-instrumentation-flask[test] urllib: pip install {toxinidir}/instrumentation/opentelemetry-instrumentation-urllib[test] From 818ef4322303dabc066a7f84e0f8879e5f558df9 Mon Sep 17 00:00:00 2001 From: Srikanth Chekuri Date: Tue, 13 Jun 2023 17:24:41 +0530 Subject: [PATCH 06/29] remove srikanthccv from maintainers (#1792) Co-authored-by: Shalev Roda <65566801+shalevr@users.noreply.github.com> Co-authored-by: Diego Hurtado From 37d85f07458900762f75ec6e4942c5dec2f93694 Mon Sep 17 00:00:00 2001 From: Nimrod Shlagman Date: Tue, 13 Jun 2023 15:37:55 +0300 Subject: [PATCH 07/29] Sanitize redis db_statement by default (#1776) Co-authored-by: Srikanth Chekuri Co-authored-by: Shalev Roda <65566801+shalevr@users.noreply.github.com> --- CHANGELOG.md | 2 + .../instrumentation/redis/__init__.py | 35 +------------- .../redis/environment_variables.py | 17 ------- .../instrumentation/redis/util.py | 46 ++++++------------- .../tests/test_redis.py | 28 +---------- .../tests/redis/test_redis_functional.py | 42 +++++++---------- 6 files changed, 36 insertions(+), 134 deletions(-) delete mode 100644 instrumentation/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/environment_variables.py diff --git a/CHANGELOG.md b/CHANGELOG.md index ca0c1db9b9..000216e2a1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,6 +38,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Fix redis db.statements to be sanitized by default + ([#1778](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1778)) - Fix elasticsearch db.statement attribute to be sanitized by default ([#1758](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1758)) - Fix `AttributeError` when AWS Lambda handler receives a list event 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 c1068bda27..188840c7b8 100644 --- a/instrumentation/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/__init__.py @@ -64,8 +64,6 @@ async def redis_get(): response_hook (Callable) - a function with extra user-defined logic to be performed after performing the request this function signature is: def response_hook(span: Span, instance: redis.connection.Connection, response) -> None -sanitize_query (Boolean) - default False, enable the Redis query sanitization - for example: .. code: python @@ -88,27 +86,11 @@ def response_hook(span, instance, response): client = redis.StrictRedis(host="localhost", port=6379) client.get("my-key") -Configuration -------------- - -Query sanitization -****************** -To enable query sanitization with an environment variable, set -``OTEL_PYTHON_INSTRUMENTATION_SANITIZE_REDIS`` to "true". - -For example, - -:: - - export OTEL_PYTHON_INSTRUMENTATION_SANITIZE_REDIS="true" - -will result in traced queries like "SET ? ?". API --- """ import typing -from os import environ from typing import Any, Collection import redis @@ -116,9 +98,6 @@ def response_hook(span, instance, response): from opentelemetry import trace from opentelemetry.instrumentation.instrumentor import BaseInstrumentor -from opentelemetry.instrumentation.redis.environment_variables import ( - OTEL_PYTHON_INSTRUMENTATION_SANITIZE_REDIS, -) from opentelemetry.instrumentation.redis.package import _instruments from opentelemetry.instrumentation.redis.util import ( _extract_conn_attributes, @@ -161,10 +140,9 @@ def _instrument( tracer, request_hook: _RequestHookT = None, response_hook: _ResponseHookT = None, - sanitize_query: bool = False, ): def _traced_execute_command(func, instance, args, kwargs): - query = _format_command_args(args, sanitize_query) + query = _format_command_args(args) if len(args) > 0 and args[0]: name = args[0] @@ -194,7 +172,7 @@ def _traced_execute_pipeline(func, instance, args, kwargs): cmds = [ _format_command_args( - c.args if hasattr(c, "args") else c[0], sanitize_query + c.args if hasattr(c, "args") else c[0], ) for c in command_stack ] @@ -307,15 +285,6 @@ def _instrument(self, **kwargs): tracer, request_hook=kwargs.get("request_hook"), response_hook=kwargs.get("response_hook"), - sanitize_query=kwargs.get( - "sanitize_query", - environ.get( - OTEL_PYTHON_INSTRUMENTATION_SANITIZE_REDIS, "false" - ) - .lower() - .strip() - == "true", - ), ) def _uninstrument(self, **kwargs): diff --git a/instrumentation/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/environment_variables.py b/instrumentation/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/environment_variables.py deleted file mode 100644 index 750b97445e..0000000000 --- a/instrumentation/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/environment_variables.py +++ /dev/null @@ -1,17 +0,0 @@ -# Copyright The OpenTelemetry Authors -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. - -OTEL_PYTHON_INSTRUMENTATION_SANITIZE_REDIS = ( - "OTEL_PYTHON_INSTRUMENTATION_SANITIZE_REDIS" -) diff --git a/instrumentation/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/util.py b/instrumentation/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/util.py index 1eadaba718..b24f9b2655 100644 --- a/instrumentation/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/util.py +++ b/instrumentation/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/util.py @@ -48,41 +48,23 @@ def _extract_conn_attributes(conn_kwargs): return attributes -def _format_command_args(args, sanitize_query): +def _format_command_args(args): """Format and sanitize command arguments, and trim them as needed""" cmd_max_len = 1000 value_too_long_mark = "..." - if sanitize_query: - # Sanitized query format: "COMMAND ? ?" - args_length = len(args) - if args_length > 0: - out = [str(args[0])] + ["?"] * (args_length - 1) - out_str = " ".join(out) - if len(out_str) > cmd_max_len: - out_str = ( - out_str[: cmd_max_len - len(value_too_long_mark)] - + value_too_long_mark - ) - else: - out_str = "" - return out_str + # Sanitized query format: "COMMAND ? ?" + args_length = len(args) + if args_length > 0: + out = [str(args[0])] + ["?"] * (args_length - 1) + out_str = " ".join(out) - value_max_len = 100 - length = 0 - out = [] - for arg in args: - cmd = str(arg) + if len(out_str) > cmd_max_len: + out_str = ( + out_str[: cmd_max_len - len(value_too_long_mark)] + + value_too_long_mark + ) + else: + out_str = "" - if len(cmd) > value_max_len: - cmd = cmd[:value_max_len] + value_too_long_mark - - if length + len(cmd) > cmd_max_len: - prefix = cmd[: cmd_max_len - length] - out.append(f"{prefix}{value_too_long_mark}") - break - - out.append(cmd) - length += len(cmd) - - return " ".join(out) + return out_str diff --git a/instrumentation/opentelemetry-instrumentation-redis/tests/test_redis.py b/instrumentation/opentelemetry-instrumentation-redis/tests/test_redis.py index 56a0df6a0a..cc6e7de75a 100644 --- a/instrumentation/opentelemetry-instrumentation-redis/tests/test_redis.py +++ b/instrumentation/opentelemetry-instrumentation-redis/tests/test_redis.py @@ -168,22 +168,11 @@ def test_query_sanitizer_enabled(self): span = spans[0] self.assertEqual(span.attributes.get("db.statement"), "SET ? ?") - def test_query_sanitizer_enabled_env(self): + def test_query_sanitizer(self): redis_client = redis.Redis() connection = redis.connection.Connection() redis_client.connection = connection - RedisInstrumentor().uninstrument() - - env_patch = mock.patch.dict( - "os.environ", - {"OTEL_PYTHON_INSTRUMENTATION_SANITIZE_REDIS": "true"}, - ) - env_patch.start() - RedisInstrumentor().instrument( - tracer_provider=self.tracer_provider, - ) - with mock.patch.object(redis_client, "connection"): redis_client.set("key", "value") @@ -192,21 +181,6 @@ def test_query_sanitizer_enabled_env(self): span = spans[0] self.assertEqual(span.attributes.get("db.statement"), "SET ? ?") - env_patch.stop() - - def test_query_sanitizer_disabled(self): - redis_client = redis.Redis() - connection = redis.connection.Connection() - redis_client.connection = connection - - with mock.patch.object(redis_client, "connection"): - redis_client.set("key", "value") - - spans = self.memory_exporter.get_finished_spans() - self.assertEqual(len(spans), 1) - - span = spans[0] - self.assertEqual(span.attributes.get("db.statement"), "SET key value") def test_no_op_tracer_provider(self): RedisInstrumentor().uninstrument() diff --git a/tests/opentelemetry-docker-tests/tests/redis/test_redis_functional.py b/tests/opentelemetry-docker-tests/tests/redis/test_redis_functional.py index 675a37fa9f..dc9cf8b1dc 100644 --- a/tests/opentelemetry-docker-tests/tests/redis/test_redis_functional.py +++ b/tests/opentelemetry-docker-tests/tests/redis/test_redis_functional.py @@ -47,9 +47,7 @@ def _check_span(self, span, name): def test_long_command_sanitized(self): RedisInstrumentor().uninstrument() - RedisInstrumentor().instrument( - tracer_provider=self.tracer_provider, sanitize_query=True - ) + RedisInstrumentor().instrument(tracer_provider=self.tracer_provider) self.redis_client.mget(*range(2000)) @@ -75,7 +73,7 @@ def test_long_command(self): self._check_span(span, "MGET") self.assertTrue( span.attributes.get(SpanAttributes.DB_STATEMENT).startswith( - "MGET 0 1 2 3" + "MGET ? ? ? ?" ) ) self.assertTrue( @@ -84,9 +82,7 @@ def test_long_command(self): def test_basics_sanitized(self): RedisInstrumentor().uninstrument() - RedisInstrumentor().instrument( - tracer_provider=self.tracer_provider, sanitize_query=True - ) + RedisInstrumentor().instrument(tracer_provider=self.tracer_provider) self.assertIsNone(self.redis_client.get("cheese")) spans = self.memory_exporter.get_finished_spans() @@ -105,15 +101,13 @@ def test_basics(self): span = spans[0] self._check_span(span, "GET") self.assertEqual( - span.attributes.get(SpanAttributes.DB_STATEMENT), "GET cheese" + span.attributes.get(SpanAttributes.DB_STATEMENT), "GET ?" ) self.assertEqual(span.attributes.get("db.redis.args_length"), 2) def test_pipeline_traced_sanitized(self): RedisInstrumentor().uninstrument() - RedisInstrumentor().instrument( - tracer_provider=self.tracer_provider, sanitize_query=True - ) + RedisInstrumentor().instrument(tracer_provider=self.tracer_provider) with self.redis_client.pipeline(transaction=False) as pipeline: pipeline.set("blah", 32) @@ -144,15 +138,13 @@ def test_pipeline_traced(self): self._check_span(span, "SET RPUSH HGETALL") self.assertEqual( span.attributes.get(SpanAttributes.DB_STATEMENT), - "SET blah 32\nRPUSH foo éé\nHGETALL xxx", + "SET ? ?\nRPUSH ? ?\nHGETALL ?", ) self.assertEqual(span.attributes.get("db.redis.pipeline_length"), 3) def test_pipeline_immediate_sanitized(self): RedisInstrumentor().uninstrument() - RedisInstrumentor().instrument( - tracer_provider=self.tracer_provider, sanitize_query=True - ) + RedisInstrumentor().instrument(tracer_provider=self.tracer_provider) with self.redis_client.pipeline() as pipeline: pipeline.set("a", 1) @@ -182,7 +174,7 @@ def test_pipeline_immediate(self): span = spans[0] self._check_span(span, "SET") self.assertEqual( - span.attributes.get(SpanAttributes.DB_STATEMENT), "SET b 2" + span.attributes.get(SpanAttributes.DB_STATEMENT), "SET ? ?" ) def test_parent(self): @@ -230,7 +222,7 @@ def test_basics(self): span = spans[0] self._check_span(span, "GET") self.assertEqual( - span.attributes.get(SpanAttributes.DB_STATEMENT), "GET cheese" + span.attributes.get(SpanAttributes.DB_STATEMENT), "GET ?" ) self.assertEqual(span.attributes.get("db.redis.args_length"), 2) @@ -247,7 +239,7 @@ def test_pipeline_traced(self): self._check_span(span, "SET RPUSH HGETALL") self.assertEqual( span.attributes.get(SpanAttributes.DB_STATEMENT), - "SET blah 32\nRPUSH foo éé\nHGETALL xxx", + "SET ? ?\nRPUSH ? ?\nHGETALL ?", ) self.assertEqual(span.attributes.get("db.redis.pipeline_length"), 3) @@ -308,7 +300,7 @@ def test_long_command(self): self._check_span(span, "MGET") self.assertTrue( span.attributes.get(SpanAttributes.DB_STATEMENT).startswith( - "MGET 0 1 2 3" + "MGET ? ? ? ?" ) ) self.assertTrue( @@ -322,7 +314,7 @@ def test_basics(self): span = spans[0] self._check_span(span, "GET") self.assertEqual( - span.attributes.get(SpanAttributes.DB_STATEMENT), "GET cheese" + span.attributes.get(SpanAttributes.DB_STATEMENT), "GET ?" ) self.assertEqual(span.attributes.get("db.redis.args_length"), 2) @@ -344,7 +336,7 @@ async def pipeline_simple(): self._check_span(span, "SET RPUSH HGETALL") self.assertEqual( span.attributes.get(SpanAttributes.DB_STATEMENT), - "SET blah 32\nRPUSH foo éé\nHGETALL xxx", + "SET ? ?\nRPUSH ? ?\nHGETALL ?", ) self.assertEqual(span.attributes.get("db.redis.pipeline_length"), 3) @@ -364,7 +356,7 @@ async def pipeline_immediate(): span = spans[0] self._check_span(span, "SET") self.assertEqual( - span.attributes.get(SpanAttributes.DB_STATEMENT), "SET b 2" + span.attributes.get(SpanAttributes.DB_STATEMENT), "SET ? ?" ) def test_parent(self): @@ -412,7 +404,7 @@ def test_basics(self): span = spans[0] self._check_span(span, "GET") self.assertEqual( - span.attributes.get(SpanAttributes.DB_STATEMENT), "GET cheese" + span.attributes.get(SpanAttributes.DB_STATEMENT), "GET ?" ) self.assertEqual(span.attributes.get("db.redis.args_length"), 2) @@ -434,7 +426,7 @@ async def pipeline_simple(): self._check_span(span, "SET RPUSH HGETALL") self.assertEqual( span.attributes.get(SpanAttributes.DB_STATEMENT), - "SET blah 32\nRPUSH foo éé\nHGETALL xxx", + "SET ? ?\nRPUSH ? ?\nHGETALL ?", ) self.assertEqual(span.attributes.get("db.redis.pipeline_length"), 3) @@ -488,5 +480,5 @@ def test_get(self): span = spans[0] self._check_span(span, "GET") self.assertEqual( - span.attributes.get(SpanAttributes.DB_STATEMENT), "GET foo" + span.attributes.get(SpanAttributes.DB_STATEMENT), "GET ?" ) From a5ed4da478c4360fd6e24893f7574b150431b7ee Mon Sep 17 00:00:00 2001 From: Phillip Verheyden Date: Tue, 13 Jun 2023 08:07:28 -0500 Subject: [PATCH 08/29] Relax httpx version to allow >= 0.18.0 (#1748) --- CHANGELOG.md | 2 ++ .../opentelemetry-instrumentation-httpx/pyproject.toml | 2 +- .../tests/test_httpx_integration.py | 4 ++-- .../src/opentelemetry/instrumentation/bootstrap_gen.py | 2 +- tox.ini | 2 +- 5 files changed, 7 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 000216e2a1..777b5a3530 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## Unreleased +- Instrument all httpx versions >= 0.18. ([#1748](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1748)) + ## Version 1.18.0/0.39b0 (2023-05-10) - `opentelemetry-instrumentation-system-metrics` Add `process.` prefix to `runtime.memory`, `runtime.cpu.time`, and `runtime.gc_count`. Change `runtime.memory` from count to UpDownCounter. ([#1735](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1735)) diff --git a/instrumentation/opentelemetry-instrumentation-httpx/pyproject.toml b/instrumentation/opentelemetry-instrumentation-httpx/pyproject.toml index 229fca1611..d079de20b3 100644 --- a/instrumentation/opentelemetry-instrumentation-httpx/pyproject.toml +++ b/instrumentation/opentelemetry-instrumentation-httpx/pyproject.toml @@ -32,7 +32,7 @@ dependencies = [ [project.optional-dependencies] instruments = [ - "httpx >= 0.18.0, <= 0.23.0", + "httpx >= 0.18.0", ] test = [ "opentelemetry-instrumentation-httpx[instruments]", diff --git a/instrumentation/opentelemetry-instrumentation-httpx/tests/test_httpx_integration.py b/instrumentation/opentelemetry-instrumentation-httpx/tests/test_httpx_integration.py index c0d3705dca..86c9a56ab2 100644 --- a/instrumentation/opentelemetry-instrumentation-httpx/tests/test_httpx_integration.py +++ b/instrumentation/opentelemetry-instrumentation-httpx/tests/test_httpx_integration.py @@ -59,7 +59,7 @@ def _async_call(coro: typing.Coroutine) -> asyncio.Task: def _response_hook(span, request: "RequestInfo", response: "ResponseInfo"): span.set_attribute( HTTP_RESPONSE_BODY, - response[2].read(), + b"".join(response[2]), ) @@ -68,7 +68,7 @@ async def _async_response_hook( ): span.set_attribute( HTTP_RESPONSE_BODY, - await response[2].aread(), + b"".join([part async for part in response[2]]), ) diff --git a/opentelemetry-instrumentation/src/opentelemetry/instrumentation/bootstrap_gen.py b/opentelemetry-instrumentation/src/opentelemetry/instrumentation/bootstrap_gen.py index 8200196ca8..f705324289 100644 --- a/opentelemetry-instrumentation/src/opentelemetry/instrumentation/bootstrap_gen.py +++ b/opentelemetry-instrumentation/src/opentelemetry/instrumentation/bootstrap_gen.py @@ -81,7 +81,7 @@ "instrumentation": "opentelemetry-instrumentation-grpc==0.40b0.dev", }, "httpx": { - "library": "httpx >= 0.18.0, <= 0.23.0", + "library": "httpx >= 0.18.0", "instrumentation": "opentelemetry-instrumentation-httpx==0.40b0.dev", }, "jinja2": { diff --git a/tox.ini b/tox.ini index 32656d7214..2313eab1d2 100644 --- a/tox.ini +++ b/tox.ini @@ -277,7 +277,7 @@ deps = httpx18: httpx>=0.18.0,<0.19.0 httpx18: respx~=0.17.0 httpx21: httpx>=0.19.0 - httpx21: respx~=0.19.0 + httpx21: respx~=0.20.1 ; FIXME: add coverage testing ; FIXME: add mypy testing From 743ac64661897087f2dca7f696e481648eb4d0fe Mon Sep 17 00:00:00 2001 From: Maciej Nachtygal <82878433+macieyng@users.noreply.github.com> Date: Fri, 16 Jun 2023 00:21:05 +0200 Subject: [PATCH 09/29] Issue #1757 - Update HTTP server/client instrumentation span names (#1759) Co-authored-by: Shalev Roda <65566801+shalevr@users.noreply.github.com> Co-authored-by: Srikanth Chekuri --- CHANGELOG.md | 4 ++ .../aiohttp_client/__init__.py | 2 +- .../tests/test_aiohttp_client_integration.py | 13 ++++--- .../instrumentation/asgi/__init__.py | 19 ++++++---- .../tests/test_asgi_middleware.py | 14 +++---- .../django/middleware/otel_middleware.py | 12 ++---- .../tests/test_middleware.py | 26 ++++--------- .../tests/test_middleware_asgi.py | 14 +++---- .../tests/test_falcon.py | 2 +- .../instrumentation/fastapi/__init__.py | 37 +++++++++++++++---- .../tests/test_fastapi_instrumentation.py | 6 +-- .../tests/test_programmatic.py | 2 +- .../instrumentation/httpx/__init__.py | 2 +- .../tests/test_httpx_integration.py | 8 ++-- .../tests/test_programmatic.py | 2 +- .../instrumentation/requests/__init__.py | 12 +++++- .../tests/test_requests_integration.py | 4 +- .../tests/test_requests_ip_support.py | 2 +- .../instrumentation/starlette/__init__.py | 37 +++++++++++++++---- .../tests/test_starlette_instrumentation.py | 4 +- .../instrumentation/tornado/__init__.py | 23 +++++++++--- .../tests/test_instrumentation.py | 23 ++++++------ .../instrumentation/urllib/__init__.py | 2 +- .../tests/test_urllib_integration.py | 4 +- .../instrumentation/urllib3/__init__.py | 2 +- .../tests/test_urllib3_integration.py | 2 +- .../tests/test_urllib3_ip_support.py | 2 +- .../instrumentation/wsgi/__init__.py | 17 ++++++++- .../tests/test_wsgi_middleware.py | 13 ++++--- .../opentelemetry/instrumentation/utils.py | 2 +- 30 files changed, 194 insertions(+), 118 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 777b5a3530..9959a49d01 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -51,6 +51,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Fix Flask instrumentation to only close the span if it was created by the same request context. ([#1692](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1692)) +### Changed +- Update HTTP server/client instrumentation span names to comply with spec + ([#1759](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1759) + ## Version 1.17.0/0.38b0 (2023-03-22) ### Added diff --git a/instrumentation/opentelemetry-instrumentation-aiohttp-client/src/opentelemetry/instrumentation/aiohttp_client/__init__.py b/instrumentation/opentelemetry-instrumentation-aiohttp-client/src/opentelemetry/instrumentation/aiohttp_client/__init__.py index 101e67f2ad..65e1601f34 100644 --- a/instrumentation/opentelemetry-instrumentation-aiohttp-client/src/opentelemetry/instrumentation/aiohttp_client/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-aiohttp-client/src/opentelemetry/instrumentation/aiohttp_client/__init__.py @@ -179,7 +179,7 @@ async def on_request_start( return http_method = params.method.upper() - request_span_name = f"HTTP {http_method}" + request_span_name = f"{http_method}" request_url = ( remove_url_credentials(trace_config_ctx.url_filter(params.url)) if callable(trace_config_ctx.url_filter) diff --git a/instrumentation/opentelemetry-instrumentation-aiohttp-client/tests/test_aiohttp_client_integration.py b/instrumentation/opentelemetry-instrumentation-aiohttp-client/tests/test_aiohttp_client_integration.py index 7c3d7b634d..6af9d41900 100644 --- a/instrumentation/opentelemetry-instrumentation-aiohttp-client/tests/test_aiohttp_client_integration.py +++ b/instrumentation/opentelemetry-instrumentation-aiohttp-client/tests/test_aiohttp_client_integration.py @@ -119,7 +119,7 @@ def test_status_codes(self): self.assert_spans( [ ( - "HTTP GET", + "GET", (span_status, None), { SpanAttributes.HTTP_METHOD: "GET", @@ -213,7 +213,7 @@ def strip_query_params(url: yarl.URL) -> str: self.assert_spans( [ ( - "HTTP GET", + "GET", (StatusCode.UNSET, None), { SpanAttributes.HTTP_METHOD: "GET", @@ -247,7 +247,7 @@ async def do_request(url): self.assert_spans( [ ( - "HTTP GET", + "GET", (expected_status, None), { SpanAttributes.HTTP_METHOD: "GET", @@ -274,7 +274,7 @@ async def request_handler(request): self.assert_spans( [ ( - "HTTP GET", + "GET", (StatusCode.ERROR, None), { SpanAttributes.HTTP_METHOD: "GET", @@ -301,7 +301,7 @@ async def request_handler(request): self.assert_spans( [ ( - "HTTP GET", + "GET", (StatusCode.ERROR, None), { SpanAttributes.HTTP_METHOD: "GET", @@ -338,7 +338,7 @@ async def do_request(url): self.assert_spans( [ ( - "HTTP GET", + "GET", (StatusCode.UNSET, None), { SpanAttributes.HTTP_METHOD: "GET", @@ -391,6 +391,7 @@ def test_instrument(self): self.get_default_request(), self.URL, self.default_handler ) span = self.assert_spans(1) + self.assertEqual("GET", span.name) self.assertEqual("GET", span.attributes[SpanAttributes.HTTP_METHOD]) self.assertEqual( f"http://{host}:{port}/test-path", 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 6fc88d3eeb..010c6accde 100644 --- a/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py @@ -415,18 +415,23 @@ def set_status_code(span, status_code): def get_default_span_details(scope: dict) -> Tuple[str, dict]: - """Default implementation for get_default_span_details + """ + Default span name is the HTTP method and URL path, or just the method. + https://github.com/open-telemetry/opentelemetry-specification/pull/3165 + https://opentelemetry.io/docs/reference/specification/trace/semantic_conventions/http/#name + Args: scope: the ASGI scope dictionary Returns: a tuple of the span name, and any attributes to attach to the span. """ - span_name = ( - scope.get("path", "").strip() - or f"HTTP {scope.get('method', '').strip()}" - ) - - return span_name, {} + path = scope.get("path", "").strip() + method = scope.get("method", "").strip() + if method and path: # http + return f"{method} {path}", {} + if path: # websocket + return path, {} + return method, {} # http with no path def _collect_target_attribute( diff --git a/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py b/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py index 7cbae79c6d..f9a5731fd3 100644 --- a/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py +++ b/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py @@ -142,12 +142,12 @@ def validate_outputs(self, outputs, error=None, modifiers=None): self.assertEqual(len(span_list), 4) expected = [ { - "name": "/ http receive", + "name": "GET / http receive", "kind": trace_api.SpanKind.INTERNAL, "attributes": {"type": "http.request"}, }, { - "name": "/ http send", + "name": "GET / http send", "kind": trace_api.SpanKind.INTERNAL, "attributes": { SpanAttributes.HTTP_STATUS_CODE: 200, @@ -155,12 +155,12 @@ def validate_outputs(self, outputs, error=None, modifiers=None): }, }, { - "name": "/ http send", + "name": "GET / http send", "kind": trace_api.SpanKind.INTERNAL, "attributes": {"type": "http.response.body"}, }, { - "name": "/", + "name": "GET /", "kind": trace_api.SpanKind.SERVER, "attributes": { SpanAttributes.HTTP_METHOD: "GET", @@ -231,7 +231,7 @@ def update_expected_span_name(expected): entry["name"] = span_name else: entry["name"] = " ".join( - [span_name] + entry["name"].split(" ")[1:] + [span_name] + entry["name"].split(" ")[2:] ) return expected @@ -493,9 +493,9 @@ def update_expected_hook_results(expected): for entry in expected: if entry["kind"] == trace_api.SpanKind.SERVER: entry["name"] = "name from server hook" - elif entry["name"] == "/ http receive": + elif entry["name"] == "GET / http receive": entry["name"] = "name from client request hook" - elif entry["name"] == "/ http send": + elif entry["name"] == "GET / http send": entry["attributes"].update({"attr-from-hook": "value"}) return expected diff --git a/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware/otel_middleware.py b/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware/otel_middleware.py index 1baa05eca9..02313a48ee 100644 --- a/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware/otel_middleware.py +++ b/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware/otel_middleware.py @@ -173,18 +173,12 @@ def _get_span_name(request): match = resolve(request.path) if hasattr(match, "route"): - return match.route + return f"{request.method} {match.route}" - # Instead of using `view_name`, better to use `_func_name` as some applications can use similar - # view names in different modules - if hasattr(match, "_func_name"): - return match._func_name # pylint: disable=protected-access - - # Fallback for safety as `_func_name` private field - return match.view_name + return request.method except Resolver404: - return f"HTTP {request.method}" + return request.method # pylint: disable=too-many-locals def process_request(self, request): diff --git a/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware.py b/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware.py index e235c8840e..1f28819df0 100644 --- a/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware.py +++ b/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware.py @@ -150,9 +150,9 @@ def test_templated_route_get(self): self.assertEqual( span.name, - "^route/(?P[0-9]{4})/template/$" + "GET ^route/(?P[0-9]{4})/template/$" if DJANGO_2_2 - else "tests.views.traced_template", + else "GET", ) self.assertEqual(span.kind, SpanKind.SERVER) self.assertEqual(span.status.status_code, StatusCode.UNSET) @@ -177,9 +177,7 @@ def test_traced_get(self): span = spans[0] - self.assertEqual( - span.name, "^traced/" if DJANGO_2_2 else "tests.views.traced" - ) + self.assertEqual(span.name, "GET ^traced/" if DJANGO_2_2 else "GET") self.assertEqual(span.kind, SpanKind.SERVER) self.assertEqual(span.status.status_code, StatusCode.UNSET) self.assertEqual(span.attributes[SpanAttributes.HTTP_METHOD], "GET") @@ -215,9 +213,7 @@ def test_traced_post(self): span = spans[0] - self.assertEqual( - span.name, "^traced/" if DJANGO_2_2 else "tests.views.traced" - ) + self.assertEqual(span.name, "POST ^traced/" if DJANGO_2_2 else "POST") self.assertEqual(span.kind, SpanKind.SERVER) self.assertEqual(span.status.status_code, StatusCode.UNSET) self.assertEqual(span.attributes[SpanAttributes.HTTP_METHOD], "POST") @@ -241,9 +237,7 @@ def test_error(self): span = spans[0] - self.assertEqual( - span.name, "^error/" if DJANGO_2_2 else "tests.views.error" - ) + self.assertEqual(span.name, "GET ^error/" if DJANGO_2_2 else "GET") self.assertEqual(span.kind, SpanKind.SERVER) self.assertEqual(span.status.status_code, StatusCode.ERROR) self.assertEqual(span.attributes[SpanAttributes.HTTP_METHOD], "GET") @@ -307,9 +301,7 @@ def test_span_name(self): span = span_list[0] self.assertEqual( span.name, - "^span_name/([0-9]{4})/$" - if DJANGO_2_2 - else "tests.views.route_span_name", + "GET ^span_name/([0-9]{4})/$" if DJANGO_2_2 else "GET", ) def test_span_name_for_query_string(self): @@ -323,9 +315,7 @@ def test_span_name_for_query_string(self): span = span_list[0] self.assertEqual( span.name, - "^span_name/([0-9]{4})/$" - if DJANGO_2_2 - else "tests.views.route_span_name", + "GET ^span_name/([0-9]{4})/$" if DJANGO_2_2 else "GET", ) def test_span_name_404(self): @@ -334,7 +324,7 @@ def test_span_name_404(self): self.assertEqual(len(span_list), 1) span = span_list[0] - self.assertEqual(span.name, "HTTP GET") + self.assertEqual(span.name, "GET") def test_traced_request_attrs(self): Client().get("/span_name/1234/", CONTENT_TYPE="test/ct") diff --git a/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware_asgi.py b/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware_asgi.py index a78501bcb8..0e2472d15e 100644 --- a/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware_asgi.py +++ b/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware_asgi.py @@ -137,7 +137,7 @@ async def test_templated_route_get(self): span = spans[0] - self.assertEqual(span.name, "^route/(?P[0-9]{4})/template/$") + self.assertEqual(span.name, "GET ^route/(?P[0-9]{4})/template/$") self.assertEqual(span.kind, SpanKind.SERVER) self.assertEqual(span.status.status_code, StatusCode.UNSET) self.assertEqual(span.attributes[SpanAttributes.HTTP_METHOD], "GET") @@ -160,7 +160,7 @@ async def test_traced_get(self): span = spans[0] - self.assertEqual(span.name, "^traced/") + self.assertEqual(span.name, "GET ^traced/") self.assertEqual(span.kind, SpanKind.SERVER) self.assertEqual(span.status.status_code, StatusCode.UNSET) self.assertEqual(span.attributes[SpanAttributes.HTTP_METHOD], "GET") @@ -195,7 +195,7 @@ async def test_traced_post(self): span = spans[0] - self.assertEqual(span.name, "^traced/") + self.assertEqual(span.name, "POST ^traced/") self.assertEqual(span.kind, SpanKind.SERVER) self.assertEqual(span.status.status_code, StatusCode.UNSET) self.assertEqual(span.attributes[SpanAttributes.HTTP_METHOD], "POST") @@ -218,7 +218,7 @@ async def test_error(self): span = spans[0] - self.assertEqual(span.name, "^error/") + self.assertEqual(span.name, "GET ^error/") self.assertEqual(span.kind, SpanKind.SERVER) self.assertEqual(span.status.status_code, StatusCode.ERROR) self.assertEqual(span.attributes[SpanAttributes.HTTP_METHOD], "GET") @@ -264,7 +264,7 @@ async def test_span_name(self): self.assertEqual(len(span_list), 1) span = span_list[0] - self.assertEqual(span.name, "^span_name/([0-9]{4})/$") + self.assertEqual(span.name, "GET ^span_name/([0-9]{4})/$") async def test_span_name_for_query_string(self): """ @@ -275,7 +275,7 @@ async def test_span_name_for_query_string(self): self.assertEqual(len(span_list), 1) span = span_list[0] - self.assertEqual(span.name, "^span_name/([0-9]{4})/$") + self.assertEqual(span.name, "GET ^span_name/([0-9]{4})/$") async def test_span_name_404(self): await self.async_client.get("/span_name/1234567890/") @@ -283,7 +283,7 @@ async def test_span_name_404(self): self.assertEqual(len(span_list), 1) span = span_list[0] - self.assertEqual(span.name, "HTTP GET") + self.assertEqual(span.name, "GET") async def test_traced_request_attrs(self): await self.async_client.get("/span_name/1234/", CONTENT_TYPE="test/ct") diff --git a/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py b/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py index aeba57a9b5..cf61a9fd3c 100644 --- a/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py +++ b/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py @@ -145,7 +145,7 @@ def test_404(self): spans = self.memory_exporter.get_finished_spans() self.assertEqual(len(spans), 1) span = spans[0] - self.assertEqual(span.name, "HTTP GET") + self.assertEqual(span.name, "GET /does-not-exist") self.assertEqual(span.status.status_code, StatusCode.UNSET) self.assertSpanHasAttributes( span, diff --git a/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py b/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py index 934c11e110..e99c8be6ed 100644 --- a/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py @@ -227,7 +227,7 @@ def instrument_app( app.add_middleware( OpenTelemetryMiddleware, excluded_urls=excluded_urls, - default_span_details=_get_route_details, + default_span_details=_get_default_span_details, server_request_hook=server_request_hook, client_request_hook=client_request_hook, client_response_hook=client_response_hook, @@ -300,7 +300,7 @@ def __init__(self, *args, **kwargs): self.add_middleware( OpenTelemetryMiddleware, excluded_urls=_InstrumentedFastAPI._excluded_urls, - default_span_details=_get_route_details, + default_span_details=_get_default_span_details, server_request_hook=_InstrumentedFastAPI._server_request_hook, client_request_hook=_InstrumentedFastAPI._client_request_hook, client_response_hook=_InstrumentedFastAPI._client_response_hook, @@ -316,15 +316,21 @@ def __del__(self): def _get_route_details(scope): - """Callback to retrieve the fastapi route being served. + """ + Function to retrieve Starlette route from scope. TODO: there is currently no way to retrieve http.route from a starlette application from scope. - See: https://github.com/encode/starlette/pull/804 + + Args: + scope: A Starlette scope + Returns: + A string containing the route or None """ app = scope["app"] route = None + for starlette_route in app.routes: match, _ = starlette_route.matches(scope) if match == Match.FULL: @@ -332,10 +338,27 @@ def _get_route_details(scope): break if match == Match.PARTIAL: route = starlette_route.path - # method only exists for http, if websocket - # leave it blank. - span_name = route or scope.get("method", "") + return route + + +def _get_default_span_details(scope): + """ + Callback to retrieve span name and attributes from scope. + + Args: + scope: A Starlette scope + Returns: + A tuple of span name and attributes + """ + route = _get_route_details(scope) + method = scope.get("method", "") attributes = {} if route: attributes[SpanAttributes.HTTP_ROUTE] = route + if method and route: # http + span_name = f"{method} {route}" + elif route: # websocket + span_name = route + else: # fallback + span_name = method return span_name, attributes diff --git a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py index 261b2e025f..9420ba2c0e 100644 --- a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py +++ b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py @@ -106,7 +106,7 @@ def test_instrument_app_with_instrument(self): spans = self.memory_exporter.get_finished_spans() self.assertEqual(len(spans), 3) for span in spans: - self.assertIn("/foobar", span.name) + self.assertIn("GET /foobar", span.name) def test_uninstrument_app(self): self._client.get("/foobar") @@ -138,7 +138,7 @@ def test_basic_fastapi_call(self): spans = self.memory_exporter.get_finished_spans() self.assertEqual(len(spans), 3) for span in spans: - self.assertIn("/foobar", span.name) + self.assertIn("GET /foobar", span.name) def test_fastapi_route_attribute_added(self): """Ensure that fastapi routes are used as the span name.""" @@ -146,7 +146,7 @@ def test_fastapi_route_attribute_added(self): spans = self.memory_exporter.get_finished_spans() self.assertEqual(len(spans), 3) for span in spans: - self.assertIn("/user/{username}", span.name) + self.assertIn("GET /user/{username}", span.name) self.assertEqual( spans[-1].attributes[SpanAttributes.HTTP_ROUTE], "/user/{username}" ) diff --git a/instrumentation/opentelemetry-instrumentation-flask/tests/test_programmatic.py b/instrumentation/opentelemetry-instrumentation-flask/tests/test_programmatic.py index 8c231b1d08..6393b927b8 100644 --- a/instrumentation/opentelemetry-instrumentation-flask/tests/test_programmatic.py +++ b/instrumentation/opentelemetry-instrumentation-flask/tests/test_programmatic.py @@ -214,7 +214,7 @@ def test_404(self): resp.close() span_list = self.memory_exporter.get_finished_spans() self.assertEqual(len(span_list), 1) - self.assertEqual(span_list[0].name, "HTTP POST") + self.assertEqual(span_list[0].name, "POST /bye") self.assertEqual(span_list[0].kind, trace.SpanKind.SERVER) self.assertEqual(span_list[0].attributes, expected_attrs) diff --git a/instrumentation/opentelemetry-instrumentation-httpx/src/opentelemetry/instrumentation/httpx/__init__.py b/instrumentation/opentelemetry-instrumentation-httpx/src/opentelemetry/instrumentation/httpx/__init__.py index 736e6c3d32..bb40adbc26 100644 --- a/instrumentation/opentelemetry-instrumentation-httpx/src/opentelemetry/instrumentation/httpx/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-httpx/src/opentelemetry/instrumentation/httpx/__init__.py @@ -209,7 +209,7 @@ class ResponseInfo(typing.NamedTuple): def _get_default_span_name(method: str) -> str: - return f"HTTP {method.strip()}" + return method.strip() def _apply_status_code(span: Span, status_code: int) -> None: diff --git a/instrumentation/opentelemetry-instrumentation-httpx/tests/test_httpx_integration.py b/instrumentation/opentelemetry-instrumentation-httpx/tests/test_httpx_integration.py index 86c9a56ab2..daddaad306 100644 --- a/instrumentation/opentelemetry-instrumentation-httpx/tests/test_httpx_integration.py +++ b/instrumentation/opentelemetry-instrumentation-httpx/tests/test_httpx_integration.py @@ -142,7 +142,7 @@ def test_basic(self): span = self.assert_span() self.assertIs(span.kind, trace.SpanKind.CLIENT) - self.assertEqual(span.name, "HTTP GET") + self.assertEqual(span.name, "GET") self.assertEqual( span.attributes, @@ -258,7 +258,7 @@ def test_invalid_url(self): span = self.assert_span() - self.assertEqual(span.name, "HTTP POST") + self.assertEqual(span.name, "POST") self.assertEqual( span.attributes[SpanAttributes.HTTP_METHOD], "POST" ) @@ -350,7 +350,7 @@ def test_request_hook_no_span_change(self): self.assertEqual(result.text, "Hello!") span = self.assert_span() - self.assertEqual(span.name, "HTTP GET") + self.assertEqual(span.name, "GET") def test_not_recording(self): with mock.patch("opentelemetry.trace.INVALID_SPAN") as mock_span: @@ -444,7 +444,7 @@ def test_request_hook_no_span_update(self): self.assertEqual(result.text, "Hello!") span = self.assert_span() - self.assertEqual(span.name, "HTTP GET") + self.assertEqual(span.name, "GET") HTTPXClientInstrumentor().uninstrument() def test_not_recording(self): diff --git a/instrumentation/opentelemetry-instrumentation-pyramid/tests/test_programmatic.py b/instrumentation/opentelemetry-instrumentation-pyramid/tests/test_programmatic.py index d3a4fa91db..478eab1937 100644 --- a/instrumentation/opentelemetry-instrumentation-pyramid/tests/test_programmatic.py +++ b/instrumentation/opentelemetry-instrumentation-pyramid/tests/test_programmatic.py @@ -145,7 +145,7 @@ def test_404(self): resp.close() span_list = self.memory_exporter.get_finished_spans() self.assertEqual(len(span_list), 1) - self.assertEqual(span_list[0].name, "HTTP POST") + self.assertEqual(span_list[0].name, "POST /bye") self.assertEqual(span_list[0].kind, trace.SpanKind.SERVER) self.assertEqual(span_list[0].attributes, expected_attrs) diff --git a/instrumentation/opentelemetry-instrumentation-requests/src/opentelemetry/instrumentation/requests/__init__.py b/instrumentation/opentelemetry-instrumentation-requests/src/opentelemetry/instrumentation/requests/__init__.py index e5bb24223c..c3dabf05a5 100644 --- a/instrumentation/opentelemetry-instrumentation-requests/src/opentelemetry/instrumentation/requests/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-requests/src/opentelemetry/instrumentation/requests/__init__.py @@ -245,8 +245,16 @@ def _uninstrument_from(instr_root, restore_as_bound_func=False): def get_default_span_name(method): - """Default implementation for name_callback, returns HTTP {method_name}.""" - return f"HTTP {method.strip()}" + """ + Default implementation for name_callback, returns HTTP {method_name}. + https://opentelemetry.io/docs/reference/specification/trace/semantic_conventions/http/#name + + Args: + method: string representing HTTP method + Returns: + span name + """ + return method.strip() class RequestsInstrumentor(BaseInstrumentor): diff --git a/instrumentation/opentelemetry-instrumentation-requests/tests/test_requests_integration.py b/instrumentation/opentelemetry-instrumentation-requests/tests/test_requests_integration.py index 6e6a68cb8f..3bd76a6995 100644 --- a/instrumentation/opentelemetry-instrumentation-requests/tests/test_requests_integration.py +++ b/instrumentation/opentelemetry-instrumentation-requests/tests/test_requests_integration.py @@ -116,7 +116,7 @@ def test_basic(self): span = self.assert_span() self.assertIs(span.kind, trace.SpanKind.CLIENT) - self.assertEqual(span.name, "HTTP GET") + self.assertEqual(span.name, "GET") self.assertEqual( span.attributes, @@ -191,7 +191,7 @@ def name_callback(method, url): self.assertEqual(result.text, "Hello!") span = self.assert_span() - self.assertEqual(span.name, "HTTP GET") + self.assertEqual(span.name, "GET") def test_not_foundbasic(self): url_404 = "http://mock/status/404" diff --git a/instrumentation/opentelemetry-instrumentation-requests/tests/test_requests_ip_support.py b/instrumentation/opentelemetry-instrumentation-requests/tests/test_requests_ip_support.py index 593ed92fe9..cf2e7fb4dd 100644 --- a/instrumentation/opentelemetry-instrumentation-requests/tests/test_requests_ip_support.py +++ b/instrumentation/opentelemetry-instrumentation-requests/tests/test_requests_ip_support.py @@ -71,7 +71,7 @@ def assert_success_span(self, response: requests.Response): span = self.assert_span() self.assertIs(trace.SpanKind.CLIENT, span.kind) - self.assertEqual("HTTP GET", span.name) + self.assertEqual("GET", span.name) attributes = { "http.status_code": 200, diff --git a/instrumentation/opentelemetry-instrumentation-starlette/src/opentelemetry/instrumentation/starlette/__init__.py b/instrumentation/opentelemetry-instrumentation-starlette/src/opentelemetry/instrumentation/starlette/__init__.py index 30f24cc65c..2d123aa70e 100644 --- a/instrumentation/opentelemetry-instrumentation-starlette/src/opentelemetry/instrumentation/starlette/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-starlette/src/opentelemetry/instrumentation/starlette/__init__.py @@ -212,7 +212,7 @@ def instrument_app( app.add_middleware( OpenTelemetryMiddleware, excluded_urls=_excluded_urls, - default_span_details=_get_route_details, + default_span_details=_get_default_span_details, server_request_hook=server_request_hook, client_request_hook=client_request_hook, client_response_hook=client_response_hook, @@ -278,7 +278,7 @@ def __init__(self, *args, **kwargs): self.add_middleware( OpenTelemetryMiddleware, excluded_urls=_excluded_urls, - default_span_details=_get_route_details, + default_span_details=_get_default_span_details, server_request_hook=_InstrumentedStarlette._server_request_hook, client_request_hook=_InstrumentedStarlette._client_request_hook, client_response_hook=_InstrumentedStarlette._client_response_hook, @@ -294,15 +294,21 @@ def __del__(self): def _get_route_details(scope): - """Callback to retrieve the starlette route being served. + """ + Function to retrieve Starlette route from scope. TODO: there is currently no way to retrieve http.route from a starlette application from scope. - See: https://github.com/encode/starlette/pull/804 + + Args: + scope: A Starlette scope + Returns: + A string containing the route or None """ app = scope["app"] route = None + for starlette_route in app.routes: match, _ = starlette_route.matches(scope) if match == Match.FULL: @@ -310,10 +316,27 @@ def _get_route_details(scope): break if match == Match.PARTIAL: route = starlette_route.path - # method only exists for http, if websocket - # leave it blank. - span_name = route or scope.get("method", "") + return route + + +def _get_default_span_details(scope): + """ + Callback to retrieve span name and attributes from scope. + + Args: + scope: A Starlette scope + Returns: + A tuple of span name and attributes + """ + route = _get_route_details(scope) + method = scope.get("method", "") attributes = {} if route: attributes[SpanAttributes.HTTP_ROUTE] = route + if method and route: # http + span_name = f"{method} {route}" + elif route: # websocket + span_name = route + else: # fallback + span_name = method return span_name, attributes diff --git a/instrumentation/opentelemetry-instrumentation-starlette/tests/test_starlette_instrumentation.py b/instrumentation/opentelemetry-instrumentation-starlette/tests/test_starlette_instrumentation.py index 9c658e0092..1f4570d293 100644 --- a/instrumentation/opentelemetry-instrumentation-starlette/tests/test_starlette_instrumentation.py +++ b/instrumentation/opentelemetry-instrumentation-starlette/tests/test_starlette_instrumentation.py @@ -93,7 +93,7 @@ def test_basic_starlette_call(self): spans = self.memory_exporter.get_finished_spans() self.assertEqual(len(spans), 3) for span in spans: - self.assertIn("/foobar", span.name) + self.assertIn("GET /foobar", span.name) def test_starlette_route_attribute_added(self): """Ensure that starlette routes are used as the span name.""" @@ -101,7 +101,7 @@ def test_starlette_route_attribute_added(self): spans = self.memory_exporter.get_finished_spans() self.assertEqual(len(spans), 3) for span in spans: - self.assertIn("/user/{username}", span.name) + self.assertIn("GET /user/{username}", span.name) self.assertEqual( spans[-1].attributes[SpanAttributes.HTTP_ROUTE], "/user/{username}" ) diff --git a/instrumentation/opentelemetry-instrumentation-tornado/src/opentelemetry/instrumentation/tornado/__init__.py b/instrumentation/opentelemetry-instrumentation-tornado/src/opentelemetry/instrumentation/tornado/__init__.py index dd8da74f3f..1e2f0e5162 100644 --- a/instrumentation/opentelemetry-instrumentation-tornado/src/opentelemetry/instrumentation/tornado/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-tornado/src/opentelemetry/instrumentation/tornado/__init__.py @@ -454,10 +454,23 @@ def _get_attributes_from_request(request): ) -def _get_operation_name(handler, request): - full_class_name = type(handler).__name__ - class_name = full_class_name.rsplit(".")[-1] - return f"{class_name}.{request.method.lower()}" +def _get_default_span_name(request): + """ + Default span name is the HTTP method and URL path, or just the method. + https://github.com/open-telemetry/opentelemetry-specification/pull/3165 + https://opentelemetry.io/docs/reference/specification/trace/semantic_conventions/http/#name + + Args: + request: Tornado request object. + Returns: + Default span name. + """ + + path = request.path + method = request.method + if method and path: + return f"{method} {path}" + return f"{method}" def _get_full_handler_name(handler): @@ -468,7 +481,7 @@ def _get_full_handler_name(handler): def _start_span(tracer, handler) -> _TraceContext: span, token = _start_internal_or_server_span( tracer=tracer, - span_name=_get_operation_name(handler, handler.request), + span_name=_get_default_span_name(handler.request), start_time=time_ns(), context_carrier=handler.request.headers, context_getter=textmap.default_getter, diff --git a/instrumentation/opentelemetry-instrumentation-tornado/tests/test_instrumentation.py b/instrumentation/opentelemetry-instrumentation-tornado/tests/test_instrumentation.py index c875a331ef..0baaa348ab 100644 --- a/instrumentation/opentelemetry-instrumentation-tornado/tests/test_instrumentation.py +++ b/instrumentation/opentelemetry-instrumentation-tornado/tests/test_instrumentation.py @@ -136,7 +136,7 @@ def _test_http_method_call(self, method): self.assertEqual(manual.parent, server.context) self.assertEqual(manual.context.trace_id, client.context.trace_id) - self.assertEqual(server.name, "MainHandler." + method.lower()) + self.assertEqual(server.name, f"{method} /") self.assertTrue(server.parent.is_remote) self.assertNotEqual(server.parent, client.context) self.assertEqual(server.parent.span_id, client.context.span_id) @@ -197,7 +197,7 @@ def _test_async_handler(self, url, handler_name): self.assertEqual(len(spans), 5) client = spans.by_name("GET") - server = spans.by_name(handler_name + ".get") + server = spans.by_name(f"GET {url}") sub_wrapper = spans.by_name("sub-task-wrapper") sub2 = spans.by_name("sub-task-2") @@ -214,7 +214,7 @@ def _test_async_handler(self, url, handler_name): self.assertEqual(sub_wrapper.parent, server.context) self.assertEqual(sub_wrapper.context.trace_id, client.context.trace_id) - self.assertEqual(server.name, handler_name + ".get") + self.assertEqual(server.name, f"GET {url}") self.assertTrue(server.parent.is_remote) self.assertNotEqual(server.parent, client.context) self.assertEqual(server.parent.span_id, client.context.span_id) @@ -230,6 +230,7 @@ def _test_async_handler(self, url, handler_name): SpanAttributes.HTTP_TARGET: url, SpanAttributes.HTTP_CLIENT_IP: "127.0.0.1", SpanAttributes.HTTP_STATUS_CODE: 201, + "tornado.handler": f"tests.tornado_test_app.{handler_name}", }, ) @@ -254,9 +255,9 @@ def test_500(self): self.assertEqual(len(spans), 2) client = spans.by_name("GET") - server = spans.by_name("BadHandler.get") + server = spans.by_name("GET /error") - self.assertEqual(server.name, "BadHandler.get") + self.assertEqual(server.name, "GET /error") self.assertEqual(server.kind, SpanKind.SERVER) self.assertSpanHasAttributes( server, @@ -291,7 +292,7 @@ def test_404(self): self.assertEqual(len(spans), 2) server, client = spans - self.assertEqual(server.name, "ErrorHandler.get") + self.assertEqual(server.name, "GET /missing-url") self.assertEqual(server.kind, SpanKind.SERVER) self.assertSpanHasAttributes( server, @@ -326,7 +327,7 @@ def test_http_error(self): self.assertEqual(len(spans), 2) server, client = spans - self.assertEqual(server.name, "RaiseHTTPErrorHandler.get") + self.assertEqual(server.name, "GET /raise_403") self.assertEqual(server.kind, SpanKind.SERVER) self.assertSpanHasAttributes( server, @@ -367,7 +368,7 @@ def test_dynamic_handler(self): self.assertEqual(len(spans), 2) server, client = spans - self.assertEqual(server.name, "DynamicHandler.get") + self.assertEqual(server.name, "GET /dyna") self.assertTrue(server.parent.is_remote) self.assertNotEqual(server.parent, client.context) self.assertEqual(server.parent.span_id, client.context.span_id) @@ -408,7 +409,7 @@ def test_handler_on_finish(self): self.assertEqual(len(spans), 3) auditor, server, client = spans - self.assertEqual(server.name, "FinishedHandler.get") + self.assertEqual(server.name, "GET /on_finish") self.assertTrue(server.parent.is_remote) self.assertNotEqual(server.parent, client.context) self.assertEqual(server.parent.span_id, client.context.span_id) @@ -535,7 +536,7 @@ def test_xheaders(self): self.assertEqual(response.code, 201) spans = self.get_finished_spans() self.assertSpanHasAttributes( - spans.by_name("MainHandler.get"), + spans.by_name("GET /"), { SpanAttributes.HTTP_METHOD: "GET", SpanAttributes.HTTP_SCHEME: "http", @@ -609,7 +610,7 @@ def test_uninstrument(self): self.assertEqual(len(spans), 3) manual, server, client = self.sorted_spans(spans) self.assertEqual(manual.name, "manual") - self.assertEqual(server.name, "MainHandler.get") + self.assertEqual(server.name, "GET /") self.assertEqual(client.name, "GET") self.memory_exporter.clear() diff --git a/instrumentation/opentelemetry-instrumentation-urllib/src/opentelemetry/instrumentation/urllib/__init__.py b/instrumentation/opentelemetry-instrumentation-urllib/src/opentelemetry/instrumentation/urllib/__init__.py index 091ccf99b1..cdd35a0bad 100644 --- a/instrumentation/opentelemetry-instrumentation-urllib/src/opentelemetry/instrumentation/urllib/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-urllib/src/opentelemetry/instrumentation/urllib/__init__.py @@ -207,7 +207,7 @@ def _instrumented_open_call( method = request.get_method().upper() - span_name = f"HTTP {method}".strip() + span_name = method.strip() url = remove_url_credentials(url) diff --git a/instrumentation/opentelemetry-instrumentation-urllib/tests/test_urllib_integration.py b/instrumentation/opentelemetry-instrumentation-urllib/tests/test_urllib_integration.py index 35e9e1f1c7..f27f594a30 100644 --- a/instrumentation/opentelemetry-instrumentation-urllib/tests/test_urllib_integration.py +++ b/instrumentation/opentelemetry-instrumentation-urllib/tests/test_urllib_integration.py @@ -124,7 +124,7 @@ def test_basic(self): span = self.assert_span() self.assertIs(span.kind, trace.SpanKind.CLIENT) - self.assertEqual(span.name, "HTTP GET") + self.assertEqual(span.name, "GET") self.assertEqual( span.attributes, @@ -209,7 +209,7 @@ def test_response_code_none(self): span = self.assert_span() self.assertIs(span.kind, trace.SpanKind.CLIENT) - self.assertEqual(span.name, "HTTP GET") + self.assertEqual(span.name, "GET") self.assertEqual( span.attributes, diff --git a/instrumentation/opentelemetry-instrumentation-urllib3/src/opentelemetry/instrumentation/urllib3/__init__.py b/instrumentation/opentelemetry-instrumentation-urllib3/src/opentelemetry/instrumentation/urllib3/__init__.py index 91d5576fc0..809f9b595f 100644 --- a/instrumentation/opentelemetry-instrumentation-urllib3/src/opentelemetry/instrumentation/urllib3/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-urllib3/src/opentelemetry/instrumentation/urllib3/__init__.py @@ -225,7 +225,7 @@ def instrumented_urlopen(wrapped, instance, args, kwargs): headers = _prepare_headers(kwargs) body = _get_url_open_arg("body", args, kwargs) - span_name = f"HTTP {method.strip()}" + span_name = method.strip() span_attributes = { SpanAttributes.HTTP_METHOD: method, SpanAttributes.HTTP_URL: url, diff --git a/instrumentation/opentelemetry-instrumentation-urllib3/tests/test_urllib3_integration.py b/instrumentation/opentelemetry-instrumentation-urllib3/tests/test_urllib3_integration.py index 315cec1112..1082776f9a 100644 --- a/instrumentation/opentelemetry-instrumentation-urllib3/tests/test_urllib3_integration.py +++ b/instrumentation/opentelemetry-instrumentation-urllib3/tests/test_urllib3_integration.py @@ -87,7 +87,7 @@ def assert_success_span( span = self.assert_span() self.assertIs(trace.SpanKind.CLIENT, span.kind) - self.assertEqual("HTTP GET", span.name) + self.assertEqual("GET", span.name) attributes = { SpanAttributes.HTTP_METHOD: "GET", diff --git a/instrumentation/opentelemetry-instrumentation-urllib3/tests/test_urllib3_ip_support.py b/instrumentation/opentelemetry-instrumentation-urllib3/tests/test_urllib3_ip_support.py index 5baddee516..1199ad3d5b 100644 --- a/instrumentation/opentelemetry-instrumentation-urllib3/tests/test_urllib3_ip_support.py +++ b/instrumentation/opentelemetry-instrumentation-urllib3/tests/test_urllib3_ip_support.py @@ -77,7 +77,7 @@ def assert_success_span( span = self.assert_span() self.assertIs(trace.SpanKind.CLIENT, span.kind) - self.assertEqual("HTTP GET", span.name) + self.assertEqual("GET", span.name) attributes = { "http.status_code": 200, diff --git a/instrumentation/opentelemetry-instrumentation-wsgi/src/opentelemetry/instrumentation/wsgi/__init__.py b/instrumentation/opentelemetry-instrumentation-wsgi/src/opentelemetry/instrumentation/wsgi/__init__.py index b4d53f9a8b..f4012d7904 100644 --- a/instrumentation/opentelemetry-instrumentation-wsgi/src/opentelemetry/instrumentation/wsgi/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-wsgi/src/opentelemetry/instrumentation/wsgi/__init__.py @@ -440,8 +440,21 @@ def add_response_attributes( def get_default_span_name(environ): - """Default implementation for name_callback, returns HTTP {METHOD_NAME}.""" - return f"HTTP {environ.get('REQUEST_METHOD', '')}".strip() + """ + Default span name is the HTTP method and URL path, or just the method. + https://github.com/open-telemetry/opentelemetry-specification/pull/3165 + https://opentelemetry.io/docs/reference/specification/trace/semantic_conventions/http/#name + + Args: + environ: The WSGI environ object. + Returns: + The span name. + """ + method = environ.get("REQUEST_METHOD", "").strip() + path = environ.get("PATH_INFO", "").strip() + if method and path: + return f"{method} {path}" + return method class OpenTelemetryMiddleware: diff --git a/instrumentation/opentelemetry-instrumentation-wsgi/tests/test_wsgi_middleware.py b/instrumentation/opentelemetry-instrumentation-wsgi/tests/test_wsgi_middleware.py index 926124caf3..c2aaf3820d 100644 --- a/instrumentation/opentelemetry-instrumentation-wsgi/tests/test_wsgi_middleware.py +++ b/instrumentation/opentelemetry-instrumentation-wsgi/tests/test_wsgi_middleware.py @@ -128,7 +128,7 @@ def validate_response( self, response, error=None, - span_name="HTTP GET", + span_name="GET /", http_method="GET", span_attributes=None, response_headers=None, @@ -284,12 +284,13 @@ def test_wsgi_metrics(self): ) self.assertTrue(number_data_point_seen and histogram_data_point_seen) - def test_default_span_name_missing_request_method(self): - """Test that default span_names with missing request method.""" - self.environ.pop("REQUEST_METHOD") + def test_default_span_name_missing_path_info(self): + """Test that default span_names with missing path info.""" + self.environ.pop("PATH_INFO") + method = self.environ.get("REQUEST_METHOD", "").strip() app = otel_wsgi.OpenTelemetryMiddleware(simple_wsgi) response = app(self.environ, self.start_response) - self.validate_response(response, span_name="HTTP", http_method=None) + self.validate_response(response, span_name=method) class TestWsgiAttributes(unittest.TestCase): @@ -455,7 +456,7 @@ def validate_response( response, exporter, error=None, - span_name="HTTP GET", + span_name="GET /", http_method="GET", ): while True: diff --git a/opentelemetry-instrumentation/src/opentelemetry/instrumentation/utils.py b/opentelemetry-instrumentation/src/opentelemetry/instrumentation/utils.py index 3022e6ddd0..35a55a1279 100644 --- a/opentelemetry-instrumentation/src/opentelemetry/instrumentation/utils.py +++ b/opentelemetry-instrumentation/src/opentelemetry/instrumentation/utils.py @@ -95,7 +95,7 @@ def _start_internal_or_server_span( Args: tracer : tracer in use by given instrumentation library - name (string): name of the span + span_name (string): name of the span start_time : start time of the span context_carrier : object which contains values that are used to construct a Context. This object From f9f7b014160a376360f254184159abf3f1b5b01b Mon Sep 17 00:00:00 2001 From: Filip Nikolovski Date: Fri, 16 Jun 2023 18:18:08 +0200 Subject: [PATCH 10/29] Fix falcon usage of Span Status (#1840) * Fix falcon usage of Span Status to only set the description if the status code is ERROR * Update changelog * Update CHANGELOG.md Co-authored-by: Srikanth Chekuri * fix lint * Use fewer variables to satisfy R0914 lint rule --------- Co-authored-by: Srikanth Chekuri --- CHANGELOG.md | 6 ++++-- .../instrumentation/falcon/__init__.py | 14 ++++++++++---- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9959a49d01..cde00bb9ab 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## Unreleased +- Fix falcon instrumentation's usage of Span Status to only set the description if the status code is ERROR. + ([#1840](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1840)) - Instrument all httpx versions >= 0.18. ([#1748](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1748)) ## Version 1.18.0/0.39b0 (2023-05-10) @@ -23,9 +25,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ([#1778](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1778)) - Add `excluded_urls` functionality to `urllib` and `urllib3` instrumentations ([#1733](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1733)) -- Make Django request span attributes available for `start_span`. +- Make Django request span attributes available for `start_span`. ([#1730](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1730)) -- Make ASGI request span attributes available for `start_span`. +- Make ASGI request span attributes available for `start_span`. ([#1762](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1762)) - `opentelemetry-instrumentation-celery` Add support for anonymous tasks. ([#1407](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1407)) diff --git a/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py b/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py index 3a6a86e4fb..73c005fa17 100644 --- a/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py @@ -208,7 +208,7 @@ def response_hook(span, req, resp): from opentelemetry.metrics import get_meter from opentelemetry.semconv.metrics import MetricInstruments from opentelemetry.semconv.trace import SpanAttributes -from opentelemetry.trace.status import Status +from opentelemetry.trace.status import Status, StatusCode from opentelemetry.util.http import get_excluded_urls, get_traced_request_attrs _logger = getLogger(__name__) @@ -461,11 +461,17 @@ def process_response( try: status_code = int(status) span.set_attribute(SpanAttributes.HTTP_STATUS_CODE, status_code) + otel_status_code = http_status_to_status_code( + status_code, server_span=True + ) + + # set the description only when the status code is ERROR + if otel_status_code is not StatusCode.ERROR: + reason = None + span.set_status( Status( - status_code=http_status_to_status_code( - status_code, server_span=True - ), + status_code=otel_status_code, description=reason, ) ) From 7292beefae6ee08c886ca3a8728e8cfe71b43fb8 Mon Sep 17 00:00:00 2001 From: Tammy Baylis <96076570+tammy-baylis-swi@users.noreply.github.com> Date: Fri, 16 Jun 2023 15:40:46 -0700 Subject: [PATCH 11/29] Request Flask attributes passed to Sampler (#1784) * Request Flask attributes passed to Sampler * Update changelog * Lint * Fix botocore test keyerror * Revert "Fix botocore test keyerror" This reverts commit fd03c55a3902b3456afd6a2ecf429afba11b0691. * botocore test does get_queue_url * Revert "botocore test does get_queue_url" This reverts commit 9530cd250dd836b3181a9361decb130e2aae1202. * Update changelog --------- Co-authored-by: Srikanth Chekuri Co-authored-by: Shalev Roda <65566801+shalevr@users.noreply.github.com> --- CHANGELOG.md | 4 ++++ .../instrumentation/flask/__init__.py | 17 ++++++++--------- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cde00bb9ab..70ba0033de 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## Unreleased +### Added + +- Make Flask request span attributes available for `start_span`. + ([#1784](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1784)) - Fix falcon instrumentation's usage of Span Status to only set the description if the status code is ERROR. ([#1840](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1840)) - Instrument all httpx versions >= 0.18. ([#1748](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1748)) diff --git a/instrumentation/opentelemetry-instrumentation-flask/src/opentelemetry/instrumentation/flask/__init__.py b/instrumentation/opentelemetry-instrumentation-flask/src/opentelemetry/instrumentation/flask/__init__.py index 73c2f4fe2d..432c6b1fbf 100644 --- a/instrumentation/opentelemetry-instrumentation-flask/src/opentelemetry/instrumentation/flask/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-flask/src/opentelemetry/instrumentation/flask/__init__.py @@ -375,27 +375,26 @@ def _before_request(): flask_request_environ = flask.request.environ span_name = get_default_span_name() + attributes = otel_wsgi.collect_request_attributes( + flask_request_environ + ) + if flask.request.url_rule: + # For 404 that result from no route found, etc, we + # don't have a url_rule. + attributes[SpanAttributes.HTTP_ROUTE] = flask.request.url_rule.rule span, token = _start_internal_or_server_span( tracer=tracer, span_name=span_name, start_time=flask_request_environ.get(_ENVIRON_STARTTIME_KEY), context_carrier=flask_request_environ, context_getter=otel_wsgi.wsgi_getter, + attributes=attributes, ) if request_hook: request_hook(span, flask_request_environ) if span.is_recording(): - attributes = otel_wsgi.collect_request_attributes( - flask_request_environ - ) - if flask.request.url_rule: - # For 404 that result from no route found, etc, we - # don't have a url_rule. - attributes[ - SpanAttributes.HTTP_ROUTE - ] = flask.request.url_rule.rule for key, value in attributes.items(): span.set_attribute(key, value) if span.is_recording() and span.kind == trace.SpanKind.SERVER: From a3a0b2409cc1fe9a03bb73da9c8bbf47b9d3d5ea Mon Sep 17 00:00:00 2001 From: Yashaswi Makula Date: Sat, 17 Jun 2023 04:38:51 +0530 Subject: [PATCH 12/29] Fixed urllib3 instrumentation example in instrumentation documentation (#1793) * corrected instrumentation example in urllib3 * Remove changelog entry --------- Co-authored-by: Shalev Roda <65566801+shalevr@users.noreply.github.com> Co-authored-by: Diego Hurtado --- .../opentelemetry-instrumentation-urllib3/README.rst | 4 ++-- .../src/opentelemetry/instrumentation/urllib3/__init__.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-urllib3/README.rst b/instrumentation/opentelemetry-instrumentation-urllib3/README.rst index 0c53c299a0..4e0089e21e 100644 --- a/instrumentation/opentelemetry-instrumentation-urllib3/README.rst +++ b/instrumentation/opentelemetry-instrumentation-urllib3/README.rst @@ -38,8 +38,8 @@ The hooks can be configured as follows: def response_hook(span, request, response): pass - URLLib3Instrumentor.instrument( - request_hook=request_hook, response_hook=response_hook) + URLLib3Instrumentor().instrument( + request_hook=request_hook, response_hook=response_hook ) Exclude lists diff --git a/instrumentation/opentelemetry-instrumentation-urllib3/src/opentelemetry/instrumentation/urllib3/__init__.py b/instrumentation/opentelemetry-instrumentation-urllib3/src/opentelemetry/instrumentation/urllib3/__init__.py index 809f9b595f..d3016ea5ee 100644 --- a/instrumentation/opentelemetry-instrumentation-urllib3/src/opentelemetry/instrumentation/urllib3/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-urllib3/src/opentelemetry/instrumentation/urllib3/__init__.py @@ -56,8 +56,8 @@ def request_hook(span, request): def response_hook(span, request, response): pass - URLLib3Instrumentor.instrument( - request_hook=request_hook, response_hook=response_hook) + URLLib3Instrumentor().instrument( + request_hook=request_hook, response_hook=response_hook ) Exclude lists From 78040836d2d401c9a0ba5013e73140b768fb58ee Mon Sep 17 00:00:00 2001 From: Iman Shafiei Date: Fri, 16 Jun 2023 16:55:15 -0700 Subject: [PATCH 13/29] Fix Invalid type NoneType for attribute X error | AWS-Lambda instrumentation (#1785) * Add None checking to the aws-lambda logic * Update changelog. * Change .get() check to 'key' in dict check. * Fix consistency issues. * Update changelog. --------- Co-authored-by: Srikanth Chekuri Co-authored-by: Shalev Roda <65566801+shalevr@users.noreply.github.com> --- CHANGELOG.md | 2 + .../instrumentation/aws_lambda/__init__.py | 94 ++++++++++--------- 2 files changed, 53 insertions(+), 43 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 70ba0033de..29cecbd899 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Fix falcon instrumentation's usage of Span Status to only set the description if the status code is ERROR. ([#1840](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1840)) - Instrument all httpx versions >= 0.18. ([#1748](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1748)) +- Fix `Invalid type NoneType for attribute X (opentelemetry-instrumentation-aws-lambda)` error when some attributes do not exist + ([#1780](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1780)) ## Version 1.18.0/0.39b0 (2023-05-10) diff --git a/instrumentation/opentelemetry-instrumentation-aws-lambda/src/opentelemetry/instrumentation/aws_lambda/__init__.py b/instrumentation/opentelemetry-instrumentation-aws-lambda/src/opentelemetry/instrumentation/aws_lambda/__init__.py index 4404839c66..799becbdcc 100644 --- a/instrumentation/opentelemetry-instrumentation-aws-lambda/src/opentelemetry/instrumentation/aws_lambda/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-aws-lambda/src/opentelemetry/instrumentation/aws_lambda/__init__.py @@ -201,30 +201,35 @@ def _set_api_gateway_v1_proxy_attributes( span.set_attribute( SpanAttributes.HTTP_METHOD, lambda_event.get("httpMethod") ) - span.set_attribute(SpanAttributes.HTTP_ROUTE, lambda_event.get("resource")) if lambda_event.get("headers"): - span.set_attribute( - SpanAttributes.HTTP_USER_AGENT, - lambda_event["headers"].get("User-Agent"), - ) - span.set_attribute( - SpanAttributes.HTTP_SCHEME, - lambda_event["headers"].get("X-Forwarded-Proto"), - ) - span.set_attribute( - SpanAttributes.NET_HOST_NAME, lambda_event["headers"].get("Host") - ) + if "User-Agent" in lambda_event["headers"]: + span.set_attribute( + SpanAttributes.HTTP_USER_AGENT, + lambda_event["headers"]["User-Agent"], + ) + if "X-Forwarded-Proto" in lambda_event["headers"]: + span.set_attribute( + SpanAttributes.HTTP_SCHEME, + lambda_event["headers"]["X-Forwarded-Proto"], + ) + if "Host" in lambda_event["headers"]: + span.set_attribute( + SpanAttributes.NET_HOST_NAME, + lambda_event["headers"]["Host"], + ) + if "resource" in lambda_event: + span.set_attribute(SpanAttributes.HTTP_ROUTE, lambda_event["resource"]) - if lambda_event.get("queryStringParameters"): - span.set_attribute( - SpanAttributes.HTTP_TARGET, - f"{lambda_event.get('resource')}?{urlencode(lambda_event.get('queryStringParameters'))}", - ) - else: - span.set_attribute( - SpanAttributes.HTTP_TARGET, lambda_event.get("resource") - ) + if lambda_event.get("queryStringParameters"): + span.set_attribute( + SpanAttributes.HTTP_TARGET, + f"{lambda_event['resource']}?{urlencode(lambda_event['queryStringParameters'])}", + ) + else: + span.set_attribute( + SpanAttributes.HTTP_TARGET, lambda_event["resource"] + ) return span @@ -237,35 +242,38 @@ def _set_api_gateway_v2_proxy_attributes( More info: https://docs.aws.amazon.com/apigateway/latest/developerguide/http-api-develop-integrations-lambda.html """ - span.set_attribute( - SpanAttributes.NET_HOST_NAME, - lambda_event["requestContext"].get("domainName"), - ) - - if lambda_event["requestContext"].get("http"): + if "domainName" in lambda_event["requestContext"]: span.set_attribute( - SpanAttributes.HTTP_METHOD, - lambda_event["requestContext"]["http"].get("method"), - ) - span.set_attribute( - SpanAttributes.HTTP_USER_AGENT, - lambda_event["requestContext"]["http"].get("userAgent"), - ) - span.set_attribute( - SpanAttributes.HTTP_ROUTE, - lambda_event["requestContext"]["http"].get("path"), + SpanAttributes.NET_HOST_NAME, + lambda_event["requestContext"]["domainName"], ) - if lambda_event.get("rawQueryString"): + if lambda_event["requestContext"].get("http"): + if "method" in lambda_event["requestContext"]["http"]: span.set_attribute( - SpanAttributes.HTTP_TARGET, - f"{lambda_event['requestContext']['http'].get('path')}?{lambda_event.get('rawQueryString')}", + SpanAttributes.HTTP_METHOD, + lambda_event["requestContext"]["http"]["method"], ) - else: + if "userAgent" in lambda_event["requestContext"]["http"]: span.set_attribute( - SpanAttributes.HTTP_TARGET, - lambda_event["requestContext"]["http"].get("path"), + SpanAttributes.HTTP_USER_AGENT, + lambda_event["requestContext"]["http"]["userAgent"], + ) + if "path" in lambda_event["requestContext"]["http"]: + span.set_attribute( + SpanAttributes.HTTP_ROUTE, + lambda_event["requestContext"]["http"]["path"], ) + if lambda_event.get("rawQueryString"): + span.set_attribute( + SpanAttributes.HTTP_TARGET, + f"{lambda_event['requestContext']['http']['path']}?{lambda_event['rawQueryString']}", + ) + else: + span.set_attribute( + SpanAttributes.HTTP_TARGET, + lambda_event["requestContext"]["http"]["path"], + ) return span From 1dd17edeeab4cb45477755fb2ef0eef23fbd8289 Mon Sep 17 00:00:00 2001 From: Akochavi <121871419+Akochavi@users.noreply.github.com> Date: Sun, 18 Jun 2023 13:45:00 +0300 Subject: [PATCH 14/29] Add metrics instrumentation celery (#1679) Co-authored-by: Shalev Roda <65566801+shalevr@users.noreply.github.com> --- CHANGELOG.md | 3 + .../instrumentation/celery/__init__.py | 46 ++++++++++- .../tests/test_metrics.py | 76 +++++++++++++++++++ 3 files changed, 123 insertions(+), 2 deletions(-) create mode 100644 instrumentation/opentelemetry-instrumentation-celery/tests/test_metrics.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 29cecbd899..b591e9a143 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## Unreleased + ### Added - Make Flask request span attributes available for `start_span`. @@ -16,6 +17,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Instrument all httpx versions >= 0.18. ([#1748](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1748)) - Fix `Invalid type NoneType for attribute X (opentelemetry-instrumentation-aws-lambda)` error when some attributes do not exist ([#1780](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1780)) +- Add metric instrumentation for celery + ([#1679](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1679)) ## Version 1.18.0/0.39b0 (2023-05-10) diff --git a/instrumentation/opentelemetry-instrumentation-celery/src/opentelemetry/instrumentation/celery/__init__.py b/instrumentation/opentelemetry-instrumentation-celery/src/opentelemetry/instrumentation/celery/__init__.py index cb265b46f8..a216765fb0 100644 --- a/instrumentation/opentelemetry-instrumentation-celery/src/opentelemetry/instrumentation/celery/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-celery/src/opentelemetry/instrumentation/celery/__init__.py @@ -60,6 +60,7 @@ def add(x, y): """ import logging +from timeit import default_timer from typing import Collection, Iterable from celery import signals # pylint: disable=no-name-in-module @@ -69,6 +70,7 @@ def add(x, y): from opentelemetry.instrumentation.celery.package import _instruments from opentelemetry.instrumentation.celery.version import __version__ from opentelemetry.instrumentation.instrumentor import BaseInstrumentor +from opentelemetry.metrics import get_meter from opentelemetry.propagate import extract, inject from opentelemetry.propagators.textmap import Getter from opentelemetry.semconv.trace import SpanAttributes @@ -104,6 +106,11 @@ def keys(self, carrier): class CeleryInstrumentor(BaseInstrumentor): + def __init__(self): + super().__init__() + self.metrics = None + self.task_id_to_start_time = {} + def instrumentation_dependencies(self) -> Collection[str]: return _instruments @@ -113,6 +120,11 @@ def _instrument(self, **kwargs): # pylint: disable=attribute-defined-outside-init self._tracer = trace.get_tracer(__name__, __version__, tracer_provider) + meter_provider = kwargs.get("meter_provider") + meter = get_meter(__name__, __version__, meter_provider) + + self.create_celery_metrics(meter) + signals.task_prerun.connect(self._trace_prerun, weak=False) signals.task_postrun.connect(self._trace_postrun, weak=False) signals.before_task_publish.connect( @@ -139,6 +151,7 @@ def _trace_prerun(self, *args, **kwargs): if task is None or task_id is None: return + self.update_task_duration_time(task_id) request = task.request tracectx = extract(request, getter=celery_getter) or None @@ -153,8 +166,7 @@ def _trace_prerun(self, *args, **kwargs): activation.__enter__() # pylint: disable=E1101 utils.attach_span(task, task_id, (span, activation)) - @staticmethod - def _trace_postrun(*args, **kwargs): + def _trace_postrun(self, *args, **kwargs): task = utils.retrieve_task(kwargs) task_id = utils.retrieve_task_id(kwargs) @@ -178,6 +190,9 @@ def _trace_postrun(*args, **kwargs): activation.__exit__(None, None, None) utils.detach_span(task, task_id) + self.update_task_duration_time(task_id) + labels = {"task": task.name, "worker": task.request.hostname} + self._record_histograms(task_id, labels) def _trace_before_publish(self, *args, **kwargs): task = utils.retrieve_task_from_sender(kwargs) @@ -277,3 +292,30 @@ def _trace_retry(*args, **kwargs): # Use `str(reason)` instead of `reason.message` in case we get # something that isn't an `Exception` span.set_attribute(_TASK_RETRY_REASON_KEY, str(reason)) + + def update_task_duration_time(self, task_id): + cur_time = default_timer() + task_duration_time_until_now = ( + cur_time - self.task_id_to_start_time[task_id] + if task_id in self.task_id_to_start_time + else cur_time + ) + self.task_id_to_start_time[task_id] = task_duration_time_until_now + + def _record_histograms(self, task_id, metric_attributes): + if task_id is None: + return + + self.metrics["flower.task.runtime.seconds"].record( + self.task_id_to_start_time.get(task_id), + attributes=metric_attributes, + ) + + def create_celery_metrics(self, meter) -> None: + self.metrics = { + "flower.task.runtime.seconds": meter.create_histogram( + name="flower.task.runtime.seconds", + unit="seconds", + description="The time it took to run the task.", + ) + } diff --git a/instrumentation/opentelemetry-instrumentation-celery/tests/test_metrics.py b/instrumentation/opentelemetry-instrumentation-celery/tests/test_metrics.py new file mode 100644 index 0000000000..46e39a6046 --- /dev/null +++ b/instrumentation/opentelemetry-instrumentation-celery/tests/test_metrics.py @@ -0,0 +1,76 @@ +import threading +import time +from timeit import default_timer + +from opentelemetry.instrumentation.celery import CeleryInstrumentor +from opentelemetry.test.test_base import TestBase + +from .celery_test_tasks import app, task_add + + +class TestMetrics(TestBase): + def setUp(self): + super().setUp() + self._worker = app.Worker( + app=app, pool="solo", concurrency=1, hostname="celery@akochavi" + ) + self._thread = threading.Thread(target=self._worker.start) + self._thread.daemon = True + self._thread.start() + + def tearDown(self): + super().tearDown() + self._worker.stop() + self._thread.join() + + def get_metrics(self): + result = task_add.delay(1, 2) + + timeout = time.time() + 60 * 1 # 1 minutes from now + while not result.ready(): + if time.time() > timeout: + break + time.sleep(0.05) + return self.get_sorted_metrics() + + def test_basic_metric(self): + CeleryInstrumentor().instrument() + start_time = default_timer() + task_runtime_estimated = (default_timer() - start_time) * 1000 + + metrics = self.get_metrics() + CeleryInstrumentor().uninstrument() + self.assertEqual(len(metrics), 1) + + task_runtime = metrics[0] + print(task_runtime) + self.assertEqual(task_runtime.name, "flower.task.runtime.seconds") + self.assert_metric_expected( + task_runtime, + [ + self.create_histogram_data_point( + count=1, + sum_data_point=task_runtime_estimated, + max_data_point=task_runtime_estimated, + min_data_point=task_runtime_estimated, + attributes={ + "task": "tests.celery_test_tasks.task_add", + "worker": "celery@akochavi", + }, + ) + ], + est_value_delta=200, + ) + + def test_metric_uninstrument(self): + CeleryInstrumentor().instrument() + metrics = self.get_metrics() + self.assertEqual(len(metrics), 1) + CeleryInstrumentor().uninstrument() + + metrics = self.get_metrics() + self.assertEqual(len(metrics), 1) + + for metric in metrics: + for point in list(metric.data.data_points): + self.assertEqual(point.count, 1) From 5ac58c2ffb4c595c9281f50606306943ce7b5044 Mon Sep 17 00:00:00 2001 From: David Gonoradsky Date: Sun, 18 Jun 2023 16:20:21 +0300 Subject: [PATCH 15/29] Add support for confluent_kafka until 2.1.1 version (#1815) * Add support for confulent_kafka until 2.1.1 version * Include 2.1.1 version * update CHANGELOG.md * run: 'tox -e generate' * resolve comments * update top version to 2.2.0 --------- Co-authored-by: Ran Nozik --- CHANGELOG.md | 2 ++ docs-requirements.txt | 2 +- instrumentation/README.md | 2 +- .../pyproject.toml | 2 +- .../opentelemetry/instrumentation/confluent_kafka/package.py | 2 +- .../src/opentelemetry/instrumentation/bootstrap_gen.py | 2 +- 6 files changed, 7 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b591e9a143..0e0229e51b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ([#1706](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1706)) - `opentelemetry-instrumentation-pymemcache` Update instrumentation to support pymemcache >4 ([#1764](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1764)) +- `opentelemetry-instrumentation-confluent-kafka` Add support for higher versions of confluent_kafka + ([#1815](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1815)) ### Added diff --git a/docs-requirements.txt b/docs-requirements.txt index 6d11d20198..4af8839334 100644 --- a/docs-requirements.txt +++ b/docs-requirements.txt @@ -26,7 +26,7 @@ boto~=2.0 botocore~=1.0 boto3~=1.0 celery>=4.0 -confluent-kafka>= 1.8.2,< 2.0.0 +confluent-kafka>= 1.8.2,<= 2.2.0 elasticsearch>=2.0,<9.0 flask~=2.0 falcon~=2.0 diff --git a/instrumentation/README.md b/instrumentation/README.md index e69dd6adbd..933e9b763d 100644 --- a/instrumentation/README.md +++ b/instrumentation/README.md @@ -11,7 +11,7 @@ | [opentelemetry-instrumentation-boto3sqs](./opentelemetry-instrumentation-boto3sqs) | boto3 ~= 1.0 | No | [opentelemetry-instrumentation-botocore](./opentelemetry-instrumentation-botocore) | botocore ~= 1.0 | No | [opentelemetry-instrumentation-celery](./opentelemetry-instrumentation-celery) | celery >= 4.0, < 6.0 | No -| [opentelemetry-instrumentation-confluent-kafka](./opentelemetry-instrumentation-confluent-kafka) | confluent-kafka >= 1.8.2, < 2.0.0 | No +| [opentelemetry-instrumentation-confluent-kafka](./opentelemetry-instrumentation-confluent-kafka) | confluent-kafka >= 1.8.2, <= 2.2.0 | No | [opentelemetry-instrumentation-dbapi](./opentelemetry-instrumentation-dbapi) | dbapi | No | [opentelemetry-instrumentation-django](./opentelemetry-instrumentation-django) | django >= 1.10 | Yes | [opentelemetry-instrumentation-elasticsearch](./opentelemetry-instrumentation-elasticsearch) | elasticsearch >= 2.0 | No diff --git a/instrumentation/opentelemetry-instrumentation-confluent-kafka/pyproject.toml b/instrumentation/opentelemetry-instrumentation-confluent-kafka/pyproject.toml index a84f7d959e..2ef1fd6b49 100644 --- a/instrumentation/opentelemetry-instrumentation-confluent-kafka/pyproject.toml +++ b/instrumentation/opentelemetry-instrumentation-confluent-kafka/pyproject.toml @@ -31,7 +31,7 @@ dependencies = [ [project.optional-dependencies] instruments = [ - "confluent-kafka >= 1.8.2, < 2.0.0", + "confluent-kafka >= 1.8.2, <= 2.2.0", ] test = [ "opentelemetry-instrumentation-confluent-kafka[instruments]", diff --git a/instrumentation/opentelemetry-instrumentation-confluent-kafka/src/opentelemetry/instrumentation/confluent_kafka/package.py b/instrumentation/opentelemetry-instrumentation-confluent-kafka/src/opentelemetry/instrumentation/confluent_kafka/package.py index eab664d9ee..21d37ebfae 100644 --- a/instrumentation/opentelemetry-instrumentation-confluent-kafka/src/opentelemetry/instrumentation/confluent_kafka/package.py +++ b/instrumentation/opentelemetry-instrumentation-confluent-kafka/src/opentelemetry/instrumentation/confluent_kafka/package.py @@ -13,4 +13,4 @@ # limitations under the License. -_instruments = ("confluent-kafka >= 1.8.2, < 2.0.0",) +_instruments = ("confluent-kafka >= 1.8.2, <= 2.2.0",) diff --git a/opentelemetry-instrumentation/src/opentelemetry/instrumentation/bootstrap_gen.py b/opentelemetry-instrumentation/src/opentelemetry/instrumentation/bootstrap_gen.py index f705324289..1e0b9fd2f5 100644 --- a/opentelemetry-instrumentation/src/opentelemetry/instrumentation/bootstrap_gen.py +++ b/opentelemetry-instrumentation/src/opentelemetry/instrumentation/bootstrap_gen.py @@ -53,7 +53,7 @@ "instrumentation": "opentelemetry-instrumentation-celery==0.40b0.dev", }, "confluent-kafka": { - "library": "confluent-kafka >= 1.8.2, < 2.0.0", + "library": "confluent-kafka >= 1.8.2, <= 2.2.0", "instrumentation": "opentelemetry-instrumentation-confluent-kafka==0.40b0.dev", }, "django": { From 8cc10a0859d0da4773985e1025d261c3e334ea28 Mon Sep 17 00:00:00 2001 From: Pablo Collins Date: Sun, 18 Jun 2023 09:55:17 -0400 Subject: [PATCH 16/29] fix redis doc (#1808) doc string rendered at https://opentelemetry-python-contrib.readthedocs.io/en/latest/instrumentation/redis/redis.html refers to `opentelemetry-instrumentation` executable which appears to be a typo Co-authored-by: Shalev Roda <65566801+shalevr@users.noreply.github.com> --- CHANGELOG.md | 1 - .../src/opentelemetry/instrumentation/redis/__init__.py | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0e0229e51b..c096812a60 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,7 +7,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## Unreleased - ### Added - Make Flask request span attributes available for `start_span`. 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 188840c7b8..3c8acdef31 100644 --- a/instrumentation/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/__init__.py @@ -16,7 +16,7 @@ Instrument `redis`_ to report Redis queries. There are two options for instrumenting code. The first option is to use the -``opentelemetry-instrumentation`` executable which will automatically +``opentelemetry-instrument`` executable which will automatically instrument your Redis client. The second is to programmatically enable instrumentation via the following code: From 60753e2a5528ac34aa415f1daa2fc2db53fc5eb4 Mon Sep 17 00:00:00 2001 From: Iman Shafiei Date: Mon, 19 Jun 2023 13:00:59 -0700 Subject: [PATCH 17/29] Add http.server.response.size metric to ASGI implementation. (#1789) * Add http.server.response.size metric to ASGI implementation. Add new unit tests. * Update changelog. * Fix linting by disabling too-many-nested-blocks * Put new logic in a new method * Refactor the placement of new logic. * Fixed the unit tests in FastAPI and Starlette * Update changelog. * FIx lint errors. * Refactor getting content-length header * Refactor getting content-length header --------- Co-authored-by: Shalev Roda <65566801+shalevr@users.noreply.github.com> Co-authored-by: Diego Hurtado --- CHANGELOG.md | 2 ++ .../instrumentation/asgi/__init__.py | 17 ++++++++++ .../tests/test_asgi_middleware.py | 31 +++++++++++++------ .../tests/test_fastapi_instrumentation.py | 7 ++++- .../tests/test_starlette_instrumentation.py | 4 ++- 5 files changed, 50 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c096812a60..de2ed9475e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ([#1780](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1780)) - Add metric instrumentation for celery ([#1679](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1679)) +- `opentelemetry-instrumentation-asgi` Add `http.server.response.size` metric + ([#1789](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1789)) ## Version 1.18.0/0.39b0 (2023-05-10) 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 010c6accde..1ee25ae7d9 100644 --- a/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py @@ -506,6 +506,11 @@ def __init__( unit="ms", description="measures the duration of the inbound HTTP request", ) + self.server_response_size_histogram = self.meter.create_histogram( + name=MetricInstruments.HTTP_SERVER_RESPONSE_SIZE, + unit="By", + description="measures the size of HTTP response messages (compressed).", + ) self.active_requests_counter = self.meter.create_up_down_counter( name=MetricInstruments.HTTP_SERVER_ACTIVE_REQUESTS, unit="requests", @@ -518,6 +523,7 @@ def __init__( self.server_request_hook = server_request_hook self.client_request_hook = client_request_hook self.client_response_hook = client_response_hook + self.content_length_header = None async def __call__(self, scope, receive, send): """The ASGI application @@ -593,6 +599,10 @@ async def __call__(self, scope, receive, send): self.active_requests_counter.add( -1, active_requests_count_attrs ) + if self.content_length_header: + self.server_response_size_histogram.record( + self.content_length_header, duration_attrs + ) if token: context.detach(token) @@ -660,6 +670,13 @@ async def otel_send(message): setter=asgi_setter, ) + content_length = asgi_getter.get(message, "content-length") + if content_length: + try: + self.content_length_header = int(content_length[0]) + except ValueError: + pass + await send(message) return otel_send diff --git a/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py b/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py index f9a5731fd3..0a0c2c301f 100644 --- a/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py +++ b/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py @@ -46,10 +46,12 @@ _expected_metric_names = [ "http.server.active_requests", "http.server.duration", + "http.server.response.size", ] _recommended_attrs = { "http.server.active_requests": _active_requests_count_attrs, "http.server.duration": _duration_attrs, + "http.server.response.size": _duration_attrs, } @@ -61,7 +63,10 @@ async def http_app(scope, receive, send): { "type": "http.response.start", "status": 200, - "headers": [[b"Content-Type", b"text/plain"]], + "headers": [ + [b"Content-Type", b"text/plain"], + [b"content-length", b"1024"], + ], } ) await send({"type": "http.response.body", "body": b"*"}) @@ -103,7 +108,10 @@ async def error_asgi(scope, receive, send): { "type": "http.response.start", "status": 200, - "headers": [[b"Content-Type", b"text/plain"]], + "headers": [ + [b"Content-Type", b"text/plain"], + [b"content-length", b"1024"], + ], } ) await send({"type": "http.response.body", "body": b"*"}) @@ -126,7 +134,8 @@ def validate_outputs(self, outputs, error=None, modifiers=None): # Check http response start self.assertEqual(response_start["status"], 200) self.assertEqual( - response_start["headers"], [[b"Content-Type", b"text/plain"]] + response_start["headers"], + [[b"Content-Type", b"text/plain"], [b"content-length", b"1024"]], ) exc_info = self.scope.get("hack_exc_info") @@ -352,6 +361,7 @@ def test_traceresponse_header(self): response_start["headers"], [ [b"Content-Type", b"text/plain"], + [b"content-length", b"1024"], [b"traceresponse", f"{traceresponse}".encode()], [b"access-control-expose-headers", b"traceresponse"], ], @@ -565,6 +575,7 @@ def test_basic_metric_success(self): "http.flavor": "1.0", } metrics_list = self.memory_metrics_reader.get_metrics_data() + # pylint: disable=too-many-nested-blocks for resource_metric in metrics_list.resource_metrics: for scope_metrics in resource_metric.scope_metrics: for metric in scope_metrics.metrics: @@ -575,9 +586,12 @@ def test_basic_metric_success(self): dict(point.attributes), ) self.assertEqual(point.count, 1) - self.assertAlmostEqual( - duration, point.sum, delta=5 - ) + if metric.name == "http.server.duration": + self.assertAlmostEqual( + duration, point.sum, delta=5 + ) + elif metric.name == "http.server.response.size": + self.assertEqual(1024, point.sum) elif isinstance(point, NumberDataPoint): self.assertDictEqual( expected_requests_count_attributes, @@ -602,13 +616,12 @@ async def target_asgi(scope, receive, send): app = otel_asgi.OpenTelemetryMiddleware(target_asgi) self.seed_app(app) self.send_default_request() - metrics_list = self.memory_metrics_reader.get_metrics_data() assertions = 0 for resource_metric in metrics_list.resource_metrics: for scope_metrics in resource_metric.scope_metrics: for metric in scope_metrics.metrics: - if metric.name != "http.server.duration": + if metric.name == "http.server.active_requests": continue for point in metric.data.data_points: if isinstance(point, HistogramDataPoint): @@ -617,7 +630,7 @@ async def target_asgi(scope, receive, send): expected_target, ) assertions += 1 - self.assertEqual(assertions, 1) + self.assertEqual(assertions, 2) def test_no_metric_for_websockets(self): self.scope = { diff --git a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py index 9420ba2c0e..7f12d6e3f3 100644 --- a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py +++ b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py @@ -44,10 +44,15 @@ _expected_metric_names = [ "http.server.active_requests", "http.server.duration", + "http.server.response.size", ] _recommended_attrs = { "http.server.active_requests": _active_requests_count_attrs, "http.server.duration": {*_duration_attrs, SpanAttributes.HTTP_TARGET}, + "http.server.response.size": { + *_duration_attrs, + SpanAttributes.HTTP_TARGET, + }, } @@ -187,7 +192,7 @@ def test_fastapi_metrics(self): for resource_metric in metrics_list.resource_metrics: self.assertTrue(len(resource_metric.scope_metrics) == 1) for scope_metric in resource_metric.scope_metrics: - self.assertTrue(len(scope_metric.metrics) == 2) + self.assertTrue(len(scope_metric.metrics) == 3) for metric in scope_metric.metrics: self.assertIn(metric.name, _expected_metric_names) data_points = list(metric.data.data_points) diff --git a/instrumentation/opentelemetry-instrumentation-starlette/tests/test_starlette_instrumentation.py b/instrumentation/opentelemetry-instrumentation-starlette/tests/test_starlette_instrumentation.py index 1f4570d293..2a53bdffb7 100644 --- a/instrumentation/opentelemetry-instrumentation-starlette/tests/test_starlette_instrumentation.py +++ b/instrumentation/opentelemetry-instrumentation-starlette/tests/test_starlette_instrumentation.py @@ -49,10 +49,12 @@ _expected_metric_names = [ "http.server.active_requests", "http.server.duration", + "http.server.response.size", ] _recommended_attrs = { "http.server.active_requests": _active_requests_count_attrs, "http.server.duration": _duration_attrs, + "http.server.response.size": _duration_attrs, } @@ -128,7 +130,7 @@ def test_starlette_metrics(self): for resource_metric in metrics_list.resource_metrics: self.assertTrue(len(resource_metric.scope_metrics) == 1) for scope_metric in resource_metric.scope_metrics: - self.assertTrue(len(scope_metric.metrics) == 2) + self.assertTrue(len(scope_metric.metrics) == 3) for metric in scope_metric.metrics: self.assertIn(metric.name, _expected_metric_names) data_points = list(metric.data.data_points) From fe9405730f8d18fe70308db52093b7b5b1e74e0c Mon Sep 17 00:00:00 2001 From: Michael Date: Tue, 20 Jun 2023 14:09:53 +0300 Subject: [PATCH 18/29] fix: Update falcon instrumentation to follow semantic conventions (#1824) * fix: Update falcon instrumentation to follow semantic conventions * docs: Update changelog * fix linter errors * Disable falcon.HTTP_200 pylint checck --------- Co-authored-by: Shalev Roda <65566801+shalevr@users.noreply.github.com> Co-authored-by: Srikanth Chekuri --- CHANGELOG.md | 5 +++ .../instrumentation/falcon/__init__.py | 7 +++- .../tests/app.py | 9 +++++ .../tests/test_falcon.py | 33 +++++++++++++++++-- 4 files changed, 50 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index de2ed9475e..c83443dc02 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## Unreleased +### Fixed + +- Update falcon instrumentation to follow semantic conventions + ([#1824](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1824)) + ### Added - Make Flask request span attributes available for `start_span`. diff --git a/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py b/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py index 73c005fa17..669f41b0ab 100644 --- a/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py @@ -428,7 +428,6 @@ def process_resource(self, req, resp, resource, params): resource_name = resource.__class__.__name__ span.set_attribute("falcon.resource", resource_name) - span.update_name(f"{resource_name}.on_{req.method.lower()}") def process_response( self, req, resp, resource, req_succeeded=None @@ -483,6 +482,12 @@ def process_response( response_headers = resp.headers if span.is_recording() and span.kind == trace.SpanKind.SERVER: + # Check if low-cardinality route is available as per semantic-conventions + if req.uri_template: + span.update_name(f"{req.method} {req.uri_template}") + else: + span.update_name(f"{req.method}") + custom_attributes = ( otel_wsgi.collect_custom_response_headers_attributes( response_headers.items() diff --git a/instrumentation/opentelemetry-instrumentation-falcon/tests/app.py b/instrumentation/opentelemetry-instrumentation-falcon/tests/app.py index 3e4c62ec3e..a4d279149d 100644 --- a/instrumentation/opentelemetry-instrumentation-falcon/tests/app.py +++ b/instrumentation/opentelemetry-instrumentation-falcon/tests/app.py @@ -61,6 +61,13 @@ def on_get(self, _, resp): resp.set_header("my-secret-header", "my-secret-value") +class UserResource: + def on_get(self, req, resp, user_id): + # pylint: disable=no-member + resp.status = falcon.HTTP_200 + resp.body = f"Hello user {user_id}" + + def make_app(): _parsed_falcon_version = package_version.parse(falcon.__version__) if _parsed_falcon_version < package_version.parse("3.0.0"): @@ -76,4 +83,6 @@ def make_app(): app.add_route( "/test_custom_response_headers", CustomResponseHeaderResource() ) + app.add_route("/user/{user_id}", UserResource()) + return app diff --git a/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py b/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py index cf61a9fd3c..2245dbfd80 100644 --- a/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py +++ b/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py @@ -110,7 +110,7 @@ def _test_method(self, method): spans = self.memory_exporter.get_finished_spans() self.assertEqual(len(spans), 1) span = spans[0] - self.assertEqual(span.name, f"HelloWorldResource.on_{method.lower()}") + self.assertEqual(span.name, f"{method} /hello") self.assertEqual(span.status.status_code, StatusCode.UNSET) self.assertEqual( span.status.description, @@ -145,7 +145,7 @@ def test_404(self): spans = self.memory_exporter.get_finished_spans() self.assertEqual(len(spans), 1) span = spans[0] - self.assertEqual(span.name, "GET /does-not-exist") + self.assertEqual(span.name, "GET") self.assertEqual(span.status.status_code, StatusCode.UNSET) self.assertSpanHasAttributes( span, @@ -177,7 +177,7 @@ def test_500(self): spans = self.memory_exporter.get_finished_spans() self.assertEqual(len(spans), 1) span = spans[0] - self.assertEqual(span.name, "ErrorResource.on_get") + self.assertEqual(span.name, "GET /error") self.assertFalse(span.status.is_ok) self.assertEqual(span.status.status_code, StatusCode.ERROR) self.assertEqual( @@ -206,6 +206,33 @@ def test_500(self): span.attributes[SpanAttributes.NET_PEER_IP], "127.0.0.1" ) + def test_url_template(self): + self.client().simulate_get("/user/123") + spans = self.memory_exporter.get_finished_spans() + self.assertEqual(len(spans), 1) + span = spans[0] + self.assertEqual(span.name, "GET /user/{user_id}") + self.assertEqual(span.status.status_code, StatusCode.UNSET) + self.assertEqual( + span.status.description, + None, + ) + self.assertSpanHasAttributes( + span, + { + SpanAttributes.HTTP_METHOD: "GET", + SpanAttributes.HTTP_SERVER_NAME: "falconframework.org", + SpanAttributes.HTTP_SCHEME: "http", + SpanAttributes.NET_HOST_PORT: 80, + SpanAttributes.HTTP_HOST: "falconframework.org", + SpanAttributes.HTTP_TARGET: "/", + SpanAttributes.NET_PEER_PORT: "65133", + SpanAttributes.HTTP_FLAVOR: "1.1", + "falcon.resource": "UserResource", + SpanAttributes.HTTP_STATUS_CODE: 200, + }, + ) + def test_uninstrument(self): self.client().simulate_get(path="/hello") spans = self.memory_exporter.get_finished_spans() From ffc9334dd73d1247f28db92a143ec7cc91624995 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Tue, 20 Jun 2023 16:01:36 +0000 Subject: [PATCH 19/29] Bump requests from 2.28.1 to 2.31.0 (#1818) Bumps [requests](https://github.com/psf/requests) from 2.28.1 to 2.31.0. - [Release notes](https://github.com/psf/requests/releases) - [Changelog](https://github.com/psf/requests/blob/main/HISTORY.md) - [Commits](https://github.com/psf/requests/compare/v2.28.1...v2.31.0) --- updated-dependencies: - dependency-name: requests dependency-type: direct:production ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Shalev Roda <65566801+shalevr@users.noreply.github.com> --- dev-requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dev-requirements.txt b/dev-requirements.txt index 8973fb9476..feab6b4b02 100644 --- a/dev-requirements.txt +++ b/dev-requirements.txt @@ -16,5 +16,5 @@ mypy-protobuf>=1.23 protobuf~=3.13 markupsafe>=2.0.1 codespell==2.1.0 -requests==2.28.1 +requests==2.31.0 ruamel.yaml==0.17.21 From 32ae65ed55150131a6889e6661de5684b133b5da Mon Sep 17 00:00:00 2001 From: Matt Oberle Date: Wed, 21 Jun 2023 08:30:35 -0400 Subject: [PATCH 20/29] fix(grpc): Allow gRPC connections via Unix socket (#1833) * fix(grpc): Allow gRPC connections via Unix socket This commit addresses issue #1832. The way `NET_PEER_IP` and `NET_PEER_PORT` are retrieved raises a `ValueError` when gRPC connections are handled via Unix sockets. ```py ip, port = ( context.peer().split(",")[0].split(":", 1)[1].rsplit(":", 1) ) ``` When using an address like `unix:///tmp/grpc.sock` the value of `context.peer()` is `"unix:"`. Substituting that in the function above... ```py ip, port = "unix:".split(",")[0].split(":", 1)[1].rsplit(":", 1) ip, port = ["unix:"][0].split(":", 1)[1].rsplit(":", 1) ip, port = "unix:".split(":", 1)[1].rsplit(":", 1) ip, port = ["unix", ""][1].rsplit(":", 1) ip, port = "".rsplit(":", 1) ip, port = [""] # ValueError ``` I "addressed" the issue by guarding the retrieval of `net.peer.*` values under an `if` statement that checks if we are using a Unix socket. I extended the `server_interceptor` tests to run against TCP and Unix socket configurations. --- **Open Questions** - [ ] The socket tests will fail on Windows. Is there a way to annotate that? - [ ] Are there other span values we should be setting for the unix socket? * Update CHANGELOG * Add placeholder attributes for linter * fix lint --------- Co-authored-by: Matt Oberle Co-authored-by: Shalev Roda <65566801+shalevr@users.noreply.github.com> --- CHANGELOG.md | 2 + .../instrumentation/grpc/_server.py | 38 ++-- .../tests/test_server_interceptor.py | 188 ++++++++---------- 3 files changed, 107 insertions(+), 121 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c83443dc02..ad6f969aa1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ([#1679](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1679)) - `opentelemetry-instrumentation-asgi` Add `http.server.response.size` metric ([#1789](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1789)) +- `opentelemetry-instrumentation-grpc` Allow gRPC connections via Unix socket + ([#1833](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1833)) ## Version 1.18.0/0.39b0 (2023-05-10) diff --git a/instrumentation/opentelemetry-instrumentation-grpc/src/opentelemetry/instrumentation/grpc/_server.py b/instrumentation/opentelemetry-instrumentation-grpc/src/opentelemetry/instrumentation/grpc/_server.py index a34cac0b3c..dcee959b4d 100644 --- a/instrumentation/opentelemetry-instrumentation-grpc/src/opentelemetry/instrumentation/grpc/_server.py +++ b/instrumentation/opentelemetry-instrumentation-grpc/src/opentelemetry/instrumentation/grpc/_server.py @@ -250,24 +250,30 @@ def _start_span( # * ipv4:127.0.0.1:57284 # * ipv4:10.2.1.1:57284,127.0.0.1:57284 # - try: - ip, port = ( - context.peer().split(",")[0].split(":", 1)[1].rsplit(":", 1) - ) - ip = unquote(ip) - attributes.update( - { - SpanAttributes.NET_PEER_IP: ip, - SpanAttributes.NET_PEER_PORT: port, - } - ) + if context.peer() != "unix:": + try: + ip, port = ( + context.peer() + .split(",")[0] + .split(":", 1)[1] + .rsplit(":", 1) + ) + ip = unquote(ip) + attributes.update( + { + SpanAttributes.NET_PEER_IP: ip, + SpanAttributes.NET_PEER_PORT: port, + } + ) - # other telemetry sources add this, so we will too - if ip in ("[::1]", "127.0.0.1"): - attributes[SpanAttributes.NET_PEER_NAME] = "localhost" + # other telemetry sources add this, so we will too + if ip in ("[::1]", "127.0.0.1"): + attributes[SpanAttributes.NET_PEER_NAME] = "localhost" - except IndexError: - logger.warning("Failed to parse peer address '%s'", context.peer()) + except IndexError: + logger.warning( + "Failed to parse peer address '%s'", context.peer() + ) return self._tracer.start_as_current_span( name=handler_call_details.method, diff --git a/instrumentation/opentelemetry-instrumentation-grpc/tests/test_server_interceptor.py b/instrumentation/opentelemetry-instrumentation-grpc/tests/test_server_interceptor.py index b48d887f5a..57f27c89d6 100644 --- a/instrumentation/opentelemetry-instrumentation-grpc/tests/test_server_interceptor.py +++ b/instrumentation/opentelemetry-instrumentation-grpc/tests/test_server_interceptor.py @@ -15,6 +15,8 @@ # pylint:disable=unused-argument # pylint:disable=no-self-use +import contextlib +import tempfile import threading from concurrent import futures @@ -78,23 +80,32 @@ def ServerStreamingMethod(self, request, context): class TestOpenTelemetryServerInterceptor(TestBase): - def test_instrumentor(self): - def handler(request, context): - return b"" - - grpc_server_instrumentor = GrpcInstrumentorServer() - grpc_server_instrumentor.instrument() - with futures.ThreadPoolExecutor(max_workers=1) as executor: + net_peer_span_attributes = { + SpanAttributes.NET_PEER_IP: "[::1]", + SpanAttributes.NET_PEER_NAME: "localhost", + } + + @contextlib.contextmanager + def server(self, max_workers=1, interceptors=None): + with futures.ThreadPoolExecutor(max_workers=max_workers) as executor: server = grpc.server( executor, options=(("grpc.so_reuseport", 0),), + interceptors=interceptors or [], ) - server.add_generic_rpc_handlers((UnaryUnaryRpcHandler(handler),)) - port = server.add_insecure_port("[::]:0") channel = grpc.insecure_channel(f"localhost:{port:d}") + yield server, channel + + def test_instrumentor(self): + def handler(request, context): + return b"" + grpc_server_instrumentor = GrpcInstrumentorServer() + grpc_server_instrumentor.instrument() + with self.server(max_workers=1) as (server, channel): + server.add_generic_rpc_handlers((UnaryUnaryRpcHandler(handler),)) rpc_call = "TestServicer/handler" try: server.start() @@ -117,8 +128,7 @@ def handler(request, context): self.assertSpanHasAttributes( span, { - SpanAttributes.NET_PEER_IP: "[::1]", - SpanAttributes.NET_PEER_NAME: "localhost", + **self.net_peer_span_attributes, SpanAttributes.RPC_METHOD: "handler", SpanAttributes.RPC_SERVICE: "TestServicer", SpanAttributes.RPC_SYSTEM: "grpc", @@ -137,17 +147,8 @@ def handler(request, context): grpc_server_instrumentor = GrpcInstrumentorServer() grpc_server_instrumentor.instrument() grpc_server_instrumentor.uninstrument() - with futures.ThreadPoolExecutor(max_workers=1) as executor: - server = grpc.server( - executor, - options=(("grpc.so_reuseport", 0),), - ) - + with self.server(max_workers=1) as (server, channel): server.add_generic_rpc_handlers((UnaryUnaryRpcHandler(handler),)) - - port = server.add_insecure_port("[::]:0") - channel = grpc.insecure_channel(f"localhost:{port:d}") - rpc_call = "TestServicer/test" try: server.start() @@ -164,15 +165,11 @@ def test_create_span(self): # Intercept gRPC calls... interceptor = server_interceptor() - with futures.ThreadPoolExecutor(max_workers=1) as executor: - server = grpc.server( - executor, - options=(("grpc.so_reuseport", 0),), - interceptors=[interceptor], - ) + with self.server( + max_workers=1, + interceptors=[interceptor], + ) as (server, channel): add_GRPCTestServerServicer_to_server(Servicer(), server) - port = server.add_insecure_port("[::]:0") - channel = grpc.insecure_channel(f"localhost:{port:d}") rpc_call = "/GRPCTestServer/SimpleMethod" request = Request(client_id=1, request_data="test") @@ -199,8 +196,7 @@ def test_create_span(self): self.assertSpanHasAttributes( span, { - SpanAttributes.NET_PEER_IP: "[::1]", - SpanAttributes.NET_PEER_NAME: "localhost", + **self.net_peer_span_attributes, SpanAttributes.RPC_METHOD: "SimpleMethod", SpanAttributes.RPC_SERVICE: "GRPCTestServer", SpanAttributes.RPC_SYSTEM: "grpc", @@ -231,15 +227,11 @@ def SimpleMethod(self, request, context): interceptor = server_interceptor() # setup the server - with futures.ThreadPoolExecutor(max_workers=1) as executor: - server = grpc.server( - executor, - options=(("grpc.so_reuseport", 0),), - interceptors=[interceptor], - ) + with self.server( + max_workers=1, + interceptors=[interceptor], + ) as (server, channel): add_GRPCTestServerServicer_to_server(TwoSpanServicer(), server) - port = server.add_insecure_port("[::]:0") - channel = grpc.insecure_channel(f"localhost:{port:d}") # setup the RPC rpc_call = "/GRPCTestServer/SimpleMethod" @@ -268,8 +260,7 @@ def SimpleMethod(self, request, context): self.assertSpanHasAttributes( parent_span, { - SpanAttributes.NET_PEER_IP: "[::1]", - SpanAttributes.NET_PEER_NAME: "localhost", + **self.net_peer_span_attributes, SpanAttributes.RPC_METHOD: "SimpleMethod", SpanAttributes.RPC_SERVICE: "GRPCTestServer", SpanAttributes.RPC_SYSTEM: "grpc", @@ -292,15 +283,11 @@ def test_create_span_streaming(self): # Intercept gRPC calls... interceptor = server_interceptor() - with futures.ThreadPoolExecutor(max_workers=1) as executor: - server = grpc.server( - executor, - options=(("grpc.so_reuseport", 0),), - interceptors=[interceptor], - ) + with self.server( + max_workers=1, + interceptors=[interceptor], + ) as (server, channel): add_GRPCTestServerServicer_to_server(Servicer(), server) - port = server.add_insecure_port("[::]:0") - channel = grpc.insecure_channel(f"localhost:{port:d}") # setup the RPC rpc_call = "/GRPCTestServer/ServerStreamingMethod" @@ -328,8 +315,7 @@ def test_create_span_streaming(self): self.assertSpanHasAttributes( span, { - SpanAttributes.NET_PEER_IP: "[::1]", - SpanAttributes.NET_PEER_NAME: "localhost", + **self.net_peer_span_attributes, SpanAttributes.RPC_METHOD: "ServerStreamingMethod", SpanAttributes.RPC_SERVICE: "GRPCTestServer", SpanAttributes.RPC_SYSTEM: "grpc", @@ -360,15 +346,11 @@ def ServerStreamingMethod(self, request, context): # Intercept gRPC calls... interceptor = server_interceptor() - with futures.ThreadPoolExecutor(max_workers=1) as executor: - server = grpc.server( - executor, - options=(("grpc.so_reuseport", 0),), - interceptors=[interceptor], - ) + with self.server( + max_workers=1, + interceptors=[interceptor], + ) as (server, channel): add_GRPCTestServerServicer_to_server(TwoSpanServicer(), server) - port = server.add_insecure_port("[::]:0") - channel = grpc.insecure_channel(f"localhost:{port:d}") # setup the RPC rpc_call = "/GRPCTestServer/ServerStreamingMethod" @@ -397,8 +379,7 @@ def ServerStreamingMethod(self, request, context): self.assertSpanHasAttributes( parent_span, { - SpanAttributes.NET_PEER_IP: "[::1]", - SpanAttributes.NET_PEER_NAME: "localhost", + **self.net_peer_span_attributes, SpanAttributes.RPC_METHOD: "ServerStreamingMethod", SpanAttributes.RPC_SERVICE: "GRPCTestServer", SpanAttributes.RPC_SYSTEM: "grpc", @@ -427,17 +408,12 @@ def handler(request, context): active_span_in_handler = trace.get_current_span() return b"" - with futures.ThreadPoolExecutor(max_workers=1) as executor: - server = grpc.server( - executor, - options=(("grpc.so_reuseport", 0),), - interceptors=[interceptor], - ) + with self.server( + max_workers=1, + interceptors=[interceptor], + ) as (server, channel): server.add_generic_rpc_handlers((UnaryUnaryRpcHandler(handler),)) - port = server.add_insecure_port("[::]:0") - channel = grpc.insecure_channel(f"localhost:{port:d}") - active_span_before_call = trace.get_current_span() try: server.start() @@ -463,17 +439,12 @@ def handler(request, context): active_spans_in_handler.append(trace.get_current_span()) return b"" - with futures.ThreadPoolExecutor(max_workers=1) as executor: - server = grpc.server( - executor, - options=(("grpc.so_reuseport", 0),), - interceptors=[interceptor], - ) + with self.server( + max_workers=1, + interceptors=[interceptor], + ) as (server, channel): server.add_generic_rpc_handlers((UnaryUnaryRpcHandler(handler),)) - port = server.add_insecure_port("[::]:0") - channel = grpc.insecure_channel(f"localhost:{port:d}") - try: server.start() channel.unary_unary("TestServicer/handler")(b"") @@ -496,8 +467,7 @@ def handler(request, context): self.assertSpanHasAttributes( span, { - SpanAttributes.NET_PEER_IP: "[::1]", - SpanAttributes.NET_PEER_NAME: "localhost", + **self.net_peer_span_attributes, SpanAttributes.RPC_METHOD: "handler", SpanAttributes.RPC_SERVICE: "TestServicer", SpanAttributes.RPC_SYSTEM: "grpc", @@ -527,17 +497,12 @@ def handler(request, context): active_spans_in_handler.append(trace.get_current_span()) return b"" - with futures.ThreadPoolExecutor(max_workers=2) as executor: - server = grpc.server( - executor, - options=(("grpc.so_reuseport", 0),), - interceptors=[interceptor], - ) + with self.server( + max_workers=2, + interceptors=[interceptor], + ) as (server, channel): server.add_generic_rpc_handlers((UnaryUnaryRpcHandler(handler),)) - port = server.add_insecure_port("[::]:0") - channel = grpc.insecure_channel(f"localhost:{port:d}") - try: server.start() # Interleave calls so spans are active on each thread at the same @@ -568,8 +533,7 @@ def handler(request, context): self.assertSpanHasAttributes( span, { - SpanAttributes.NET_PEER_IP: "[::1]", - SpanAttributes.NET_PEER_NAME: "localhost", + **self.net_peer_span_attributes, SpanAttributes.RPC_METHOD: "handler", SpanAttributes.RPC_SERVICE: "TestServicer", SpanAttributes.RPC_SYSTEM: "grpc", @@ -592,18 +556,11 @@ def test_abort(self): def handler(request, context): context.abort(grpc.StatusCode.FAILED_PRECONDITION, failure_message) - with futures.ThreadPoolExecutor(max_workers=1) as executor: - server = grpc.server( - executor, - options=(("grpc.so_reuseport", 0),), - interceptors=[interceptor], - ) - + with self.server( + max_workers=1, + interceptors=[interceptor], + ) as (server, channel): server.add_generic_rpc_handlers((UnaryUnaryRpcHandler(handler),)) - - port = server.add_insecure_port("[::]:0") - channel = grpc.insecure_channel(f"localhost:{port:d}") - rpc_call = "TestServicer/handler" server.start() @@ -635,8 +592,7 @@ def handler(request, context): self.assertSpanHasAttributes( span, { - SpanAttributes.NET_PEER_IP: "[::1]", - SpanAttributes.NET_PEER_NAME: "localhost", + **self.net_peer_span_attributes, SpanAttributes.RPC_METHOD: "handler", SpanAttributes.RPC_SERVICE: "TestServicer", SpanAttributes.RPC_SYSTEM: "grpc", @@ -647,6 +603,28 @@ def handler(request, context): ) +class TestOpenTelemetryServerInterceptorUnix( + TestOpenTelemetryServerInterceptor, +): + net_peer_span_attributes = {} + + @contextlib.contextmanager + def server(self, max_workers=1, interceptors=None): + with futures.ThreadPoolExecutor( + max_workers=max_workers + ) as executor, tempfile.TemporaryDirectory() as tmp: + server = grpc.server( + executor, + options=(("grpc.so_reuseport", 0),), + interceptors=interceptors or [], + ) + + sock = f"unix://{tmp}/grpc.sock" + server.add_insecure_port(sock) + channel = grpc.insecure_channel(sock) + yield server, channel + + def get_latch(num): """Get a countdown latch function for use in n threads.""" cv = threading.Condition() From 256d8ce12d28d80586446f94e14fa59a91596230 Mon Sep 17 00:00:00 2001 From: Iman Shafiei Date: Wed, 21 Jun 2023 13:56:38 -0700 Subject: [PATCH 21/29] Add http.server.request.size for ASGI metric implementation (#1867) * Update changelog file. * Update changelog file. * Add new request.size metric for ASGI middleware. * Clean-up. * Refactor try except section. --------- Co-authored-by: Shalev Roda <65566801+shalevr@users.noreply.github.com> --- CHANGELOG.md | 2 ++ .../instrumentation/asgi/__init__.py | 15 +++++++++++++++ .../tests/test_asgi_middleware.py | 8 +++++++- .../tests/test_fastapi_instrumentation.py | 19 +++++++++++++++++-- .../tests/test_starlette_instrumentation.py | 16 ++++++++++++++-- 5 files changed, 55 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ad6f969aa1..9aeb2e2cd4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,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 +- `opentelemetry-instrumentation-asgi` Add `http.server.request.size` metric + ([#1867](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1867)) ### Fixed 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 1ee25ae7d9..a1fa1f8e31 100644 --- a/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py @@ -511,6 +511,11 @@ def __init__( unit="By", description="measures the size of HTTP response messages (compressed).", ) + self.server_request_size_histogram = self.meter.create_histogram( + name=MetricInstruments.HTTP_SERVER_REQUEST_SIZE, + unit="By", + description="Measures the size of HTTP request messages (compressed).", + ) self.active_requests_counter = self.meter.create_up_down_counter( name=MetricInstruments.HTTP_SERVER_ACTIVE_REQUESTS, unit="requests", @@ -603,6 +608,16 @@ async def __call__(self, scope, receive, send): self.server_response_size_histogram.record( self.content_length_header, duration_attrs ) + request_size = asgi_getter.get(scope, "content-length") + if request_size: + try: + request_size_amount = int(request_size[0]) + except ValueError: + pass + else: + self.server_request_size_histogram.record( + request_size_amount, duration_attrs + ) if token: context.detach(token) diff --git a/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py b/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py index 0a0c2c301f..8ca82d0226 100644 --- a/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py +++ b/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py @@ -47,16 +47,19 @@ "http.server.active_requests", "http.server.duration", "http.server.response.size", + "http.server.request.size", ] _recommended_attrs = { "http.server.active_requests": _active_requests_count_attrs, "http.server.duration": _duration_attrs, "http.server.response.size": _duration_attrs, + "http.server.request.size": _duration_attrs, } async def http_app(scope, receive, send): message = await receive() + scope["headers"] = [(b"content-length", b"128")] assert scope["type"] == "http" if message.get("type") == "http.request": await send( @@ -99,6 +102,7 @@ async def error_asgi(scope, receive, send): assert isinstance(scope, dict) assert scope["type"] == "http" message = await receive() + scope["headers"] = [(b"content-length", b"128")] if message.get("type") == "http.request": try: raise ValueError @@ -592,6 +596,8 @@ def test_basic_metric_success(self): ) elif metric.name == "http.server.response.size": self.assertEqual(1024, point.sum) + elif metric.name == "http.server.request.size": + self.assertEqual(128, point.sum) elif isinstance(point, NumberDataPoint): self.assertDictEqual( expected_requests_count_attributes, @@ -630,7 +636,7 @@ async def target_asgi(scope, receive, send): expected_target, ) assertions += 1 - self.assertEqual(assertions, 2) + self.assertEqual(assertions, 3) def test_no_metric_for_websockets(self): self.scope = { diff --git a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py index 7f12d6e3f3..4269dfa2e4 100644 --- a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py +++ b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py @@ -45,6 +45,7 @@ "http.server.active_requests", "http.server.duration", "http.server.response.size", + "http.server.request.size", ] _recommended_attrs = { "http.server.active_requests": _active_requests_count_attrs, @@ -53,6 +54,10 @@ *_duration_attrs, SpanAttributes.HTTP_TARGET, }, + "http.server.request.size": { + *_duration_attrs, + SpanAttributes.HTTP_TARGET, + }, } @@ -251,8 +256,13 @@ def test_basic_metric_success(self): def test_basic_post_request_metric_success(self): start = default_timer() - self._client.post("/foobar") + response = self._client.post( + "/foobar", + json={"foo": "bar"}, + ) duration = max(round((default_timer() - start) * 1000), 0) + response_size = int(response.headers.get("content-length")) + request_size = int(response.request.headers.get("content-length")) metrics_list = self.memory_metrics_reader.get_metrics_data() for metric in ( metrics_list.resource_metrics[0].scope_metrics[0].metrics @@ -260,7 +270,12 @@ def test_basic_post_request_metric_success(self): for point in list(metric.data.data_points): if isinstance(point, HistogramDataPoint): self.assertEqual(point.count, 1) - self.assertAlmostEqual(duration, point.sum, delta=30) + if metric.name == "http.server.duration": + self.assertAlmostEqual(duration, point.sum, delta=30) + elif metric.name == "http.server.response.size": + self.assertEqual(response_size, point.sum) + elif metric.name == "http.server.request.size": + self.assertEqual(request_size, point.sum) if isinstance(point, NumberDataPoint): self.assertEqual(point.value, 0) diff --git a/instrumentation/opentelemetry-instrumentation-starlette/tests/test_starlette_instrumentation.py b/instrumentation/opentelemetry-instrumentation-starlette/tests/test_starlette_instrumentation.py index 2a53bdffb7..e1c77312a4 100644 --- a/instrumentation/opentelemetry-instrumentation-starlette/tests/test_starlette_instrumentation.py +++ b/instrumentation/opentelemetry-instrumentation-starlette/tests/test_starlette_instrumentation.py @@ -50,11 +50,13 @@ "http.server.active_requests", "http.server.duration", "http.server.response.size", + "http.server.request.size", ] _recommended_attrs = { "http.server.active_requests": _active_requests_count_attrs, "http.server.duration": _duration_attrs, "http.server.response.size": _duration_attrs, + "http.server.request.size": _duration_attrs, } @@ -165,8 +167,13 @@ def test_basic_post_request_metric_success(self): "http.scheme": "http", "http.server_name": "testserver", } - self._client.post("/foobar") + response = self._client.post( + "/foobar", + json={"foo": "bar"}, + ) duration = max(round((default_timer() - start) * 1000), 0) + response_size = int(response.headers.get("content-length")) + request_size = int(response.request.headers.get("content-length")) metrics_list = self.memory_metrics_reader.get_metrics_data() for metric in ( metrics_list.resource_metrics[0].scope_metrics[0].metrics @@ -174,10 +181,15 @@ def test_basic_post_request_metric_success(self): for point in list(metric.data.data_points): if isinstance(point, HistogramDataPoint): self.assertEqual(point.count, 1) - self.assertAlmostEqual(duration, point.sum, delta=30) self.assertDictEqual( dict(point.attributes), expected_duration_attributes ) + if metric.name == "http.server.duration": + self.assertAlmostEqual(duration, point.sum, delta=30) + elif metric.name == "http.server.response.size": + self.assertEqual(response_size, point.sum) + elif metric.name == "http.server.request.size": + self.assertEqual(request_size, point.sum) if isinstance(point, NumberDataPoint): self.assertDictEqual( expected_requests_count_attributes, From c9004bd375c02171085f2b103cda2349ba8de954 Mon Sep 17 00:00:00 2001 From: Nimrod Shlagman Date: Sun, 25 Jun 2023 07:44:57 +0300 Subject: [PATCH 22/29] Fix elastic-search sanitization for bulk queries (#1870) * support sanitization for str body response * add CHANGELOG entry --------- Co-authored-by: Shalev Roda <65566801+shalevr@users.noreply.github.com> --- CHANGELOG.md | 2 ++ .../src/opentelemetry/instrumentation/elasticsearch/utils.py | 4 ++++ .../tests/test_elasticsearch.py | 4 ++++ 3 files changed, 10 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9aeb2e2cd4..a9c0097431 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Fix elastic-search instrumentation sanitization to support bulk queries + ([#1870](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1870)) - Update falcon instrumentation to follow semantic conventions ([#1824](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1824)) diff --git a/instrumentation/opentelemetry-instrumentation-elasticsearch/src/opentelemetry/instrumentation/elasticsearch/utils.py b/instrumentation/opentelemetry-instrumentation-elasticsearch/src/opentelemetry/instrumentation/elasticsearch/utils.py index ca4a466bab..97f2bc3b87 100644 --- a/instrumentation/opentelemetry-instrumentation-elasticsearch/src/opentelemetry/instrumentation/elasticsearch/utils.py +++ b/instrumentation/opentelemetry-instrumentation-elasticsearch/src/opentelemetry/instrumentation/elasticsearch/utils.py @@ -11,6 +11,7 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. +import json sanitized_keys = ( "message", @@ -51,6 +52,9 @@ def _unflatten_dict(d): def sanitize_body(body) -> str: + if isinstance(body, str): + body = json.loads(body) + flatten_body = _flatten_dict(body) for key in flatten_body: diff --git a/instrumentation/opentelemetry-instrumentation-elasticsearch/tests/test_elasticsearch.py b/instrumentation/opentelemetry-instrumentation-elasticsearch/tests/test_elasticsearch.py index 02bf3eb591..df91fe6d65 100644 --- a/instrumentation/opentelemetry-instrumentation-elasticsearch/tests/test_elasticsearch.py +++ b/instrumentation/opentelemetry-instrumentation-elasticsearch/tests/test_elasticsearch.py @@ -479,3 +479,7 @@ def test_body_sanitization(self, _): sanitize_body(sanitization_queries.filter_query), str(sanitization_queries.filter_query_sanitized), ) + self.assertEqual( + sanitize_body(json.dumps(sanitization_queries.interval_query)), + str(sanitization_queries.interval_query_sanitized), + ) From e70437a36ea8d153c0fa11b9ceb575f7001ad744 Mon Sep 17 00:00:00 2001 From: Tammy Baylis <96076570+tammy-baylis-swi@users.noreply.github.com> Date: Sat, 24 Jun 2023 22:25:09 -0700 Subject: [PATCH 23/29] Add conditional elastic_transport import (#1810) * Add conditional elastic_transport import * Update changelog * Add future es8 tests * Update CHANGELOG.md Co-authored-by: Diego Hurtado * Add license, rm pylint disable * Consistent elastic version check * lint import * Update CHANGELOG.md --------- Co-authored-by: Diego Hurtado Co-authored-by: Shalev Roda <65566801+shalevr@users.noreply.github.com> --- CHANGELOG.md | 2 + .../instrumentation/elasticsearch/__init__.py | 38 ++++++++++++----- .../tests/helpers_es8.py | 41 +++++++++++++++++++ .../tests/test_elasticsearch.py | 4 +- tox.ini | 2 + 5 files changed, 76 insertions(+), 11 deletions(-) create mode 100644 instrumentation/opentelemetry-instrumentation-elasticsearch/tests/helpers_es8.py diff --git a/CHANGELOG.md b/CHANGELOG.md index a9c0097431..ea4843f843 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ([#1789](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1789)) - `opentelemetry-instrumentation-grpc` Allow gRPC connections via Unix socket ([#1833](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1833)) +- Fix elasticsearch `Transport.perform_request` instrument wrap for elasticsearch >= 8 + ([#1810](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1810)) ## Version 1.18.0/0.39b0 (2023-05-10) diff --git a/instrumentation/opentelemetry-instrumentation-elasticsearch/src/opentelemetry/instrumentation/elasticsearch/__init__.py b/instrumentation/opentelemetry-instrumentation-elasticsearch/src/opentelemetry/instrumentation/elasticsearch/__init__.py index d39c172c6a..480ccb6402 100644 --- a/instrumentation/opentelemetry-instrumentation-elasticsearch/src/opentelemetry/instrumentation/elasticsearch/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-elasticsearch/src/opentelemetry/instrumentation/elasticsearch/__init__.py @@ -98,6 +98,12 @@ def response_hook(span, response): from .utils import sanitize_body +# Split of elasticsearch and elastic_transport in 8.0.0+ +# https://www.elastic.co/guide/en/elasticsearch/client/python-api/master/release-notes.html#rn-8-0-0 +es_transport_split = elasticsearch.VERSION[0] > 7 +if es_transport_split: + import elastic_transport + logger = getLogger(__name__) @@ -137,16 +143,28 @@ def _instrument(self, **kwargs): tracer = get_tracer(__name__, __version__, tracer_provider) request_hook = kwargs.get("request_hook") response_hook = kwargs.get("response_hook") - _wrap( - elasticsearch, - "Transport.perform_request", - _wrap_perform_request( - tracer, - self._span_name_prefix, - request_hook, - response_hook, - ), - ) + if es_transport_split: + _wrap( + elastic_transport, + "Transport.perform_request", + _wrap_perform_request( + tracer, + self._span_name_prefix, + request_hook, + response_hook, + ), + ) + else: + _wrap( + elasticsearch, + "Transport.perform_request", + _wrap_perform_request( + tracer, + self._span_name_prefix, + request_hook, + response_hook, + ), + ) def _uninstrument(self, **kwargs): unwrap(elasticsearch.Transport, "perform_request") diff --git a/instrumentation/opentelemetry-instrumentation-elasticsearch/tests/helpers_es8.py b/instrumentation/opentelemetry-instrumentation-elasticsearch/tests/helpers_es8.py new file mode 100644 index 0000000000..04ed2efda2 --- /dev/null +++ b/instrumentation/opentelemetry-instrumentation-elasticsearch/tests/helpers_es8.py @@ -0,0 +1,41 @@ +# Copyright The OpenTelemetry Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +from elasticsearch_dsl import Document, Keyword, Text + + +class Article(Document): + title = Text(analyzer="snowball", fields={"raw": Keyword()}) + body = Text(analyzer="snowball") + + class Index: + name = "test-index" + + +dsl_create_statement = { + "mappings": { + "properties": { + "title": { + "analyzer": "snowball", + "fields": {"raw": {"type": "keyword"}}, + "type": "text", + }, + "body": {"analyzer": "snowball", "type": "text"}, + } + } +} +dsl_index_result = (1, {}, '{"result": "created"}') +dsl_index_span_name = "Elasticsearch/test-index/_doc/2" +dsl_index_url = "/test-index/_doc/2" +dsl_search_method = "POST" diff --git a/instrumentation/opentelemetry-instrumentation-elasticsearch/tests/test_elasticsearch.py b/instrumentation/opentelemetry-instrumentation-elasticsearch/tests/test_elasticsearch.py index df91fe6d65..37dd5f9cd7 100644 --- a/instrumentation/opentelemetry-instrumentation-elasticsearch/tests/test_elasticsearch.py +++ b/instrumentation/opentelemetry-instrumentation-elasticsearch/tests/test_elasticsearch.py @@ -37,7 +37,9 @@ major_version = elasticsearch.VERSION[0] -if major_version == 7: +if major_version == 8: + from . import helpers_es8 as helpers # pylint: disable=no-name-in-module +elif major_version == 7: from . import helpers_es7 as helpers # pylint: disable=no-name-in-module elif major_version == 6: from . import helpers_es6 as helpers # pylint: disable=no-name-in-module diff --git a/tox.ini b/tox.ini index 2313eab1d2..c7d4c56193 100644 --- a/tox.ini +++ b/tox.ini @@ -255,6 +255,8 @@ deps = ; FIXME: Elasticsearch >=7 causes CI workflow tests to hang, see open-telemetry/opentelemetry-python-contrib#620 ; elasticsearch7: elasticsearch-dsl>=7.0,<8.0 ; elasticsearch7: elasticsearch>=7.0,<8.0 + ; elasticsearch8: elasticsearch-dsl>=8.0,<9.0 + ; elasticsearch8: elasticsearch>=8.0,<9.0 falcon1: falcon ==1.4.1 falcon2: falcon >=2.0.0,<3.0.0 falcon3: falcon >=3.0.0,<4.0.0 From cd6b024327a53e3cd81249068a372129f68efaf4 Mon Sep 17 00:00:00 2001 From: Vivanov98 <66319645+Vivanov98@users.noreply.github.com> Date: Sun, 25 Jun 2023 13:03:54 +0100 Subject: [PATCH 24/29] Fix async redis clients tracing (#1830) * Fix async redis clients tracing * Update changelog * Add functional integration tests and fix linting issues --------- Co-authored-by: Shalev Roda <65566801+shalevr@users.noreply.github.com> --- CHANGELOG.md | 1 + .../instrumentation/redis/__init__.py | 122 +++++++++++----- .../tests/test_redis.py | 49 +++++++ .../tests/redis/test_redis_functional.py | 132 ++++++++++++++++++ 4 files changed, 270 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ea4843f843..82fd3a23fe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- Fix async redis clients not being traced correctly ([#1830](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1830)) - Make Flask request span attributes available for `start_span`. ([#1784](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1784)) - Fix falcon instrumentation's usage of Span Status to only set the description if the status code is ERROR. 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 3c8acdef31..ba4b8d529e 100644 --- a/instrumentation/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/__init__.py @@ -136,6 +136,43 @@ def _set_connection_attributes(span, conn): span.set_attribute(key, value) +def _build_span_name(instance, cmd_args): + if len(cmd_args) > 0 and cmd_args[0]: + name = cmd_args[0] + else: + name = instance.connection_pool.connection_kwargs.get("db", 0) + return name + + +def _build_span_meta_data_for_pipeline(instance): + try: + command_stack = ( + instance.command_stack + if hasattr(instance, "command_stack") + else instance._command_stack + ) + + cmds = [ + _format_command_args(c.args if hasattr(c, "args") else c[0]) + for c in command_stack + ] + resource = "\n".join(cmds) + + span_name = " ".join( + [ + (c.args[0] if hasattr(c, "args") else c[0][0]) + for c in command_stack + ] + ) + except (AttributeError, IndexError): + command_stack = [] + resource = "" + span_name = "" + + return command_stack, resource, span_name + + +# pylint: disable=R0915 def _instrument( tracer, request_hook: _RequestHookT = None, @@ -143,11 +180,8 @@ def _instrument( ): def _traced_execute_command(func, instance, args, kwargs): query = _format_command_args(args) + name = _build_span_name(instance, args) - if len(args) > 0 and args[0]: - name = args[0] - else: - name = instance.connection_pool.connection_kwargs.get("db", 0) with tracer.start_as_current_span( name, kind=trace.SpanKind.CLIENT ) as span: @@ -163,31 +197,11 @@ def _traced_execute_command(func, instance, args, kwargs): return response def _traced_execute_pipeline(func, instance, args, kwargs): - try: - command_stack = ( - instance.command_stack - if hasattr(instance, "command_stack") - else instance._command_stack - ) - - cmds = [ - _format_command_args( - c.args if hasattr(c, "args") else c[0], - ) - for c in command_stack - ] - resource = "\n".join(cmds) - - span_name = " ".join( - [ - (c.args[0] if hasattr(c, "args") else c[0][0]) - for c in command_stack - ] - ) - except (AttributeError, IndexError): - command_stack = [] - resource = "" - span_name = "" + ( + command_stack, + resource, + span_name, + ) = _build_span_meta_data_for_pipeline(instance) with tracer.start_as_current_span( span_name, kind=trace.SpanKind.CLIENT @@ -232,32 +246,72 @@ def _traced_execute_pipeline(func, instance, args, kwargs): "ClusterPipeline.execute", _traced_execute_pipeline, ) + + async def _async_traced_execute_command(func, instance, 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: + if span.is_recording(): + span.set_attribute(SpanAttributes.DB_STATEMENT, query) + _set_connection_attributes(span, instance) + span.set_attribute("db.redis.args_length", len(args)) + if callable(request_hook): + request_hook(span, instance, args, kwargs) + response = await func(*args, **kwargs) + if callable(response_hook): + response_hook(span, instance, response) + return response + + async def _async_traced_execute_pipeline(func, instance, args, kwargs): + ( + command_stack, + resource, + span_name, + ) = _build_span_meta_data_for_pipeline(instance) + + with tracer.start_as_current_span( + span_name, kind=trace.SpanKind.CLIENT + ) as span: + if span.is_recording(): + span.set_attribute(SpanAttributes.DB_STATEMENT, resource) + _set_connection_attributes(span, instance) + span.set_attribute( + "db.redis.pipeline_length", len(command_stack) + ) + response = await func(*args, **kwargs) + if callable(response_hook): + response_hook(span, instance, response) + return response + if redis.VERSION >= _REDIS_ASYNCIO_VERSION: wrap_function_wrapper( "redis.asyncio", f"{redis_class}.execute_command", - _traced_execute_command, + _async_traced_execute_command, ) wrap_function_wrapper( "redis.asyncio.client", f"{pipeline_class}.execute", - _traced_execute_pipeline, + _async_traced_execute_pipeline, ) wrap_function_wrapper( "redis.asyncio.client", f"{pipeline_class}.immediate_execute_command", - _traced_execute_command, + _async_traced_execute_command, ) if redis.VERSION >= _REDIS_ASYNCIO_CLUSTER_VERSION: wrap_function_wrapper( "redis.asyncio.cluster", "RedisCluster.execute_command", - _traced_execute_command, + _async_traced_execute_command, ) wrap_function_wrapper( "redis.asyncio.cluster", "ClusterPipeline.execute", - _traced_execute_pipeline, + _async_traced_execute_pipeline, ) diff --git a/instrumentation/opentelemetry-instrumentation-redis/tests/test_redis.py b/instrumentation/opentelemetry-instrumentation-redis/tests/test_redis.py index cc6e7de75a..11e56ad953 100644 --- a/instrumentation/opentelemetry-instrumentation-redis/tests/test_redis.py +++ b/instrumentation/opentelemetry-instrumentation-redis/tests/test_redis.py @@ -11,9 +11,11 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. +import asyncio from unittest import mock import redis +import redis.asyncio from opentelemetry import trace from opentelemetry.instrumentation.redis import RedisInstrumentor @@ -21,6 +23,24 @@ from opentelemetry.trace import SpanKind +class AsyncMock: + """A sufficient async mock implementation. + + Python 3.7 doesn't have an inbuilt async mock class, so this is used. + """ + + def __init__(self): + self.mock = mock.Mock() + + async def __call__(self, *args, **kwargs): + future = asyncio.Future() + future.set_result("random") + return future + + def __getattr__(self, item): + return AsyncMock() + + class TestRedis(TestBase): def setUp(self): super().setUp() @@ -87,6 +107,35 @@ def test_instrument_uninstrument(self): spans = self.memory_exporter.get_finished_spans() self.assertEqual(len(spans), 1) + def test_instrument_uninstrument_async_client_command(self): + redis_client = redis.asyncio.Redis() + + with mock.patch.object(redis_client, "connection", AsyncMock()): + asyncio.run(redis_client.get("key")) + + spans = self.memory_exporter.get_finished_spans() + self.assertEqual(len(spans), 1) + self.memory_exporter.clear() + + # Test uninstrument + RedisInstrumentor().uninstrument() + + with mock.patch.object(redis_client, "connection", AsyncMock()): + asyncio.run(redis_client.get("key")) + + spans = self.memory_exporter.get_finished_spans() + self.assertEqual(len(spans), 0) + self.memory_exporter.clear() + + # Test instrument again + RedisInstrumentor().instrument() + + with mock.patch.object(redis_client, "connection", AsyncMock()): + asyncio.run(redis_client.get("key")) + + spans = self.memory_exporter.get_finished_spans() + self.assertEqual(len(spans), 1) + def test_response_hook(self): redis_client = redis.Redis() connection = redis.connection.Connection() diff --git a/tests/opentelemetry-docker-tests/tests/redis/test_redis_functional.py b/tests/opentelemetry-docker-tests/tests/redis/test_redis_functional.py index dc9cf8b1dc..481b8d21c8 100644 --- a/tests/opentelemetry-docker-tests/tests/redis/test_redis_functional.py +++ b/tests/opentelemetry-docker-tests/tests/redis/test_redis_functional.py @@ -13,6 +13,7 @@ # limitations under the License. import asyncio +from time import time_ns import redis import redis.asyncio @@ -318,6 +319,29 @@ def test_basics(self): ) self.assertEqual(span.attributes.get("db.redis.args_length"), 2) + def test_execute_command_traced_full_time(self): + """Command should be traced for coroutine execution time, not creation time.""" + coro_created_time = None + finish_time = None + + async def pipeline_simple(): + nonlocal coro_created_time + nonlocal finish_time + + # delay coroutine creation from coroutine execution + coro = self.redis_client.get("foo") + coro_created_time = time_ns() + await coro + finish_time = time_ns() + + async_call(pipeline_simple()) + + spans = self.memory_exporter.get_finished_spans() + self.assertEqual(len(spans), 1) + span = spans[0] + self.assertTrue(span.start_time > coro_created_time) + self.assertTrue(span.end_time < finish_time) + def test_pipeline_traced(self): async def pipeline_simple(): async with self.redis_client.pipeline( @@ -340,6 +364,35 @@ async def pipeline_simple(): ) self.assertEqual(span.attributes.get("db.redis.pipeline_length"), 3) + def test_pipeline_traced_full_time(self): + """Command should be traced for coroutine execution time, not creation time.""" + coro_created_time = None + finish_time = None + + async def pipeline_simple(): + async with self.redis_client.pipeline( + transaction=False + ) as pipeline: + nonlocal coro_created_time + nonlocal finish_time + pipeline.set("blah", 32) + pipeline.rpush("foo", "éé") + pipeline.hgetall("xxx") + + # delay coroutine creation from coroutine execution + coro = pipeline.execute() + coro_created_time = time_ns() + await coro + finish_time = time_ns() + + async_call(pipeline_simple()) + + spans = self.memory_exporter.get_finished_spans() + self.assertEqual(len(spans), 1) + span = spans[0] + self.assertTrue(span.start_time > coro_created_time) + self.assertTrue(span.end_time < finish_time) + def test_pipeline_immediate(self): async def pipeline_immediate(): async with self.redis_client.pipeline() as pipeline: @@ -359,6 +412,33 @@ async def pipeline_immediate(): span.attributes.get(SpanAttributes.DB_STATEMENT), "SET ? ?" ) + def test_pipeline_immediate_traced_full_time(self): + """Command should be traced for coroutine execution time, not creation time.""" + coro_created_time = None + finish_time = None + + async def pipeline_simple(): + async with self.redis_client.pipeline( + transaction=False + ) as pipeline: + nonlocal coro_created_time + nonlocal finish_time + pipeline.set("a", 1) + + # delay coroutine creation from coroutine execution + coro = pipeline.immediate_execute_command("SET", "b", 2) + coro_created_time = time_ns() + await coro + finish_time = time_ns() + + async_call(pipeline_simple()) + + spans = self.memory_exporter.get_finished_spans() + self.assertEqual(len(spans), 1) + span = spans[0] + self.assertTrue(span.start_time > coro_created_time) + self.assertTrue(span.end_time < finish_time) + def test_parent(self): """Ensure OpenTelemetry works with redis.""" ot_tracer = trace.get_tracer("redis_svc") @@ -408,6 +488,29 @@ def test_basics(self): ) self.assertEqual(span.attributes.get("db.redis.args_length"), 2) + def test_execute_command_traced_full_time(self): + """Command should be traced for coroutine execution time, not creation time.""" + coro_created_time = None + finish_time = None + + async def pipeline_simple(): + nonlocal coro_created_time + nonlocal finish_time + + # delay coroutine creation from coroutine execution + coro = self.redis_client.get("foo") + coro_created_time = time_ns() + await coro + finish_time = time_ns() + + async_call(pipeline_simple()) + + spans = self.memory_exporter.get_finished_spans() + self.assertEqual(len(spans), 1) + span = spans[0] + self.assertTrue(span.start_time > coro_created_time) + self.assertTrue(span.end_time < finish_time) + def test_pipeline_traced(self): async def pipeline_simple(): async with self.redis_client.pipeline( @@ -430,6 +533,35 @@ async def pipeline_simple(): ) self.assertEqual(span.attributes.get("db.redis.pipeline_length"), 3) + def test_pipeline_traced_full_time(self): + """Command should be traced for coroutine execution time, not creation time.""" + coro_created_time = None + finish_time = None + + async def pipeline_simple(): + async with self.redis_client.pipeline( + transaction=False + ) as pipeline: + nonlocal coro_created_time + nonlocal finish_time + pipeline.set("blah", 32) + pipeline.rpush("foo", "éé") + pipeline.hgetall("xxx") + + # delay coroutine creation from coroutine execution + coro = pipeline.execute() + coro_created_time = time_ns() + await coro + finish_time = time_ns() + + async_call(pipeline_simple()) + + spans = self.memory_exporter.get_finished_spans() + self.assertEqual(len(spans), 1) + span = spans[0] + self.assertTrue(span.start_time > coro_created_time) + self.assertTrue(span.end_time < finish_time) + def test_parent(self): """Ensure OpenTelemetry works with redis.""" ot_tracer = trace.get_tracer("redis_svc") From a45c9c37924d78419d3482781d17086fad5cc3a8 Mon Sep 17 00:00:00 2001 From: Shalev Roda <65566801+shalevr@users.noreply.github.com> Date: Mon, 26 Jun 2023 09:41:27 +0300 Subject: [PATCH 25/29] Update maintainers list (#1874) --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 09015cdd5f..167a9867d9 100644 --- a/README.md +++ b/README.md @@ -97,7 +97,6 @@ Approvers ([@open-telemetry/python-approvers](https://github.com/orgs/open-telem - [Aaron Abbott](https://github.com/aabmass), Google - [Jeremy Voss](https://github.com/jeremydvoss), Microsoft - [Sanket Mehta](https://github.com/sanketmehta28), Cisco -- [Shalev Roda](https://github.com/shalevr), Cisco Emeritus Approvers: @@ -112,6 +111,7 @@ Maintainers ([@open-telemetry/python-maintainers](https://github.com/orgs/open-t - [Diego Hurtado](https://github.com/ocelotl), Lightstep - [Leighton Chen](https://github.com/lzchen), Microsoft +- [Shalev Roda](https://github.com/shalevr), Cisco Emeritus Maintainers: From 2e49ba1af886289f46e57ae9cf0b1a201f4f3e82 Mon Sep 17 00:00:00 2001 From: Rytis Bagdziunas Date: Tue, 27 Jun 2023 10:37:27 +0200 Subject: [PATCH 26/29] Use a weak reference to sqlalchemy Engine to avoid memory leak (#1771) * Use a weak reference to sqlalchemy Engine to avoid memory leak Closes #1761 By using a weak reference to the `Engine` object, we can avoid the memory leak as disposed `Engines` get properly deallocated. Whenever `SQLAlchemy` is uninstrumented, we only trigger a removal for those event listeners which are listening for objects that haven't been garbage-collected yet. * Made a mistake in resolving the weak reference * Fixed formatting issues * Updated changelog * Added unit test to check that engine was garbage collected * Do not save engine in EngineTracer to avoid memory leak * Add an empty line to satisfy black formatter * Fix isort complaints * Fixed the issue when pool name is not set and =None * Fix formatting issue * Rebased after changes in a recent commit * Updated PR number in changelog --------- Co-authored-by: Shalev Roda <65566801+shalevr@users.noreply.github.com> --- CHANGELOG.md | 2 + .../instrumentation/sqlalchemy/engine.py | 53 ++++++++++++------- .../tests/test_sqlalchemy.py | 23 ++++++++ 3 files changed, 60 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 82fd3a23fe..c5dfd9e601 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,6 +40,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `opentelemetry-instrumentation-system-metrics` Add `process.` prefix to `runtime.memory`, `runtime.cpu.time`, and `runtime.gc_count`. Change `runtime.memory` from count to UpDownCounter. ([#1735](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1735)) - Add request and response hooks for GRPC instrumentation (client only) ([#1706](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1706)) +- Fix memory leak in SQLAlchemy instrumentation where disposed `Engine` does not get garbage collected + ([#1771](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1771) - `opentelemetry-instrumentation-pymemcache` Update instrumentation to support pymemcache >4 ([#1764](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1764)) - `opentelemetry-instrumentation-confluent-kafka` Add support for higher versions of confluent_kafka diff --git a/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py b/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py index 9ff6057728..0e18bc9bed 100644 --- a/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py +++ b/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py @@ -13,6 +13,7 @@ # limitations under the License. import os import re +import weakref from sqlalchemy.event import ( # pylint: disable=no-name-in-module listen, @@ -99,11 +100,11 @@ def __init__( commenter_options=None, ): self.tracer = tracer - self.engine = engine self.connections_usage = connections_usage self.vendor = _normalize_vendor(engine.name) self.enable_commenter = enable_commenter self.commenter_options = commenter_options if commenter_options else {} + self._engine_attrs = _get_attributes_from_engine(engine) self._leading_comment_remover = re.compile(r"^/\*.*?\*/") self._register_event_listener( @@ -118,23 +119,11 @@ def __init__( self._register_event_listener(engine, "checkin", self._pool_checkin) self._register_event_listener(engine, "checkout", self._pool_checkout) - def _get_connection_string(self): - drivername = self.engine.url.drivername or "" - host = self.engine.url.host or "" - port = self.engine.url.port or "" - database = self.engine.url.database or "" - return f"{drivername}://{host}:{port}/{database}" - - def _get_pool_name(self): - if self.engine.pool.logging_name is not None: - return self.engine.pool.logging_name - return self._get_connection_string() - def _add_idle_to_connection_usage(self, value): self.connections_usage.add( value, attributes={ - "pool.name": self._get_pool_name(), + **self._engine_attrs, "state": "idle", }, ) @@ -143,7 +132,7 @@ def _add_used_to_connection_usage(self, value): self.connections_usage.add( value, attributes={ - "pool.name": self._get_pool_name(), + **self._engine_attrs, "state": "used", }, ) @@ -169,12 +158,21 @@ def _pool_checkout( @classmethod def _register_event_listener(cls, target, identifier, func, *args, **kw): listen(target, identifier, func, *args, **kw) - cls._remove_event_listener_params.append((target, identifier, func)) + cls._remove_event_listener_params.append( + (weakref.ref(target), identifier, func) + ) @classmethod def remove_all_event_listeners(cls): - for remove_params in cls._remove_event_listener_params: - remove(*remove_params) + for ( + weak_ref_target, + identifier, + func, + ) in cls._remove_event_listener_params: + # Remove an event listener only if saved weak reference points to an object + # which has not been garbage collected + if weak_ref_target() is not None: + remove(weak_ref_target(), identifier, func) cls._remove_event_listener_params.clear() def _operation_name(self, db_name, statement): @@ -300,3 +298,22 @@ def _get_attributes_from_cursor(vendor, cursor, attrs): if info.port: attrs[SpanAttributes.NET_PEER_PORT] = int(info.port) return attrs + + +def _get_connection_string(engine): + drivername = engine.url.drivername or "" + host = engine.url.host or "" + port = engine.url.port or "" + database = engine.url.database or "" + return f"{drivername}://{host}:{port}/{database}" + + +def _get_attributes_from_engine(engine): + """Set metadata attributes of the database engine""" + attrs = {} + + attrs["pool.name"] = getattr( + getattr(engine, "pool", None), "logging_name", None + ) or _get_connection_string(engine) + + return attrs diff --git a/instrumentation/opentelemetry-instrumentation-sqlalchemy/tests/test_sqlalchemy.py b/instrumentation/opentelemetry-instrumentation-sqlalchemy/tests/test_sqlalchemy.py index 981da107db..8f706ca8c8 100644 --- a/instrumentation/opentelemetry-instrumentation-sqlalchemy/tests/test_sqlalchemy.py +++ b/instrumentation/opentelemetry-instrumentation-sqlalchemy/tests/test_sqlalchemy.py @@ -307,3 +307,26 @@ def test_no_op_tracer_provider(self): cnx.execute("SELECT 1 + 1;").fetchall() spans = self.memory_exporter.get_finished_spans() self.assertEqual(len(spans), 0) + + def test_no_memory_leakage_if_engine_diposed(self): + SQLAlchemyInstrumentor().instrument() + import gc + import weakref + + from sqlalchemy import create_engine + + callback = mock.Mock() + + def make_shortlived_engine(): + engine = create_engine("sqlite:///:memory:") + # Callback will be called if engine is deallocated during garbage + # collection + weakref.finalize(engine, callback) + with engine.connect() as conn: + conn.execute("SELECT 1 + 1;").fetchall() + + for _ in range(0, 5): + make_shortlived_engine() + + gc.collect() + assert callback.call_count == 5 From 79d62b3bcd8b203cdeb4ec55fc4cc89824bbc20d Mon Sep 17 00:00:00 2001 From: Tammy Baylis <96076570+tammy-baylis-swi@users.noreply.github.com> Date: Tue, 27 Jun 2023 03:43:35 -0700 Subject: [PATCH 27/29] sqlalchemy wrap_create_engine now accepts sqlcommenter options (#1873) * sqlalchemy wrap_create_engine accepts sqlcommenter options * Changelog * Lint * Fix default val * Add sqlalchemy tests * Change a default in _instrument get * Lint * More lint * Update default Co-authored-by: Shalev Roda <65566801+shalevr@users.noreply.github.com> * Update args doc * lintttt --------- Co-authored-by: Shalev Roda <65566801+shalevr@users.noreply.github.com> --- CHANGELOG.md | 1 + .../instrumentation/sqlalchemy/__init__.py | 17 +++- .../instrumentation/sqlalchemy/engine.py | 20 +++- .../tests/test_sqlalchemy.py | 97 +++++++++++++++++++ .../tests/test_sqlcommenter.py | 18 ++++ 5 files changed, 146 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c5dfd9e601..afa5e795ef 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ([#1870](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1870)) - Update falcon instrumentation to follow semantic conventions ([#1824](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1824)) +- Fix sqlalchemy instrumentation wrap methods to accept sqlcommenter options([#1873](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1873)) ### Added diff --git a/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/__init__.py b/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/__init__.py index 77db23b417..84eeb59541 100644 --- a/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/__init__.py @@ -134,6 +134,9 @@ def _instrument(self, **kwargs): ``engine``: a SQLAlchemy engine instance ``engines``: a list of SQLAlchemy engine instances ``tracer_provider``: a TracerProvider, defaults to global + ``meter_provider``: a MeterProvider, defaults to global + ``enable_commenter``: bool to enable sqlcommenter, defaults to False + ``commenter_options``: dict of sqlcommenter config, defaults to None Returns: An instrumented engine if passed in as an argument or list of instrumented engines, None otherwise. @@ -151,16 +154,21 @@ def _instrument(self, **kwargs): ) enable_commenter = kwargs.get("enable_commenter", False) + commenter_options = kwargs.get("commenter_options", {}) _w( "sqlalchemy", "create_engine", - _wrap_create_engine(tracer, connections_usage, enable_commenter), + _wrap_create_engine( + tracer, connections_usage, enable_commenter, commenter_options + ), ) _w( "sqlalchemy.engine", "create_engine", - _wrap_create_engine(tracer, connections_usage, enable_commenter), + _wrap_create_engine( + tracer, connections_usage, enable_commenter, commenter_options + ), ) _w( "sqlalchemy.engine.base", @@ -172,7 +180,10 @@ def _instrument(self, **kwargs): "sqlalchemy.ext.asyncio", "create_async_engine", _wrap_create_async_engine( - tracer, connections_usage, enable_commenter + tracer, + connections_usage, + enable_commenter, + commenter_options, ), ) if kwargs.get("engine") is not None: diff --git a/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py b/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py index 0e18bc9bed..1cf980929b 100644 --- a/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py +++ b/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py @@ -43,7 +43,7 @@ def _normalize_vendor(vendor): def _wrap_create_async_engine( - tracer, connections_usage, enable_commenter=False + tracer, connections_usage, enable_commenter=False, commenter_options=None ): # pylint: disable=unused-argument def _wrap_create_async_engine_internal(func, module, args, kwargs): @@ -52,20 +52,32 @@ def _wrap_create_async_engine_internal(func, module, args, kwargs): """ engine = func(*args, **kwargs) EngineTracer( - tracer, engine.sync_engine, connections_usage, enable_commenter + tracer, + engine.sync_engine, + connections_usage, + enable_commenter, + commenter_options, ) return engine return _wrap_create_async_engine_internal -def _wrap_create_engine(tracer, connections_usage, enable_commenter=False): +def _wrap_create_engine( + tracer, connections_usage, enable_commenter=False, commenter_options=None +): def _wrap_create_engine_internal(func, _module, args, kwargs): """Trace the SQLAlchemy engine, creating an `EngineTracer` object that will listen to SQLAlchemy events. """ engine = func(*args, **kwargs) - EngineTracer(tracer, engine, connections_usage, enable_commenter) + EngineTracer( + tracer, + engine, + connections_usage, + enable_commenter, + commenter_options, + ) return engine return _wrap_create_engine_internal diff --git a/instrumentation/opentelemetry-instrumentation-sqlalchemy/tests/test_sqlalchemy.py b/instrumentation/opentelemetry-instrumentation-sqlalchemy/tests/test_sqlalchemy.py index 8f706ca8c8..f729fa6d80 100644 --- a/instrumentation/opentelemetry-instrumentation-sqlalchemy/tests/test_sqlalchemy.py +++ b/instrumentation/opentelemetry-instrumentation-sqlalchemy/tests/test_sqlalchemy.py @@ -12,6 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. import asyncio +import logging from unittest import mock import pytest @@ -176,6 +177,43 @@ def test_create_engine_wrapper(self): "opentelemetry.instrumentation.sqlalchemy", ) + def test_create_engine_wrapper_enable_commenter(self): + logging.getLogger("sqlalchemy.engine").setLevel(logging.INFO) + SQLAlchemyInstrumentor().instrument( + enable_commenter=True, + commenter_options={"db_framework": False}, + ) + from sqlalchemy import create_engine # pylint: disable-all + + engine = create_engine("sqlite:///:memory:") + cnx = engine.connect() + cnx.execute("SELECT 1;").fetchall() + # sqlcommenter + self.assertRegex( + self.caplog.records[-2].getMessage(), + r"SELECT 1 /\*db_driver='(.*)',traceparent='\d{1,2}-[a-zA-Z0-9_]{32}-[a-zA-Z0-9_]{16}-\d{1,2}'\*/;", + ) + + def test_create_engine_wrapper_enable_commenter_otel_values_false(self): + logging.getLogger("sqlalchemy.engine").setLevel(logging.INFO) + SQLAlchemyInstrumentor().instrument( + enable_commenter=True, + commenter_options={ + "db_framework": False, + "opentelemetry_values": False, + }, + ) + from sqlalchemy import create_engine # pylint: disable-all + + engine = create_engine("sqlite:///:memory:") + cnx = engine.connect() + cnx.execute("SELECT 1;").fetchall() + # sqlcommenter + self.assertRegex( + self.caplog.records[-2].getMessage(), + r"SELECT 1 /\*db_driver='(.*)'\*/;", + ) + def test_custom_tracer_provider(self): provider = TracerProvider( resource=Resource.create( @@ -242,6 +280,65 @@ async def run(): asyncio.get_event_loop().run_until_complete(run()) + @pytest.mark.skipif( + not sqlalchemy.__version__.startswith("1.4"), + reason="only run async tests for 1.4", + ) + def test_create_async_engine_wrapper_enable_commenter(self): + async def run(): + logging.getLogger("sqlalchemy.engine").setLevel(logging.INFO) + SQLAlchemyInstrumentor().instrument( + enable_commenter=True, + commenter_options={ + "db_framework": False, + }, + ) + from sqlalchemy.ext.asyncio import ( # pylint: disable-all + create_async_engine, + ) + + engine = create_async_engine("sqlite+aiosqlite:///:memory:") + async with engine.connect() as cnx: + await cnx.execute(sqlalchemy.text("SELECT 1;")) + # sqlcommenter + self.assertRegex( + self.caplog.records[1].getMessage(), + r"SELECT 1 /\*db_driver='(.*)',traceparent='\d{1,2}-[a-zA-Z0-9_]{32}-[a-zA-Z0-9_]{16}-\d{1,2}'\*/;", + ) + + asyncio.get_event_loop().run_until_complete(run()) + + @pytest.mark.skipif( + not sqlalchemy.__version__.startswith("1.4"), + reason="only run async tests for 1.4", + ) + def test_create_async_engine_wrapper_enable_commenter_otel_values_false( + self, + ): + async def run(): + logging.getLogger("sqlalchemy.engine").setLevel(logging.INFO) + SQLAlchemyInstrumentor().instrument( + enable_commenter=True, + commenter_options={ + "db_framework": False, + "opentelemetry_values": False, + }, + ) + from sqlalchemy.ext.asyncio import ( # pylint: disable-all + create_async_engine, + ) + + engine = create_async_engine("sqlite+aiosqlite:///:memory:") + async with engine.connect() as cnx: + await cnx.execute(sqlalchemy.text("SELECT 1;")) + # sqlcommenter + self.assertRegex( + self.caplog.records[1].getMessage(), + r"SELECT 1 /\*db_driver='(.*)'\*/;", + ) + + asyncio.get_event_loop().run_until_complete(run()) + def test_uninstrument(self): engine = create_engine("sqlite:///:memory:") SQLAlchemyInstrumentor().instrument( diff --git a/instrumentation/opentelemetry-instrumentation-sqlalchemy/tests/test_sqlcommenter.py b/instrumentation/opentelemetry-instrumentation-sqlalchemy/tests/test_sqlcommenter.py index 5f9e75a1aa..f13c552bf4 100644 --- a/instrumentation/opentelemetry-instrumentation-sqlalchemy/tests/test_sqlcommenter.py +++ b/instrumentation/opentelemetry-instrumentation-sqlalchemy/tests/test_sqlcommenter.py @@ -56,6 +56,24 @@ def test_sqlcommenter_enabled(self): r"SELECT 1 /\*db_driver='(.*)',traceparent='\d{1,2}-[a-zA-Z0-9_]{32}-[a-zA-Z0-9_]{16}-\d{1,2}'\*/;", ) + def test_sqlcommenter_enabled_otel_values_false(self): + engine = create_engine("sqlite:///:memory:") + SQLAlchemyInstrumentor().instrument( + engine=engine, + tracer_provider=self.tracer_provider, + enable_commenter=True, + commenter_options={ + "db_framework": False, + "opentelemetry_values": False, + }, + ) + cnx = engine.connect() + cnx.execute("SELECT 1;").fetchall() + self.assertRegex( + self.caplog.records[-2].getMessage(), + r"SELECT 1 /\*db_driver='(.*)'\*/;", + ) + def test_sqlcommenter_flask_integration(self): engine = create_engine("sqlite:///:memory:") SQLAlchemyInstrumentor().instrument( From a1f60446721a924d0c5dff8bc422ce93bb15e69b Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Wed, 28 Jun 2023 12:29:01 +0200 Subject: [PATCH 28/29] Add statement of maintainership (#1859) Co-authored-by: Shalev Roda <65566801+shalevr@users.noreply.github.com> --- README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/README.md b/README.md index 167a9867d9..ce1f8f3df4 100644 --- a/README.md +++ b/README.md @@ -49,6 +49,8 @@ depend on `opentelemetry-sdk` or another package that implements the API. **Please note** that these libraries are currently in _beta_, and shouldn't generally be used in production environments. +Unless explicitly stated otherwise, any instrumentation here for a particular library is not developed or maintained by the authors of such library. + The [`instrumentation/`](https://github.com/open-telemetry/opentelemetry-python-contrib/tree/main/instrumentation) directory includes OpenTelemetry instrumentation packages, which can be installed From dadcd01524449ddee07a8d8405890a60caeb8c8e Mon Sep 17 00:00:00 2001 From: Mario Jonke Date: Mon, 3 Jul 2023 23:49:20 +0200 Subject: [PATCH 29/29] urllib3: Add instrumentation support for version 2 (#1879) * urllib3: Add instrumentation support for version 2 * changelog --- CHANGELOG.md | 2 ++ instrumentation/README.md | 2 +- .../pyproject.toml | 2 +- .../opentelemetry/instrumentation/urllib3/package.py | 2 +- .../tests/test_urllib3_integration.py | 4 +++- .../tests/test_urllib3_metrics.py | 2 +- .../opentelemetry/instrumentation/bootstrap_gen.py | 2 +- tox.ini | 12 +++++++----- 8 files changed, 17 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index afa5e795ef..a98a9be176 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ([#1833](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1833)) - Fix elasticsearch `Transport.perform_request` instrument wrap for elasticsearch >= 8 ([#1810](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1810)) +- `opentelemetry-instrumentation-urllib3` Add support for urllib3 version 2 + ([#1879](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1879)) ## Version 1.18.0/0.39b0 (2023-05-10) diff --git a/instrumentation/README.md b/instrumentation/README.md index 933e9b763d..940339a1a1 100644 --- a/instrumentation/README.md +++ b/instrumentation/README.md @@ -41,5 +41,5 @@ | [opentelemetry-instrumentation-tornado](./opentelemetry-instrumentation-tornado) | tornado >= 5.1.1 | Yes | [opentelemetry-instrumentation-tortoiseorm](./opentelemetry-instrumentation-tortoiseorm) | tortoise-orm >= 0.17.0 | No | [opentelemetry-instrumentation-urllib](./opentelemetry-instrumentation-urllib) | urllib | Yes -| [opentelemetry-instrumentation-urllib3](./opentelemetry-instrumentation-urllib3) | urllib3 >= 1.0.0, < 2.0.0 | Yes +| [opentelemetry-instrumentation-urllib3](./opentelemetry-instrumentation-urllib3) | urllib3 >= 1.0.0, < 3.0.0 | Yes | [opentelemetry-instrumentation-wsgi](./opentelemetry-instrumentation-wsgi) | wsgi | Yes \ No newline at end of file diff --git a/instrumentation/opentelemetry-instrumentation-urllib3/pyproject.toml b/instrumentation/opentelemetry-instrumentation-urllib3/pyproject.toml index be52c117f1..167931228a 100644 --- a/instrumentation/opentelemetry-instrumentation-urllib3/pyproject.toml +++ b/instrumentation/opentelemetry-instrumentation-urllib3/pyproject.toml @@ -34,7 +34,7 @@ dependencies = [ [project.optional-dependencies] instruments = [ - "urllib3 >= 1.0.0, < 2.0.0", + "urllib3 >= 1.0.0, < 3.0.0", ] test = [ "opentelemetry-instrumentation-urllib3[instruments]", diff --git a/instrumentation/opentelemetry-instrumentation-urllib3/src/opentelemetry/instrumentation/urllib3/package.py b/instrumentation/opentelemetry-instrumentation-urllib3/src/opentelemetry/instrumentation/urllib3/package.py index 2f5df62de8..9d52db0a1f 100644 --- a/instrumentation/opentelemetry-instrumentation-urllib3/src/opentelemetry/instrumentation/urllib3/package.py +++ b/instrumentation/opentelemetry-instrumentation-urllib3/src/opentelemetry/instrumentation/urllib3/package.py @@ -13,6 +13,6 @@ # limitations under the License. -_instruments = ("urllib3 >= 1.0.0, < 2.0.0",) +_instruments = ("urllib3 >= 1.0.0, < 3.0.0",) _supports_metrics = True diff --git a/instrumentation/opentelemetry-instrumentation-urllib3/tests/test_urllib3_integration.py b/instrumentation/opentelemetry-instrumentation-urllib3/tests/test_urllib3_integration.py index 1082776f9a..7ba7e2731b 100644 --- a/instrumentation/opentelemetry-instrumentation-urllib3/tests/test_urllib3_integration.py +++ b/instrumentation/opentelemetry-instrumentation-urllib3/tests/test_urllib3_integration.py @@ -328,7 +328,9 @@ def response_hook(span, request, response): def test_request_hook_params(self): def request_hook(span, request, headers, body): - span.set_attribute("request_hook_headers", json.dumps(headers)) + span.set_attribute( + "request_hook_headers", json.dumps(dict(headers)) + ) span.set_attribute("request_hook_body", body) URLLib3Instrumentor().uninstrument() diff --git a/instrumentation/opentelemetry-instrumentation-urllib3/tests/test_urllib3_metrics.py b/instrumentation/opentelemetry-instrumentation-urllib3/tests/test_urllib3_metrics.py index 6bf61a9fd8..2fd4cb2c5c 100644 --- a/instrumentation/opentelemetry-instrumentation-urllib3/tests/test_urllib3_metrics.py +++ b/instrumentation/opentelemetry-instrumentation-urllib3/tests/test_urllib3_metrics.py @@ -18,7 +18,7 @@ import httpretty import urllib3 import urllib3.exceptions -from urllib3.request import encode_multipart_formdata +from urllib3 import encode_multipart_formdata from opentelemetry.instrumentation.urllib3 import URLLib3Instrumentor from opentelemetry.test.httptest import HttpTestBase diff --git a/opentelemetry-instrumentation/src/opentelemetry/instrumentation/bootstrap_gen.py b/opentelemetry-instrumentation/src/opentelemetry/instrumentation/bootstrap_gen.py index 1e0b9fd2f5..c67c4642dd 100644 --- a/opentelemetry-instrumentation/src/opentelemetry/instrumentation/bootstrap_gen.py +++ b/opentelemetry-instrumentation/src/opentelemetry/instrumentation/bootstrap_gen.py @@ -161,7 +161,7 @@ "instrumentation": "opentelemetry-instrumentation-tortoiseorm==0.40b0.dev", }, "urllib3": { - "library": "urllib3 >= 1.0.0, < 2.0.0", + "library": "urllib3 >= 1.0.0, < 3.0.0", "instrumentation": "opentelemetry-instrumentation-urllib3==0.40b0.dev", }, } diff --git a/tox.ini b/tox.ini index c7d4c56193..b0d33dfebc 100644 --- a/tox.ini +++ b/tox.ini @@ -92,8 +92,8 @@ envlist = pypy3-test-instrumentation-urllib ; opentelemetry-instrumentation-urllib3 - py3{7,8,9,10,11}-test-instrumentation-urllib3 - ;pypy3-test-instrumentation-urllib3 + py3{7,8,9,10,11}-test-instrumentation-urllib3v{1,2} + ;pypy3-test-instrumentation-urllib3v{1,2} ; opentelemetry-instrumentation-requests py3{7,8,9,10,11}-test-instrumentation-requests @@ -280,6 +280,8 @@ deps = httpx18: respx~=0.17.0 httpx21: httpx>=0.19.0 httpx21: respx~=0.20.1 + urllib3v1: urllib3 >=1.0.0,<2.0.0 + urllib3v2: urllib3 >=2.0.0,<3.0.0 ; FIXME: add coverage testing ; FIXME: add mypy testing @@ -310,7 +312,7 @@ changedir = test-instrumentation-fastapi: instrumentation/opentelemetry-instrumentation-fastapi/tests test-instrumentation-flask{213,220}: instrumentation/opentelemetry-instrumentation-flask/tests test-instrumentation-urllib: instrumentation/opentelemetry-instrumentation-urllib/tests - test-instrumentation-urllib3: instrumentation/opentelemetry-instrumentation-urllib3/tests + test-instrumentation-urllib3v{1,2}: instrumentation/opentelemetry-instrumentation-urllib3/tests test-instrumentation-grpc: instrumentation/opentelemetry-instrumentation-grpc/tests test-instrumentation-jinja2: instrumentation/opentelemetry-instrumentation-jinja2/tests test-instrumentation-kafka-python: instrumentation/opentelemetry-instrumentation-kafka-python/tests @@ -369,7 +371,7 @@ commands_pre = grpc: pip install {toxinidir}/instrumentation/opentelemetry-instrumentation-grpc[test] - falcon{1,2,3},flask{213,220},django{1,2,3,4},pyramid,tornado,starlette,fastapi,aiohttp,asgi,requests,urllib,urllib3,wsgi: pip install {toxinidir}/util/opentelemetry-util-http[test] + falcon{1,2,3},flask{213,220},django{1,2,3,4},pyramid,tornado,starlette,fastapi,aiohttp,asgi,requests,urllib,urllib3v{1,2},wsgi: pip install {toxinidir}/util/opentelemetry-util-http[test] wsgi,falcon{1,2,3},flask{213,220},django{1,2,3,4},pyramid: pip install {toxinidir}/instrumentation/opentelemetry-instrumentation-wsgi[test] asgi,django{3,4},starlette,fastapi: pip install {toxinidir}/instrumentation/opentelemetry-instrumentation-asgi[test] @@ -388,7 +390,7 @@ commands_pre = urllib: pip install {toxinidir}/instrumentation/opentelemetry-instrumentation-urllib[test] - urllib3: pip install {toxinidir}/instrumentation/opentelemetry-instrumentation-urllib3[test] + urllib3v{1,2}: pip install {toxinidir}/instrumentation/opentelemetry-instrumentation-urllib3[test] botocore: pip install {toxinidir}/instrumentation/opentelemetry-instrumentation-botocore[test]