From dc9f7f7e6cc7351d3948b2b374ef09d79a07464e Mon Sep 17 00:00:00 2001 From: emdneto <9735060+emdneto@users.noreply.github.com> Date: Mon, 3 Mar 2025 18:30:25 -0300 Subject: [PATCH 1/5] show test fail at main Signed-off-by: emdneto <9735060+emdneto@users.noreply.github.com> --- .../tests/test_requests_integration.py | 39 +++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/instrumentation/opentelemetry-instrumentation-requests/tests/test_requests_integration.py b/instrumentation/opentelemetry-instrumentation-requests/tests/test_requests_integration.py index 366cd0c233..6cd75b8a36 100644 --- a/instrumentation/opentelemetry-instrumentation-requests/tests/test_requests_integration.py +++ b/instrumentation/opentelemetry-instrumentation-requests/tests/test_requests_integration.py @@ -832,3 +832,42 @@ def test_basic_metric_both_semconv(self): dict(data_point.attributes), ) self.assertEqual(data_point.count, 1) + + def test_basic_metric_non_recording_span(self): + # tracer_provider = trace.NoOpTracerProvider() + # trace.set_tracer_provider(tracer_provider=tracer_provider) + + expected_attributes = { + SpanAttributes.HTTP_STATUS_CODE: 200, + SpanAttributes.HTTP_HOST: "examplehost", + SpanAttributes.NET_PEER_PORT: 8000, + SpanAttributes.NET_PEER_NAME: "examplehost", + SpanAttributes.HTTP_METHOD: "GET", + SpanAttributes.HTTP_FLAVOR: "1.1", + SpanAttributes.HTTP_SCHEME: "http", + } + + with mock.patch("opentelemetry.trace.INVALID_SPAN") as mock_span: + RequestsInstrumentor().uninstrument() + RequestsInstrumentor().instrument( + tracer_provider=trace.NoOpTracerProvider() + ) + mock_span.is_recording.return_value = False + result = self.perform_request(self.URL) + self.assertEqual(result.text, "Hello!") + + # self.assert_span(None, 0) + self.assertFalse(mock_span.is_recording()) + self.assertTrue(mock_span.is_recording.called) + self.assertFalse(mock_span.set_attribute.called) + self.assertFalse(mock_span.set_status.called) + 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: + for point in list(metric.data.data_points): + self.assertDictEqual( + expected_attributes, dict(point.attributes) + ) + self.assertEqual(point.count, 1) From eec33fc01c0baea64a4a05682e4e50d8d592a83e Mon Sep 17 00:00:00 2001 From: emdneto <9735060+emdneto@users.noreply.github.com> Date: Mon, 3 Mar 2025 18:48:10 -0300 Subject: [PATCH 2/5] implement fix -- ci should pass now Signed-off-by: emdneto <9735060+emdneto@users.noreply.github.com> --- CHANGELOG.md | 6 +- .../instrumentation/requests/__init__.py | 55 +++++++++++-------- 2 files changed, 37 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a8ebe71151..cdf48ec76c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,7 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `opentelemetry-instrumentation-openai-v2` Update doc for OpenAI Instrumentation to support OpenAI Compatible Platforms ([#3279](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3279)) -- `opentelemetry-instrumentation-system-metrics` Add `process` metrics and deprecated `process.runtime` prefixed ones +- `opentelemetry-instrumentation-system-metrics` Add `process` metrics and deprecated `process.runtime` prefixed ones ([#3250](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3250)) - `opentelemetry-instrumentation-botocore` Add support for GenAI user events and lazy initialize tracer ([#3258](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3258)) @@ -37,6 +37,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ([#3247](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3247)) - `opentelemetry-instrumentation-asyncpg` Fix fallback for empty queries. ([#3253](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3253)) +- `opentelemetry-instrumentation-requests` always record span status code in duration metric + ([#3323](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3323)) ## Version 1.30.0/0.51b0 (2025-02-03) @@ -96,7 +98,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Breaking changes -- `opentelemetry-exporter-prometheus-remote-write` updated protobuf required version from 4.21 to 5.26 and regenerated protobufs +- `opentelemetry-exporter-prometheus-remote-write` updated protobuf required version from 4.21 to 5.26 and regenerated protobufs ([#3219](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3219)) - `opentelemetry-instrumentation-sqlalchemy` including sqlcomment in `db.statement` span attribute value is now opt-in ([#3112](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3112)) 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 6c197d530e..ad72603ce3 100644 --- a/instrumentation/opentelemetry-instrumentation-requests/src/opentelemetry/instrumentation/requests/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-requests/src/opentelemetry/instrumentation/requests/__init__.py @@ -99,15 +99,14 @@ def response_hook(span, request_obj, response) _set_http_network_protocol_version, _set_http_peer_port_client, _set_http_scheme, - _set_http_status_code, _set_http_url, + _set_status, _StabilityMode, ) from opentelemetry.instrumentation.instrumentor import BaseInstrumentor from opentelemetry.instrumentation.requests.package import _instruments from opentelemetry.instrumentation.requests.version import __version__ from opentelemetry.instrumentation.utils import ( - http_status_to_status_code, is_http_instrumentation_enabled, suppress_http_instrumentation, ) @@ -124,7 +123,6 @@ def response_hook(span, request_obj, response) ) from opentelemetry.trace import SpanKind, Tracer, get_tracer from opentelemetry.trace.span import Span -from opentelemetry.trace.status import StatusCode from opentelemetry.util.http import ( ExcludeList, get_excluded_urls, @@ -140,6 +138,32 @@ def response_hook(span, request_obj, response) _ResponseHookT = Optional[Callable[[Span, PreparedRequest, Response], None]] +def _set_http_status_code_attribute( + span, + status_code, + metric_attributes=None, + sem_conv_opt_in_mode=_StabilityMode.DEFAULT, +): + status_code_str = str(status_code) + try: + status_code = int(status_code) + except ValueError: + status_code = -1 + if metric_attributes is None: + metric_attributes = {} + # When we have durations we should set metrics only once + # Also the decision to include status code on a histogram should + # not be dependent on tracing decisions. + _set_status( + span, + metric_attributes, + status_code, + status_code_str, + server_span=False, + sem_conv_opt_in_mode=sem_conv_opt_in_mode, + ) + + # pylint: disable=unused-argument # pylint: disable=R0915 def _instrument( @@ -267,25 +291,12 @@ def get_or_create_headers(): if isinstance(result, Response): span_attributes = {} - if span.is_recording(): - _set_http_status_code( - span_attributes, - result.status_code, - sem_conv_opt_in_mode, - ) - _set_http_status_code( - metric_labels, result.status_code, sem_conv_opt_in_mode - ) - status_code = http_status_to_status_code( - result.status_code - ) - span.set_status(status_code) - if ( - _report_new(sem_conv_opt_in_mode) - and status_code is StatusCode.ERROR - ): - span_attributes[ERROR_TYPE] = str(result.status_code) - metric_labels[ERROR_TYPE] = str(result.status_code) + _set_http_status_code_attribute( + span, + result.status_code, + metric_labels, + sem_conv_opt_in_mode, + ) if result.raw is not None: version = getattr(result.raw, "version", None) From fabd6074aa14d343c7294f70485cdb633049bc27 Mon Sep 17 00:00:00 2001 From: emdneto <9735060+emdneto@users.noreply.github.com> Date: Mon, 3 Mar 2025 18:50:33 -0300 Subject: [PATCH 3/5] remove uneeded comment Signed-off-by: emdneto <9735060+emdneto@users.noreply.github.com> --- .../tests/test_requests_integration.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-requests/tests/test_requests_integration.py b/instrumentation/opentelemetry-instrumentation-requests/tests/test_requests_integration.py index 6cd75b8a36..23d45aa2de 100644 --- a/instrumentation/opentelemetry-instrumentation-requests/tests/test_requests_integration.py +++ b/instrumentation/opentelemetry-instrumentation-requests/tests/test_requests_integration.py @@ -834,9 +834,6 @@ def test_basic_metric_both_semconv(self): self.assertEqual(data_point.count, 1) def test_basic_metric_non_recording_span(self): - # tracer_provider = trace.NoOpTracerProvider() - # trace.set_tracer_provider(tracer_provider=tracer_provider) - expected_attributes = { SpanAttributes.HTTP_STATUS_CODE: 200, SpanAttributes.HTTP_HOST: "examplehost", From 0a700b26b1e4c68e0aaeb552bb358345f5b0dabe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Em=C3=ADdio=20Neto?= <9735060+emdneto@users.noreply.github.com> Date: Mon, 3 Mar 2025 19:13:00 -0300 Subject: [PATCH 4/5] Update instrumentation/opentelemetry-instrumentation-requests/tests/test_requests_integration.py --- .../tests/test_requests_integration.py | 1 - 1 file changed, 1 deletion(-) diff --git a/instrumentation/opentelemetry-instrumentation-requests/tests/test_requests_integration.py b/instrumentation/opentelemetry-instrumentation-requests/tests/test_requests_integration.py index 23d45aa2de..ca5b3e953f 100644 --- a/instrumentation/opentelemetry-instrumentation-requests/tests/test_requests_integration.py +++ b/instrumentation/opentelemetry-instrumentation-requests/tests/test_requests_integration.py @@ -853,7 +853,6 @@ def test_basic_metric_non_recording_span(self): result = self.perform_request(self.URL) self.assertEqual(result.text, "Hello!") - # self.assert_span(None, 0) self.assertFalse(mock_span.is_recording()) self.assertTrue(mock_span.is_recording.called) self.assertFalse(mock_span.set_attribute.called) From b1308a058089e517fc642680a2599f8a711fa571 Mon Sep 17 00:00:00 2001 From: emdneto <9735060+emdneto@users.noreply.github.com> Date: Wed, 5 Mar 2025 16:57:39 -0300 Subject: [PATCH 5/5] use get_sorted_metrics Signed-off-by: emdneto <9735060+emdneto@users.noreply.github.com> --- .../tests/test_requests_integration.py | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-requests/tests/test_requests_integration.py b/instrumentation/opentelemetry-instrumentation-requests/tests/test_requests_integration.py index ca5b3e953f..efe9f75f56 100644 --- a/instrumentation/opentelemetry-instrumentation-requests/tests/test_requests_integration.py +++ b/instrumentation/opentelemetry-instrumentation-requests/tests/test_requests_integration.py @@ -852,18 +852,14 @@ def test_basic_metric_non_recording_span(self): mock_span.is_recording.return_value = False result = self.perform_request(self.URL) self.assertEqual(result.text, "Hello!") - self.assertFalse(mock_span.is_recording()) self.assertTrue(mock_span.is_recording.called) self.assertFalse(mock_span.set_attribute.called) self.assertFalse(mock_span.set_status.called) - 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: - for point in list(metric.data.data_points): - self.assertDictEqual( - expected_attributes, dict(point.attributes) - ) - self.assertEqual(point.count, 1) + metrics = self.get_sorted_metrics() + self.assertEqual(len(metrics), 1) + duration_data_point = metrics[0].data.data_points[0] + self.assertDictEqual( + expected_attributes, dict(duration_data_point.attributes) + ) + self.assertEqual(duration_data_point.count, 1)