requests: always record span status code in duration metrics (open-telemetry#3323)

* show test fail at main

Signed-off-by: emdneto <[email protected]>

* implement fix -- ci should pass now

Signed-off-by: emdneto <[email protected]>

* remove uneeded comment

Signed-off-by: emdneto <[email protected]>

* Update instrumentation/opentelemetry-instrumentation-requests/tests/test_requests_integration.py

* use get_sorted_metrics

Signed-off-by: emdneto <[email protected]>

---------

Signed-off-by: emdneto <[email protected]>
Co-authored-by: Leighton Chen <[email protected]>
Co-authored-by: Shalev Roda <[email protected]>

Author: 3 people

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))
@@ -41,6 +41,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)
 
@@ -100,7 +102,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))

instrumentation/opentelemetry-instrumentation-requests/src/opentelemetry/instrumentation/requests/__init__.py

@@ -101,15 +101,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,
 )
@@ -126,7 +125,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,
@@ -142,6 +140,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(
@@ -269,25 +293,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)

instrumentation/opentelemetry-instrumentation-requests/tests/test_requests_integration.py

@@ -832,3 +832,34 @@ 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):
+        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.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 = 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)