HTTP semantic convention stability migration for urllib (open-telemetry#2736)

lzchen · web-flow · commit c87ffd4a3595 · 2024-07-30T11:28:31.000-07:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -56,6 +56,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   ([#2715](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2715))
 - `opentelemetry-instrumentation-django` Implement new semantic convention opt-in with stable http semantic conventions
   ([#2714](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2714))
+- `opentelemetry-instrumentation-urllib` Implement new semantic convention opt-in migration
+  ([#2736](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2736))
 
 ### Breaking changes
 
@@ -71,6 +73,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   ([#2682](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2682))
 - Populate `{method}` as `HTTP` on `_OTHER` methods from scope for `django` middleware
   ([#2714](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2714))
+- Populate `{method}` as `HTTP` on `_OTHER` methods from scope for `urllib` instrumentation
+  ([#2736](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2736))
 - `opentelemetry-instrumentation-httpx`, `opentelemetry-instrumentation-aiohttp-client`,
   `opentelemetry-instrumentation-requests` Populate `{method}` as `HTTP` on `_OTHER` methods
   ([#2726](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2726))
diff --git a/instrumentation/README.md b/instrumentation/README.md
@@ -45,6 +45,6 @@
 | [opentelemetry-instrumentation-threading](./opentelemetry-instrumentation-threading) | threading | No | experimental
 | [opentelemetry-instrumentation-tornado](./opentelemetry-instrumentation-tornado) | tornado >= 5.1.1 | Yes | experimental
 | [opentelemetry-instrumentation-tortoiseorm](./opentelemetry-instrumentation-tortoiseorm) | tortoise-orm >= 0.17.0 | No | experimental
-| [opentelemetry-instrumentation-urllib](./opentelemetry-instrumentation-urllib) | urllib | Yes | experimental
+| [opentelemetry-instrumentation-urllib](./opentelemetry-instrumentation-urllib) | urllib | Yes | migration
 | [opentelemetry-instrumentation-urllib3](./opentelemetry-instrumentation-urllib3) | urllib3 >= 1.0.0, < 3.0.0 | Yes | migration
 | [opentelemetry-instrumentation-wsgi](./opentelemetry-instrumentation-wsgi) | wsgi | Yes | migration
diff --git a/instrumentation/opentelemetry-instrumentation-urllib/src/opentelemetry/instrumentation/urllib/__init__.py b/instrumentation/opentelemetry-instrumentation-urllib/src/opentelemetry/instrumentation/urllib/__init__.py
@@ -85,25 +85,49 @@ def response_hook(span, request_obj, response)
     Request,
 )
 
+from opentelemetry.instrumentation._semconv import (
+    _client_duration_attrs_new,
+    _client_duration_attrs_old,
+    _filter_semconv_duration_attrs,
+    _get_schema_url,
+    _HTTPStabilityMode,
+    _OpenTelemetrySemanticConventionStability,
+    _OpenTelemetryStabilitySignalType,
+    _report_new,
+    _report_old,
+    _set_http_method,
+    _set_http_network_protocol_version,
+    _set_http_url,
+    _set_status,
+)
 from opentelemetry.instrumentation.instrumentor import BaseInstrumentor
 from opentelemetry.instrumentation.urllib.package import _instruments
 from opentelemetry.instrumentation.urllib.version import __version__
 from opentelemetry.instrumentation.utils import (
-    http_status_to_status_code,
     is_http_instrumentation_enabled,
     suppress_http_instrumentation,
 )
 from opentelemetry.metrics import Histogram, get_meter
 from opentelemetry.propagate import inject
+from opentelemetry.semconv._incubating.metrics.http_metrics import (
+    HTTP_CLIENT_REQUEST_BODY_SIZE,
+    HTTP_CLIENT_RESPONSE_BODY_SIZE,
+    create_http_client_request_body_size,
+    create_http_client_response_body_size,
+)
+from opentelemetry.semconv.attributes.error_attributes import ERROR_TYPE
 from opentelemetry.semconv.metrics import MetricInstruments
+from opentelemetry.semconv.metrics.http_metrics import (
+    HTTP_CLIENT_REQUEST_DURATION,
+)
 from opentelemetry.semconv.trace import SpanAttributes
 from opentelemetry.trace import Span, SpanKind, get_tracer
-from opentelemetry.trace.status import Status
 from opentelemetry.util.http import (
     ExcludeList,
     get_excluded_urls,
     parse_excluded_urls,
     remove_url_credentials,
+    sanitize_method,
 )
 
 _excluded_urls_from_env = get_excluded_urls("URLLIB")
@@ -133,23 +157,29 @@ def _instrument(self, **kwargs):
                 ``excluded_urls``: A string containing a comma-delimited
                     list of regexes used to exclude URLs from tracking
         """
+        # initialize semantic conventions opt-in if needed
+        _OpenTelemetrySemanticConventionStability._initialize()
+        sem_conv_opt_in_mode = _OpenTelemetrySemanticConventionStability._get_opentelemetry_stability_opt_in_mode(
+            _OpenTelemetryStabilitySignalType.HTTP,
+        )
+        schema_url = _get_schema_url(sem_conv_opt_in_mode)
         tracer_provider = kwargs.get("tracer_provider")
         tracer = get_tracer(
             __name__,
             __version__,
             tracer_provider,
-            schema_url="https://opentelemetry.io/schemas/1.11.0",
+            schema_url=schema_url,
         )
         excluded_urls = kwargs.get("excluded_urls")
         meter_provider = kwargs.get("meter_provider")
         meter = get_meter(
             __name__,
             __version__,
             meter_provider,
-            schema_url="https://opentelemetry.io/schemas/1.11.0",
+            schema_url=schema_url,
         )
 
-        histograms = _create_client_histograms(meter)
+        histograms = _create_client_histograms(meter, sem_conv_opt_in_mode)
 
         _instrument(
             tracer,
@@ -161,6 +191,7 @@ def _instrument(self, **kwargs):
                 if excluded_urls is None
                 else parse_excluded_urls(excluded_urls)
             ),
+            sem_conv_opt_in_mode=sem_conv_opt_in_mode,
         )
 
     def _uninstrument(self, **kwargs):
@@ -173,12 +204,14 @@ def uninstrument_opener(
         _uninstrument_from(opener, restore_as_bound_func=True)
 
 
+# pylint: disable=too-many-statements
 def _instrument(
     tracer,
     histograms: Dict[str, Histogram],
     request_hook: _RequestHookT = None,
     response_hook: _ResponseHookT = None,
     excluded_urls: ExcludeList = None,
+    sem_conv_opt_in_mode: _HTTPStabilityMode = _HTTPStabilityMode.DEFAULT,
 ):
     """Enables tracing of all requests calls that go through
     :code:`urllib.Client._make_request`"""
@@ -214,14 +247,22 @@ def _instrumented_open_call(
 
         method = request.get_method().upper()
 
-        span_name = method.strip()
+        span_name = _get_span_name(method)
 
         url = remove_url_credentials(url)
 
-        labels = {
-            SpanAttributes.HTTP_METHOD: method,
-            SpanAttributes.HTTP_URL: url,
-        }
+        data = getattr(request, "data", None)
+        request_size = 0 if data is None else len(data)
+
+        labels = {}
+
+        _set_http_method(
+            labels,
+            method,
+            sanitize_method(method),
+            sem_conv_opt_in_mode,
+        )
+        _set_http_url(labels, url, sem_conv_opt_in_mode)
 
         with tracer.start_as_current_span(
             span_name, kind=SpanKind.CLIENT, attributes=labels
@@ -241,24 +282,50 @@ def _instrumented_open_call(
                     exception = exc
                     result = getattr(exc, "file", None)
                 finally:
-                    elapsed_time = round((default_timer() - start_time) * 1000)
-
+                    duration_s = default_timer() - start_time
+            response_size = 0
             if result is not None:
+                response_size = int(result.headers.get("Content-Length", 0))
                 code_ = result.getcode()
-                labels[SpanAttributes.HTTP_STATUS_CODE] = str(code_)
-
-                if span.is_recording() and code_ is not None:
-                    span.set_attribute(SpanAttributes.HTTP_STATUS_CODE, code_)
-                    span.set_status(Status(http_status_to_status_code(code_)))
+                # set http status code based on semconv
+                if code_:
+                    _set_status_code_attribute(
+                        span, code_, labels, sem_conv_opt_in_mode
+                    )
 
                 ver_ = str(getattr(result, "version", ""))
                 if ver_:
-                    labels[SpanAttributes.HTTP_FLAVOR] = (
-                        f"{ver_[:1]}.{ver_[:-1]}"
+                    _set_http_network_protocol_version(
+                        labels, f"{ver_[:1]}.{ver_[:-1]}", sem_conv_opt_in_mode
                     )
 
+            if exception is not None and _report_new(sem_conv_opt_in_mode):
+                span.set_attribute(ERROR_TYPE, type(exception).__qualname__)
+                labels[ERROR_TYPE] = type(exception).__qualname__
+
+            duration_attrs_old = _filter_semconv_duration_attrs(
+                labels,
+                _client_duration_attrs_old,
+                _client_duration_attrs_new,
+                sem_conv_opt_in_mode=_HTTPStabilityMode.DEFAULT,
+            )
+            duration_attrs_new = _filter_semconv_duration_attrs(
+                labels,
+                _client_duration_attrs_old,
+                _client_duration_attrs_new,
+                sem_conv_opt_in_mode=_HTTPStabilityMode.HTTP,
+            )
+
+            duration_attrs_old[SpanAttributes.HTTP_URL] = url
+
             _record_histograms(
-                histograms, labels, request, result, elapsed_time
+                histograms,
+                duration_attrs_old,
+                duration_attrs_new,
+                request_size,
+                response_size,
+                duration_s,
+                sem_conv_opt_in_mode,
             )
 
             if callable(response_hook):
@@ -296,43 +363,108 @@ def _uninstrument_from(instr_root, restore_as_bound_func=False):
     setattr(instr_root, instr_func_name, original)
 
 
-def _create_client_histograms(meter) -> Dict[str, Histogram]:
-    histograms = {
-        MetricInstruments.HTTP_CLIENT_DURATION: meter.create_histogram(
-            name=MetricInstruments.HTTP_CLIENT_DURATION,
-            unit="ms",
-            description="Measures the duration of outbound HTTP requests.",
-        ),
-        MetricInstruments.HTTP_CLIENT_REQUEST_SIZE: meter.create_histogram(
-            name=MetricInstruments.HTTP_CLIENT_REQUEST_SIZE,
-            unit="By",
-            description="Measures the size of HTTP request messages.",
-        ),
-        MetricInstruments.HTTP_CLIENT_RESPONSE_SIZE: meter.create_histogram(
-            name=MetricInstruments.HTTP_CLIENT_RESPONSE_SIZE,
-            unit="By",
-            description="Measures the size of HTTP response messages.",
-        ),
-    }
+def _get_span_name(method: str) -> str:
+    method = sanitize_method(method.strip())
+    if method == "_OTHER":
+        method = "HTTP"
+    return method
+
+
+def _set_status_code_attribute(
+    span: Span,
+    status_code: int,
+    metric_attributes: dict = None,
+    sem_conv_opt_in_mode: _HTTPStabilityMode = _HTTPStabilityMode.DEFAULT,
+) -> None:
+
+    status_code_str = str(status_code)
+    try:
+        status_code = int(status_code)
+    except ValueError:
+        status_code = -1
+
+    if metric_attributes is None:
+        metric_attributes = {}
+
+    _set_status(
+        span,
+        metric_attributes,
+        status_code,
+        status_code_str,
+        server_span=False,
+        sem_conv_opt_in_mode=sem_conv_opt_in_mode,
+    )
+
+
+def _create_client_histograms(
+    meter, sem_conv_opt_in_mode=_HTTPStabilityMode.DEFAULT
+) -> Dict[str, Histogram]:
+    histograms = {}
+    if _report_old(sem_conv_opt_in_mode):
+        histograms[MetricInstruments.HTTP_CLIENT_DURATION] = (
+            meter.create_histogram(
+                name=MetricInstruments.HTTP_CLIENT_DURATION,
+                unit="ms",
+                description="Measures the duration of the outbound HTTP request",
+            )
+        )
+        histograms[MetricInstruments.HTTP_CLIENT_REQUEST_SIZE] = (
+            meter.create_histogram(
+                name=MetricInstruments.HTTP_CLIENT_REQUEST_SIZE,
+                unit="By",
+                description="Measures the size of HTTP request messages.",
+            )
+        )
+        histograms[MetricInstruments.HTTP_CLIENT_RESPONSE_SIZE] = (
+            meter.create_histogram(
+                name=MetricInstruments.HTTP_CLIENT_RESPONSE_SIZE,
+                unit="By",
+                description="Measures the size of HTTP response messages.",
+            )
+        )
+    if _report_new(sem_conv_opt_in_mode):
+        histograms[HTTP_CLIENT_REQUEST_DURATION] = meter.create_histogram(
+            name=HTTP_CLIENT_REQUEST_DURATION,
+            unit="s",
+            description="Duration of HTTP client requests.",
+        )
+        histograms[HTTP_CLIENT_REQUEST_BODY_SIZE] = (
+            create_http_client_request_body_size(meter)
+        )
+        histograms[HTTP_CLIENT_RESPONSE_BODY_SIZE] = (
+            create_http_client_response_body_size(meter)
+        )
 
     return histograms
 
 
 def _record_histograms(
-    histograms, metric_attributes, request, response, elapsed_time
+    histograms: Dict[str, Histogram],
+    metric_attributes_old: dict,
+    metric_attributes_new: dict,
+    request_size: int,
+    response_size: int,
+    duration_s: float,
+    sem_conv_opt_in_mode: _HTTPStabilityMode = _HTTPStabilityMode.DEFAULT,
 ):
-    histograms[MetricInstruments.HTTP_CLIENT_DURATION].record(
-        elapsed_time, attributes=metric_attributes
-    )
-
-    data = getattr(request, "data", None)
-    request_size = 0 if data is None else len(data)
-    histograms[MetricInstruments.HTTP_CLIENT_REQUEST_SIZE].record(
-        request_size, attributes=metric_attributes
-    )
-
-    if response is not None:
-        response_size = int(response.headers.get("Content-Length", 0))
+    if _report_old(sem_conv_opt_in_mode):
+        duration = max(round(duration_s * 1000), 0)
+        histograms[MetricInstruments.HTTP_CLIENT_DURATION].record(
+            duration, attributes=metric_attributes_old
+        )
+        histograms[MetricInstruments.HTTP_CLIENT_REQUEST_SIZE].record(
+            request_size, attributes=metric_attributes_old
+        )
         histograms[MetricInstruments.HTTP_CLIENT_RESPONSE_SIZE].record(
-            response_size, attributes=metric_attributes
+            response_size, attributes=metric_attributes_old
+        )
+    if _report_new(sem_conv_opt_in_mode):
+        histograms[HTTP_CLIENT_REQUEST_DURATION].record(
+            duration_s, attributes=metric_attributes_new
+        )
+        histograms[HTTP_CLIENT_REQUEST_BODY_SIZE].record(
+            request_size, attributes=metric_attributes_new
+        )
+        histograms[HTTP_CLIENT_RESPONSE_BODY_SIZE].record(
+            response_size, attributes=metric_attributes_new
         )
diff --git a/instrumentation/opentelemetry-instrumentation-urllib/src/opentelemetry/instrumentation/urllib/package.py b/instrumentation/opentelemetry-instrumentation-urllib/src/opentelemetry/instrumentation/urllib/package.py
@@ -16,3 +16,5 @@
 _instruments = tuple()
 
 _supports_metrics = True
+
+_semconv_status = "migration"
diff --git a/instrumentation/opentelemetry-instrumentation-urllib/tests/test_metrics_instrumentation.py b/instrumentation/opentelemetry-instrumentation-urllib/tests/test_metrics_instrumentation.py
diff --git a/instrumentation/opentelemetry-instrumentation-urllib/tests/test_urllib_integration.py b/instrumentation/opentelemetry-instrumentation-urllib/tests/test_urllib_integration.py
