diff --git a/CHANGELOG.md b/CHANGELOG.md index 41bfbd0a4f..dd39f86522 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- `opentelemetry-instrumentation-httpx`: fix missing metric response attributes when tracing is disabled + ([#3615](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3615)) - `opentelemetry-instrumentation-asgi`: fix excluded_urls in instrumentation-asgi ([#3567](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3567)) - `opentelemetry-resource-detector-containerid`: make it more quiet on platforms without cgroups 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 d3d73a1e08..c93f9b71c5 100644 --- a/instrumentation/opentelemetry-instrumentation-httpx/src/opentelemetry/instrumentation/httpx/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-httpx/src/opentelemetry/instrumentation/httpx/__init__.py @@ -417,7 +417,6 @@ def _apply_request_client_attributes_to_span( def _apply_response_client_attributes_to_span( span: Span, - metric_attributes: dict[str, typing.Any], status_code: int, http_version: str, semconv: _StabilityMode, @@ -433,6 +432,30 @@ def _apply_response_client_attributes_to_span( http_status_code = http_status_to_status_code(status_code) span.set_status(http_status_code) + if http_status_code == StatusCode.ERROR and _report_new(semconv): + # http semconv transition: new error.type + span_attributes[ERROR_TYPE] = str(status_code) + + if http_version and _report_new(semconv): + # http semconv transition: http.flavor -> network.protocol.version + _set_http_network_protocol_version( + span_attributes, + http_version.replace("HTTP/", ""), + semconv, + ) + + for key, val in span_attributes.items(): + span.set_attribute(key, val) + + +def _apply_response_client_attributes_to_metrics( + span: Span | None, + metric_attributes: dict[str, typing.Any], + status_code: int, + http_version: str, + semconv: _StabilityMode, +) -> None: + """Apply response attributes to metric attributes.""" # Set HTTP status code in metric attributes _set_status( span, @@ -443,26 +466,12 @@ def _apply_response_client_attributes_to_span( sem_conv_opt_in_mode=semconv, ) - if http_status_code == StatusCode.ERROR and _report_new(semconv): - # http semconv transition: new error.type - span_attributes[ERROR_TYPE] = str(status_code) - if http_version and _report_new(semconv): - # http semconv transition: http.flavor -> network.protocol.version _set_http_network_protocol_version( metric_attributes, http_version.replace("HTTP/", ""), semconv, ) - if _report_new(semconv): - _set_http_network_protocol_version( - span_attributes, - http_version.replace("HTTP/", ""), - semconv, - ) - - for key, val in span_attributes.items(): - span.set_attribute(key, val) class SyncOpenTelemetryTransport(httpx.BaseTransport): @@ -592,11 +601,19 @@ def handle_request( _extract_response(response) ) + # Always apply response attributes to metrics + _apply_response_client_attributes_to_metrics( + span, + metric_attributes, + status_code, + http_version, + self._sem_conv_opt_in_mode, + ) + if span.is_recording(): # apply http client response attributes according to semconv _apply_response_client_attributes_to_span( span, - metric_attributes, status_code, http_version, self._sem_conv_opt_in_mode, @@ -777,11 +794,19 @@ async def handle_async_request( _extract_response(response) ) + # Always apply response attributes to metrics + _apply_response_client_attributes_to_metrics( + span, + metric_attributes, + status_code, + http_version, + self._sem_conv_opt_in_mode, + ) + if span.is_recording(): # apply http client response attributes according to semconv _apply_response_client_attributes_to_span( span, - metric_attributes, status_code, http_version, self._sem_conv_opt_in_mode, @@ -1001,11 +1026,19 @@ def _handle_request_wrapper( # pylint: disable=too-many-locals _extract_response(response) ) + # Always apply response attributes to metrics + _apply_response_client_attributes_to_metrics( + span, + metric_attributes, + status_code, + http_version, + sem_conv_opt_in_mode, + ) + if span.is_recording(): # apply http client response attributes according to semconv _apply_response_client_attributes_to_span( span, - metric_attributes, status_code, http_version, sem_conv_opt_in_mode, @@ -1109,11 +1142,19 @@ async def _handle_async_request_wrapper( # pylint: disable=too-many-locals _extract_response(response) ) + # Always apply response attributes to metrics + _apply_response_client_attributes_to_metrics( + span, + metric_attributes, + status_code, + http_version, + sem_conv_opt_in_mode, + ) + if span.is_recording(): # apply http client response attributes according to semconv _apply_response_client_attributes_to_span( span, - metric_attributes, status_code, http_version, sem_conv_opt_in_mode, diff --git a/instrumentation/opentelemetry-instrumentation-httpx/tests/test_httpx_integration.py b/instrumentation/opentelemetry-instrumentation-httpx/tests/test_httpx_integration.py index 28185b9336..2812aa7a2f 100644 --- a/instrumentation/opentelemetry-instrumentation-httpx/tests/test_httpx_integration.py +++ b/instrumentation/opentelemetry-instrumentation-httpx/tests/test_httpx_integration.py @@ -39,6 +39,18 @@ from opentelemetry.instrumentation.utils import suppress_http_instrumentation from opentelemetry.propagate import get_global_textmap, set_global_textmap from opentelemetry.sdk import resources +from opentelemetry.semconv._incubating.attributes.http_attributes import ( + HTTP_FLAVOR, + HTTP_HOST, + HTTP_METHOD, + HTTP_SCHEME, + HTTP_STATUS_CODE, + HTTP_URL, +) +from opentelemetry.semconv._incubating.attributes.net_attributes import ( + NET_PEER_NAME, + NET_PEER_PORT, +) from opentelemetry.semconv.attributes.error_attributes import ERROR_TYPE from opentelemetry.semconv.attributes.http_attributes import ( HTTP_REQUEST_METHOD, @@ -55,7 +67,7 @@ SERVER_PORT, ) from opentelemetry.semconv.attributes.url_attributes import URL_FULL -from opentelemetry.semconv.trace import SpanAttributes +from opentelemetry.semconv.schemas import Schemas from opentelemetry.test.mock_textmap import MockTextMapPropagator from opentelemetry.test.test_base import TestBase from opentelemetry.trace import StatusCode @@ -76,6 +88,7 @@ HTTP_RESPONSE_BODY = "http.response.body" +SCHEMA_URL = Schemas.V1_21_0.value def _is_url_tuple(request: "RequestInfo"): @@ -216,9 +229,9 @@ def test_basic(self): self.assertEqual( dict(span.attributes), { - SpanAttributes.HTTP_METHOD: "GET", - SpanAttributes.HTTP_URL: self.URL, - SpanAttributes.HTTP_STATUS_CODE: 200, + HTTP_METHOD: "GET", + HTTP_URL: self.URL, + HTTP_STATUS_CODE: 200, }, ) @@ -237,9 +250,9 @@ def test_basic_metrics(self): self.assertEqual( dict(duration_data_point.attributes), { - SpanAttributes.HTTP_STATUS_CODE: 200, - SpanAttributes.HTTP_METHOD: "GET", - SpanAttributes.HTTP_SCHEME: "http", + HTTP_STATUS_CODE: 200, + HTTP_METHOD: "GET", + HTTP_SCHEME: "http", }, ) self.assertEqual(duration_data_point.count, 1) @@ -263,9 +276,9 @@ def test_nonstandard_http_method(self): self.assertEqual( dict(span.attributes), { - SpanAttributes.HTTP_METHOD: "_OTHER", - SpanAttributes.HTTP_URL: self.URL, - SpanAttributes.HTTP_STATUS_CODE: 405, + HTTP_METHOD: "_OTHER", + HTTP_URL: self.URL, + HTTP_STATUS_CODE: 405, }, ) @@ -282,9 +295,9 @@ def test_nonstandard_http_method(self): self.assertEqual( dict(duration_data_point.attributes), { - SpanAttributes.HTTP_STATUS_CODE: 405, - SpanAttributes.HTTP_METHOD: "_OTHER", - SpanAttributes.HTTP_SCHEME: "http", + HTTP_STATUS_CODE: 405, + HTTP_METHOD: "_OTHER", + HTTP_SCHEME: "http", }, ) @@ -354,7 +367,7 @@ def test_basic_new_semconv(self): self.assertEqual( span.instrumentation_scope.schema_url, - SpanAttributes.SCHEMA_URL, + SCHEMA_URL, ) self.assertEqual( dict(span.attributes), @@ -404,23 +417,23 @@ def test_basic_both_semconv(self): self.assertEqual( span.instrumentation_scope.schema_url, - SpanAttributes.SCHEMA_URL, + SCHEMA_URL, ) self.assertEqual( dict(span.attributes), { - SpanAttributes.HTTP_METHOD: "GET", + HTTP_METHOD: "GET", HTTP_REQUEST_METHOD: "GET", - SpanAttributes.HTTP_URL: url, + HTTP_URL: url, URL_FULL: url, - SpanAttributes.HTTP_HOST: "mock", + HTTP_HOST: "mock", SERVER_ADDRESS: "mock", NETWORK_PEER_ADDRESS: "mock", - SpanAttributes.NET_PEER_PORT: 8080, - SpanAttributes.HTTP_STATUS_CODE: 200, + NET_PEER_PORT: 8080, + HTTP_STATUS_CODE: 200, HTTP_RESPONSE_STATUS_CODE: 200, - SpanAttributes.HTTP_FLAVOR: "1.1", + HTTP_FLAVOR: "1.1", NETWORK_PROTOCOL_VERSION: "1.1", SERVER_PORT: 8080, NETWORK_PEER_PORT: 8080, @@ -440,13 +453,13 @@ def test_basic_both_semconv(self): self.assertEqual( dict(metrics[0].data.data_points[0].attributes), { - SpanAttributes.HTTP_FLAVOR: "1.1", - SpanAttributes.HTTP_HOST: "mock", - SpanAttributes.HTTP_METHOD: "GET", - SpanAttributes.HTTP_SCHEME: "http", - SpanAttributes.NET_PEER_NAME: "mock", - SpanAttributes.NET_PEER_PORT: 8080, - SpanAttributes.HTTP_STATUS_CODE: 200, + HTTP_FLAVOR: "1.1", + HTTP_HOST: "mock", + HTTP_METHOD: "GET", + HTTP_SCHEME: "http", + NET_PEER_NAME: "mock", + NET_PEER_PORT: 8080, + HTTP_STATUS_CODE: 200, }, ) self.assertEqual(metrics[0].name, "http.client.duration") @@ -477,9 +490,7 @@ def test_not_foundbasic(self): self.assertEqual(result.status_code, 404) span = self.assert_span() - self.assertEqual( - span.attributes.get(SpanAttributes.HTTP_STATUS_CODE), 404 - ) + self.assertEqual(span.attributes.get(HTTP_STATUS_CODE), 404) self.assertIs( span.status.status_code, trace.StatusCode.ERROR, @@ -489,9 +500,7 @@ def test_not_foundbasic(self): self.assertEqual(len(metrics), 1) duration_data_point = metrics[0].data.data_points[0] self.assertEqual( - duration_data_point.attributes.get( - SpanAttributes.HTTP_STATUS_CODE - ), + duration_data_point.attributes.get(HTTP_STATUS_CODE), 404, ) @@ -535,9 +544,7 @@ def test_not_foundbasic_both_semconv(self): self.assertEqual(result.status_code, 404) span = self.assert_span() - self.assertEqual( - span.attributes.get(SpanAttributes.HTTP_STATUS_CODE), 404 - ) + self.assertEqual(span.attributes.get(HTTP_STATUS_CODE), 404) self.assertEqual( span.attributes.get(HTTP_RESPONSE_STATUS_CODE), 404 ) @@ -554,7 +561,7 @@ def test_not_foundbasic_both_semconv(self): self.assertEqual( metrics[0] .data.data_points[0] - .attributes.get(SpanAttributes.HTTP_STATUS_CODE), + .attributes.get(HTTP_STATUS_CODE), 404, ) self.assertEqual( @@ -612,9 +619,9 @@ def test_requests_500_error(self): self.assertEqual( dict(span.attributes), { - SpanAttributes.HTTP_METHOD: "GET", - SpanAttributes.HTTP_URL: self.URL, - SpanAttributes.HTTP_STATUS_CODE: 500, + HTTP_METHOD: "GET", + HTTP_URL: self.URL, + HTTP_STATUS_CODE: 500, }, ) self.assertEqual(span.status.status_code, StatusCode.ERROR) @@ -689,14 +696,14 @@ def test_requests_timeout_exception_both_semconv(self): self.assertEqual( dict(span.attributes), { - SpanAttributes.HTTP_METHOD: "GET", + HTTP_METHOD: "GET", HTTP_REQUEST_METHOD: "GET", - SpanAttributes.HTTP_URL: url, + HTTP_URL: url, URL_FULL: url, - SpanAttributes.HTTP_HOST: "mock", + HTTP_HOST: "mock", SERVER_ADDRESS: "mock", NETWORK_PEER_ADDRESS: "mock", - SpanAttributes.NET_PEER_PORT: 8080, + NET_PEER_PORT: 8080, SERVER_PORT: 8080, NETWORK_PEER_PORT: 8080, ERROR_TYPE: "TimeoutException", @@ -722,10 +729,8 @@ def test_invalid_url(self): span = self.assert_span() self.assertEqual(span.name, "POST") - self.assertEqual( - span.attributes[SpanAttributes.HTTP_METHOD], "POST" - ) - self.assertEqual(span.attributes[SpanAttributes.HTTP_URL], url) + self.assertEqual(span.attributes[HTTP_METHOD], "POST") + self.assertEqual(span.attributes[HTTP_URL], url) self.assertEqual(span.status.status_code, StatusCode.ERROR) def test_if_headers_equals_none(self): @@ -790,6 +795,85 @@ def test_custom_meter_provider(self): ) self.assertEqual(data_point.count, 1) + def _run_disabled_tracing_metrics_attributes_test( + self, + url: str, + expected_attributes: dict, + expected_number_of_metrics: int = 1, + ) -> None: + with mock.patch("opentelemetry.trace.INVALID_SPAN") as mock_span: + client = self.create_client( + self.create_transport( + tracer_provider=trace.NoOpTracerProvider() + ) + ) + mock_span.is_recording.return_value = False + self.perform_request(url, client=client) + + self.assertFalse(mock_span.is_recording()) + self.assertTrue(mock_span.is_recording.called) + + metrics = self.assert_metrics( + num_metrics=expected_number_of_metrics + ) + duration_data_point = metrics[0].data.data_points[0] + + self.assertEqual(duration_data_point.count, 1) + self.assertEqual( + dict(duration_data_point.attributes), + expected_attributes, + ) + + def test_metrics_have_response_attributes_with_disabled_tracing( + self, + ) -> None: + """Test that metrics have response attributes when tracing is disabled for old + semantic conventions.""" + self._run_disabled_tracing_metrics_attributes_test( + url="http://mock:8080/status/200", + expected_attributes={ + HTTP_STATUS_CODE: 200, + HTTP_METHOD: "GET", + HTTP_SCHEME: "http", + }, + expected_number_of_metrics=1, + ) + + def test_metrics_have_response_attributes_with_disabled_tracing_both_semconv( + self, + ) -> None: + """Test that metrics have response attributes when tracing is disabled for both + semantic conventions.""" + self._run_disabled_tracing_metrics_attributes_test( + url="http://mock:8080/status/200", + expected_attributes={ + HTTP_STATUS_CODE: 200, + HTTP_METHOD: "GET", + HTTP_SCHEME: "http", + HTTP_FLAVOR: "1.1", + HTTP_HOST: "mock", + NET_PEER_NAME: "mock", + NET_PEER_PORT: 8080, + }, + expected_number_of_metrics=2, + ) + + def test_metrics_have_response_attributes_with_disabled_tracing_new_semconv( + self, + ) -> None: + """Test that metrics have response attributes when tracing is disabled with new semantic conventions.""" + self._run_disabled_tracing_metrics_attributes_test( + url="http://mock:8080/status/200", + expected_attributes={ + SERVER_ADDRESS: "mock", + HTTP_REQUEST_METHOD: "GET", + HTTP_RESPONSE_STATUS_CODE: 200, + NETWORK_PROTOCOL_VERSION: "1.1", + SERVER_PORT: 8080, + }, + expected_number_of_metrics=1, + ) + def test_response_hook(self): transport = self.create_transport( tracer_provider=self.tracer_provider, @@ -803,9 +887,9 @@ def test_response_hook(self): self.assertEqual( dict(span.attributes), { - SpanAttributes.HTTP_METHOD: "GET", - SpanAttributes.HTTP_URL: self.URL, - SpanAttributes.HTTP_STATUS_CODE: 200, + HTTP_METHOD: "GET", + HTTP_URL: self.URL, + HTTP_STATUS_CODE: 200, HTTP_RESPONSE_BODY: "Hello!", }, ) @@ -885,12 +969,8 @@ def test_client_mounts_with_instrumented_transport(self): self.perform_request(self.URL, client=client1) self.perform_request(https_url, client=client2) spans = self.assert_span(num_spans=2) - self.assertEqual( - spans[0].attributes[SpanAttributes.HTTP_URL], self.URL - ) - self.assertEqual( - spans[1].attributes[SpanAttributes.HTTP_URL], https_url - ) + self.assertEqual(spans[0].attributes[HTTP_URL], self.URL) + self.assertEqual(spans[1].attributes[HTTP_URL], https_url) @mock.patch.dict("os.environ", {"NO_PROXY": ""}, clear=True) class BaseInstrumentorTest(BaseTest, metaclass=abc.ABCMeta): @@ -982,9 +1062,9 @@ def test_response_hook(self): self.assertEqual( dict(span.attributes), { - SpanAttributes.HTTP_METHOD: "GET", - SpanAttributes.HTTP_URL: self.URL, - SpanAttributes.HTTP_STATUS_CODE: 200, + HTTP_METHOD: "GET", + HTTP_URL: self.URL, + HTTP_STATUS_CODE: 200, HTTP_RESPONSE_BODY: "Hello!", }, ) @@ -1003,9 +1083,9 @@ def test_response_hook_sync_async_kwargs(self): self.assertEqual( dict(span.attributes), { - SpanAttributes.HTTP_METHOD: "GET", - SpanAttributes.HTTP_URL: self.URL, - SpanAttributes.HTTP_STATUS_CODE: 200, + HTTP_METHOD: "GET", + HTTP_URL: self.URL, + HTTP_STATUS_CODE: 200, HTTP_RESPONSE_BODY: "Hello!", }, ) @@ -1108,7 +1188,7 @@ def test_instrumentation_without_client(self): with self.subTest(idx=idx, res=res): self.assertEqual(res.text, "Hello!") self.assertEqual( - spans[idx].attributes[SpanAttributes.HTTP_URL], + spans[idx].attributes[HTTP_URL], self.URL, ) @@ -1306,12 +1386,12 @@ def test_remove_sensitive_params(self): self.perform_request(new_url) span = self.assert_span() - actual_url = span.attributes[SpanAttributes.HTTP_URL] + actual_url = span.attributes[HTTP_URL] if "@" in actual_url: # If credentials are present, they must be redacted self.assertEqual( - span.attributes[SpanAttributes.HTTP_URL], + span.attributes[HTTP_URL], "http://REDACTED:REDACTED@mock/status/200?sig=REDACTED", ) else: @@ -1392,11 +1472,11 @@ def test_remove_sensitive_params(self): self.perform_request(new_url) span = self.assert_span() - actual_url = span.attributes[SpanAttributes.HTTP_URL] + actual_url = span.attributes[HTTP_URL] if "@" in actual_url: self.assertEqual( - span.attributes[SpanAttributes.HTTP_URL], + span.attributes[HTTP_URL], "http://REDACTED:REDACTED@mock/status/200?Signature=REDACTED", ) else: @@ -1509,9 +1589,9 @@ def test_async_response_hook_does_nothing_if_not_coroutine(self): self.assertEqual( dict(span.attributes), { - SpanAttributes.HTTP_METHOD: "GET", - SpanAttributes.HTTP_URL: self.URL, - SpanAttributes.HTTP_STATUS_CODE: 200, + HTTP_METHOD: "GET", + HTTP_URL: self.URL, + HTTP_STATUS_CODE: 200, }, )