HTTP transition for flask (#2454)

open-telemetry · Apr 25, 2024 · c8d5f85 · c8d5f85
1 parent d5b5925
commit c8d5f85
Show file tree

Hide file tree

Showing 10 changed files with 399 additions and 66 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -15,6 +15,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   ([#2372](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2372))
 - Drop support for instrumenting elasticsearch client < 6`
   ([#2422](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2422))
+- `opentelemetry-instrumentation-wsgi` Add `http.method` to `span.name`
+  ([#2425](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2425))
+- `opentelemetry-instrumentation-flask` Add `http.method` to `span.name`
+  ([#2454](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2454))
 
 ### Added
 
@@ -23,6 +27,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   ([#2382](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2382))
 - `opentelemetry-instrumentation-wsgi` Implement new semantic convention opt-in with stable http semantic conventions
   ([#2425](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2425))
+- `opentelemetry-instrumentation-flask` Implement new semantic convention opt-in with stable http semantic conventions
+  ([#2454](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2454))
 - `opentelemetry-instrumentation-threading` Initial release for threading
   ([#2253](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2253))
 - `opentelemetry-instrumentation-pika` Instrumentation for `channel.consume()` (supported

diff --git a/instrumentation/README.md b/instrumentation/README.md
@@ -20,7 +20,7 @@
 | [opentelemetry-instrumentation-elasticsearch](./opentelemetry-instrumentation-elasticsearch) | elasticsearch >= 2.0 | No | experimental
 | [opentelemetry-instrumentation-falcon](./opentelemetry-instrumentation-falcon) | falcon >= 1.4.1, < 4.0.0 | Yes | experimental
 | [opentelemetry-instrumentation-fastapi](./opentelemetry-instrumentation-fastapi) | fastapi ~= 0.58 | Yes | experimental
-| [opentelemetry-instrumentation-flask](./opentelemetry-instrumentation-flask) | flask >= 1.0 | Yes | experimental
+| [opentelemetry-instrumentation-flask](./opentelemetry-instrumentation-flask) | flask >= 1.0 | Yes | migration
 | [opentelemetry-instrumentation-grpc](./opentelemetry-instrumentation-grpc) | grpcio ~= 1.27 | No | experimental
 | [opentelemetry-instrumentation-httpx](./opentelemetry-instrumentation-httpx) | httpx >= 0.18.0 | No | experimental
 | [opentelemetry-instrumentation-jinja2](./opentelemetry-instrumentation-jinja2) | jinja2 >= 2.7, < 4.0 | No | experimental
@@ -48,4 +48,4 @@
 | [opentelemetry-instrumentation-tortoiseorm](./opentelemetry-instrumentation-tortoiseorm) | tortoise-orm >= 0.17.0 | No | experimental
 | [opentelemetry-instrumentation-urllib](./opentelemetry-instrumentation-urllib) | urllib | Yes | experimental
 | [opentelemetry-instrumentation-urllib3](./opentelemetry-instrumentation-urllib3) | urllib3 >= 1.0.0, < 3.0.0 | Yes | experimental
-| [opentelemetry-instrumentation-wsgi](./opentelemetry-instrumentation-wsgi) | wsgi | Yes | experimental
+| [opentelemetry-instrumentation-wsgi](./opentelemetry-instrumentation-wsgi) | wsgi | Yes | migration
diff --git a/...n/opentelemetry-instrumentation-flask/src/opentelemetry/instrumentation/flask/__init__.py b/...n/opentelemetry-instrumentation-flask/src/opentelemetry/instrumentation/flask/__init__.py
@@ -250,6 +250,15 @@ def response_hook(span: Span, status: str, response_headers: List):
 
 import opentelemetry.instrumentation.wsgi as otel_wsgi
 from opentelemetry import context, trace
+from opentelemetry.instrumentation._semconv import (
+    _METRIC_ATTRIBUTES_SERVER_DURATION_NAME,
+    _get_schema_url,
+    _HTTPStabilityMode,
+    _OpenTelemetrySemanticConventionStability,
+    _OpenTelemetryStabilitySignalType,
+    _report_new,
+    _report_old,
+)
 from opentelemetry.instrumentation.flask.package import _instruments
 from opentelemetry.instrumentation.flask.version import __version__
 from opentelemetry.instrumentation.instrumentor import BaseInstrumentor
@@ -260,7 +269,11 @@ def response_hook(span: Span, status: str, response_headers: List):
 from opentelemetry.metrics import get_meter
 from opentelemetry.semconv.metrics import MetricInstruments
 from opentelemetry.semconv.trace import SpanAttributes
-from opentelemetry.util.http import get_excluded_urls, parse_excluded_urls
+from opentelemetry.util.http import (
+    get_excluded_urls,
+    parse_excluded_urls,
+    sanitize_method,
+)
 
 _logger = getLogger(__name__)
 
@@ -286,8 +299,13 @@ def _request_ctx_ref() -> weakref.ReferenceType:
 
 
 def get_default_span_name():
+    method = sanitize_method(
+        flask.request.environ.get("REQUEST_METHOD", "").strip()
+    )
+    if method == "_OTHER":
+        method = "HTTP"
     try:
-        span_name = flask.request.url_rule.rule
+        span_name = f"{method} {flask.request.url_rule.rule}"
     except AttributeError:
         span_name = otel_wsgi.get_default_span_name(flask.request.environ)
     return span_name
@@ -296,9 +314,11 @@ def get_default_span_name():
 def _rewrapped_app(
     wsgi_app,
     active_requests_counter,
-    duration_histogram,
+    duration_histogram_old=None,
     response_hook=None,
     excluded_urls=None,
+    sem_conv_opt_in_mode=_HTTPStabilityMode.DEFAULT,
+    duration_histogram_new=None,
 ):
     def _wrapped_app(wrapped_app_environ, start_response):
         # We want to measure the time for route matching, etc.
@@ -307,11 +327,16 @@ def _wrapped_app(wrapped_app_environ, start_response):
         # we better avoid it.
         wrapped_app_environ[_ENVIRON_STARTTIME_KEY] = time_ns()
         start = default_timer()
-        attributes = otel_wsgi.collect_request_attributes(wrapped_app_environ)
+        attributes = otel_wsgi.collect_request_attributes(
+            wrapped_app_environ, sem_conv_opt_in_mode
+        )
         active_requests_count_attrs = (
-            otel_wsgi._parse_active_request_count_attrs(attributes)
+            otel_wsgi._parse_active_request_count_attrs(
+                attributes,
+                sem_conv_opt_in_mode,
+            )
         )
-        duration_attrs = otel_wsgi._parse_duration_attrs(attributes)
+
         active_requests_counter.add(1, active_requests_count_attrs)
 
         def _start_response(status, response_headers, *args, **kwargs):
@@ -330,13 +355,12 @@ def _start_response(status, response_headers, *args, **kwargs):
 
                 if span:
                     otel_wsgi.add_response_attributes(
-                        span, status, response_headers
+                        span,
+                        status,
+                        response_headers,
+                        attributes,
+                        sem_conv_opt_in_mode,
                     )
-                    status_code = otel_wsgi._parse_status_code(status)
-                    if status_code is not None:
-                        duration_attrs[SpanAttributes.HTTP_STATUS_CODE] = (
-                            status_code
-                        )
                     if (
                         span.is_recording()
                         and span.kind == trace.SpanKind.SERVER
@@ -357,8 +381,21 @@ def _start_response(status, response_headers, *args, **kwargs):
             return start_response(status, response_headers, *args, **kwargs)
 
         result = wsgi_app(wrapped_app_environ, _start_response)
-        duration = max(round((default_timer() - start) * 1000), 0)
-        duration_histogram.record(duration, duration_attrs)
+        duration_s = default_timer() - start
+        if duration_histogram_old:
+            duration_attrs_old = otel_wsgi._parse_duration_attrs(
+                attributes, _HTTPStabilityMode.DEFAULT
+            )
+            duration_histogram_old.record(
+                max(round(duration_s * 1000), 0), duration_attrs_old
+            )
+        if duration_histogram_new:
+            duration_attrs_new = otel_wsgi._parse_duration_attrs(
+                attributes, _HTTPStabilityMode.HTTP
+            )
+            duration_histogram_new.record(
+                max(duration_s, 0), duration_attrs_new
+            )
         active_requests_counter.add(-1, active_requests_count_attrs)
         return result
 
@@ -371,6 +408,7 @@ def _wrapped_before_request(
     excluded_urls=None,
     enable_commenter=True,
     commenter_options=None,
+    sem_conv_opt_in_mode=_HTTPStabilityMode.DEFAULT,
 ):
     def _before_request():
         if excluded_urls and excluded_urls.url_disabled(flask.request.url):
@@ -379,7 +417,8 @@ def _before_request():
         span_name = get_default_span_name()
 
         attributes = otel_wsgi.collect_request_attributes(
-            flask_request_environ
+            flask_request_environ,
+            sem_conv_opt_in_mode=sem_conv_opt_in_mode,
         )
         if flask.request.url_rule:
             # For 404 that result from no route found, etc, we
@@ -490,6 +529,7 @@ class _InstrumentedFlask(flask.Flask):
     _enable_commenter = True
     _commenter_options = None
     _meter_provider = None
+    _sem_conv_opt_in_mode = _HTTPStabilityMode.DEFAULT
 
     def __init__(self, *args, **kwargs):
         super().__init__(*args, **kwargs)
@@ -503,11 +543,20 @@ def __init__(self, *args, **kwargs):
             _InstrumentedFlask._meter_provider,
             schema_url="https://opentelemetry.io/schemas/1.11.0",
         )
-        duration_histogram = meter.create_histogram(
-            name=MetricInstruments.HTTP_SERVER_DURATION,
-            unit="ms",
-            description="Duration of HTTP client requests.",
-        )
+        duration_histogram_old = None
+        if _report_old(_InstrumentedFlask._sem_conv_opt_in_mode):
+            duration_histogram_old = meter.create_histogram(
+                name=MetricInstruments.HTTP_SERVER_DURATION,
+                unit="ms",
+                description="measures the duration of the inbound HTTP request",
+            )
+        duration_histogram_new = None
+        if _report_new(_InstrumentedFlask._sem_conv_opt_in_mode):
+            duration_histogram_new = meter.create_histogram(
+                name=_METRIC_ATTRIBUTES_SERVER_DURATION_NAME,
+                unit="s",
+                description="measures the duration of the inbound HTTP request",
+            )
         active_requests_counter = meter.create_up_down_counter(
             name=MetricInstruments.HTTP_SERVER_ACTIVE_REQUESTS,
             unit="requests",
@@ -517,9 +566,11 @@ def __init__(self, *args, **kwargs):
         self.wsgi_app = _rewrapped_app(
             self.wsgi_app,
             active_requests_counter,
-            duration_histogram,
+            duration_histogram_old,
             _InstrumentedFlask._response_hook,
             excluded_urls=_InstrumentedFlask._excluded_urls,
+            sem_conv_opt_in_mode=_InstrumentedFlask._sem_conv_opt_in_mode,
+            duration_histogram_new=duration_histogram_new,
         )
 
         tracer = trace.get_tracer(
@@ -535,6 +586,7 @@ def __init__(self, *args, **kwargs):
             excluded_urls=_InstrumentedFlask._excluded_urls,
             enable_commenter=_InstrumentedFlask._enable_commenter,
             commenter_options=_InstrumentedFlask._commenter_options,
+            sem_conv_opt_in_mode=_InstrumentedFlask._sem_conv_opt_in_mode,
         )
         self._before_request = _before_request
         self.before_request(_before_request)
@@ -578,11 +630,19 @@ def _instrument(self, **kwargs):
         _InstrumentedFlask._commenter_options = commenter_options
         meter_provider = kwargs.get("meter_provider")
         _InstrumentedFlask._meter_provider = meter_provider
+
+        sem_conv_opt_in_mode = _OpenTelemetrySemanticConventionStability._get_opentelemetry_stability_opt_in_mode(
+            _OpenTelemetryStabilitySignalType.HTTP,
+        )
+
+        _InstrumentedFlask._sem_conv_opt_in_mode = sem_conv_opt_in_mode
+
         flask.Flask = _InstrumentedFlask
 
     def _uninstrument(self, **kwargs):
         flask.Flask = self._original_flask
 
+    # pylint: disable=too-many-locals
     @staticmethod
     def instrument_app(
         app,
@@ -598,6 +658,11 @@ def instrument_app(
             app._is_instrumented_by_opentelemetry = False
 
         if not app._is_instrumented_by_opentelemetry:
+            # initialize semantic conventions opt-in if needed
+            _OpenTelemetrySemanticConventionStability._initialize()
+            sem_conv_opt_in_mode = _OpenTelemetrySemanticConventionStability._get_opentelemetry_stability_opt_in_mode(
+                _OpenTelemetryStabilitySignalType.HTTP,
+            )
             excluded_urls = (
                 parse_excluded_urls(excluded_urls)
                 if excluded_urls is not None
@@ -607,33 +672,44 @@ def instrument_app(
                 __name__,
                 __version__,
                 meter_provider,
-                schema_url="https://opentelemetry.io/schemas/1.11.0",
-            )
-            duration_histogram = meter.create_histogram(
-                name=MetricInstruments.HTTP_SERVER_DURATION,
-                unit="ms",
-                description="Duration of HTTP client requests.",
+                schema_url=_get_schema_url(sem_conv_opt_in_mode),
             )
+            duration_histogram_old = None
+            if _report_old(sem_conv_opt_in_mode):
+                duration_histogram_old = meter.create_histogram(
+                    name=MetricInstruments.HTTP_SERVER_DURATION,
+                    unit="ms",
+                    description="measures the duration of the inbound HTTP request",
+                )
+            duration_histogram_new = None
+            if _report_new(sem_conv_opt_in_mode):
+                duration_histogram_new = meter.create_histogram(
+                    name=_METRIC_ATTRIBUTES_SERVER_DURATION_NAME,
+                    unit="s",
+                    description="measures the duration of the inbound HTTP request",
+                )
             active_requests_counter = meter.create_up_down_counter(
                 name=MetricInstruments.HTTP_SERVER_ACTIVE_REQUESTS,
-                unit="requests",
-                description="measures the number of concurrent HTTP requests that are currently in-flight",
+                unit="{request}",
+                description="Number of active HTTP server requests.",
             )
 
             app._original_wsgi_app = app.wsgi_app
             app.wsgi_app = _rewrapped_app(
                 app.wsgi_app,
                 active_requests_counter,
-                duration_histogram,
-                response_hook,
+                duration_histogram_old,
+                response_hook=response_hook,
                 excluded_urls=excluded_urls,
+                sem_conv_opt_in_mode=sem_conv_opt_in_mode,
+                duration_histogram_new=duration_histogram_new,
             )
 
             tracer = trace.get_tracer(
                 __name__,
                 __version__,
                 tracer_provider,
-                schema_url="https://opentelemetry.io/schemas/1.11.0",
+                schema_url=_get_schema_url(sem_conv_opt_in_mode),
             )
 
             _before_request = _wrapped_before_request(
@@ -644,6 +720,7 @@ def instrument_app(
                 commenter_options=(
                     commenter_options if commenter_options else {}
                 ),
+                sem_conv_opt_in_mode=sem_conv_opt_in_mode,
             )
             app._before_request = _before_request
             app.before_request(_before_request)

diff --git a/...on/opentelemetry-instrumentation-flask/src/opentelemetry/instrumentation/flask/package.py b/...on/opentelemetry-instrumentation-flask/src/opentelemetry/instrumentation/flask/package.py
@@ -16,3 +16,5 @@
 _instruments = ("flask >= 1.0",)
 
 _supports_metrics = True
+
+_semconv_status = "migration"
diff --git a/instrumentation/opentelemetry-instrumentation-flask/tests/test_copy_context.py b/instrumentation/opentelemetry-instrumentation-flask/tests/test_copy_context.py
@@ -44,5 +44,5 @@ def test_copycontext(self):
         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("GET /copy_context", resp.json["span_name"])
         self.assertEqual("a-header", resp.json["request_header"])