From 2d68dc5ac09ed52eb1271df31e027f520f646069 Mon Sep 17 00:00:00 2001 From: Filip Nikolovski Date: Fri, 9 Aug 2024 17:13:45 +0200 Subject: [PATCH 01/20] Implement new HTTP semantic convention opt-in for Falcon --- CHANGELOG.md | 1 + .../instrumentation/falcon/__init__.py | 67 ++++++++++++------- .../tests/test_falcon.py | 18 +++-- 3 files changed, 56 insertions(+), 30 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 49b66f69df..729f453bef 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `opentelemetry-instrumentation-kafka-python` Instrument temporary fork, kafka-python-ng inside kafka-python's instrumentation ([#2537](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2537))) +- `opentelemetry-instrumentation-falcon` Implement new HTTP semantic convention opt-in for Falcon ## Breaking changes diff --git a/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py b/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py index 1a252b9a16..c9ddc58c69 100644 --- a/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py @@ -193,6 +193,14 @@ def response_hook(span, req, resp): import opentelemetry.instrumentation.wsgi as otel_wsgi from opentelemetry import context, trace +from opentelemetry.instrumentation._semconv import ( + _HTTPStabilityMode, + _OpenTelemetrySemanticConventionStability, + _OpenTelemetryStabilitySignalType, + _report_new, + _report_old, + _set_status, +) from opentelemetry.instrumentation.falcon.package import _instruments from opentelemetry.instrumentation.falcon.version import __version__ from opentelemetry.instrumentation.instrumentor import BaseInstrumentor @@ -203,12 +211,12 @@ def response_hook(span, req, resp): from opentelemetry.instrumentation.utils import ( _start_internal_or_server_span, extract_attributes_from_object, - http_status_to_status_code, ) from opentelemetry.metrics import get_meter +from opentelemetry.semconv.attributes.error_attributes import ERROR_TYPE from opentelemetry.semconv.metrics import MetricInstruments from opentelemetry.semconv.trace import SpanAttributes -from opentelemetry.trace.status import Status, StatusCode +from opentelemetry.trace.status import StatusCode from opentelemetry.util.http import get_excluded_urls, get_traced_request_attrs _logger = getLogger(__name__) @@ -243,6 +251,11 @@ class _InstrumentedFalconAPI(getattr(falcon, _instrument_app)): def __init__(self, *args, **kwargs): otel_opts = kwargs.pop("_otel_opts", {}) + _OpenTelemetrySemanticConventionStability._initialize() + self._sem_conv_opt_in_mode = _OpenTelemetrySemanticConventionStability._get_opentelemetry_stability_opt_in_mode( + _OpenTelemetryStabilitySignalType.HTTP, + ) + # inject trace middleware self._middlewares_list = kwargs.pop("middleware", []) if self._middlewares_list is None: @@ -283,6 +296,7 @@ def __init__(self, *args, **kwargs): ), otel_opts.pop("request_hook", None), otel_opts.pop("response_hook", None), + self._sem_conv_opt_in_mode, ) self._middlewares_list.insert(0, trace_middleware) kwargs["middleware"] = self._middlewares_list @@ -382,9 +396,21 @@ def _start_response(status, response_headers, *args, **kwargs): raise finally: if span.is_recording(): - duration_attrs[SpanAttributes.HTTP_STATUS_CODE] = ( - span.attributes.get(SpanAttributes.HTTP_STATUS_CODE) - ) + if _report_old(self._sem_conv_opt_in_mode): + duration_attrs[SpanAttributes.HTTP_STATUS_CODE] = ( + span.attributes.get(SpanAttributes.HTTP_STATUS_CODE) + ) + if _report_new(self._sem_conv_opt_in_mode): + duration_attrs[ + SpanAttributes.HTTP_RESPONSE_STATUS_CODE + ] = span.attributes.get( + SpanAttributes.HTTP_RESPONSE_STATUS_CODE + ) + if span.status.status_code == StatusCode.ERROR: + duration_attrs[ERROR_TYPE] = span.attributes.get( + ERROR_TYPE + ) + duration = max(round((default_timer() - start) * 1000), 0) self.duration_histogram.record(duration, duration_attrs) self.active_requests_counter.add(-1, active_requests_count_attrs) @@ -409,11 +435,13 @@ def __init__( traced_request_attrs=None, request_hook=None, response_hook=None, + sem_conv_opt_in_mode: _HTTPStabilityMode = _HTTPStabilityMode.DEFAULT, ): self.tracer = tracer self._traced_request_attrs = traced_request_attrs self._request_hook = request_hook self._response_hook = response_hook + self._sem_conv_opt_in_mode = sem_conv_opt_in_mode def process_request(self, req, resp): span = req.env.get(_ENVIRON_SPAN_KEY) @@ -446,10 +474,8 @@ def process_response( return status = resp.status - reason = None if resource is None: status = "404" - reason = "NotFound" else: if _ENVIRON_EXC in req.env: exc = req.env[_ENVIRON_EXC] @@ -459,28 +485,18 @@ def process_response( if exc_type and not req_succeeded: if "HTTPNotFound" in exc_type.__name__: status = "404" - reason = "NotFound" else: status = "500" - reason = f"{exc_type.__name__}: {exc}" status = status.split(" ")[0] try: - status_code = int(status) - span.set_attribute(SpanAttributes.HTTP_STATUS_CODE, status_code) - otel_status_code = http_status_to_status_code( - status_code, server_span=True - ) - - # set the description only when the status code is ERROR - if otel_status_code is not StatusCode.ERROR: - reason = None - - span.set_status( - Status( - status_code=otel_status_code, - description=reason, - ) + _set_status( + span, + {}, + int(status), + resp.status, + span.kind == trace.SpanKind.SERVER, + self._sem_conv_opt_in_mode, ) # Falcon 1 does not support response headers. So @@ -493,6 +509,9 @@ def process_response( # Check if low-cardinality route is available as per semantic-conventions if req.uri_template: span.update_name(f"{req.method} {req.uri_template}") + span.set_attribute( + SpanAttributes.HTTP_ROUTE, req.uri_template + ) else: span.update_name(f"{req.method}") diff --git a/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py b/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py index dbc2512ca0..da2874107b 100644 --- a/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py +++ b/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py @@ -22,7 +22,9 @@ from opentelemetry import trace from opentelemetry.instrumentation._semconv import ( + _server_active_requests_count_attrs_new, _server_active_requests_count_attrs_old, + _server_duration_attrs_new, _server_duration_attrs_old, ) from opentelemetry.instrumentation.falcon import FalconInstrumentor @@ -53,8 +55,10 @@ "http.server.duration", ] _recommended_attrs = { - "http.server.active_requests": _server_active_requests_count_attrs_old, - "http.server.duration": _server_duration_attrs_old, + "http.server.active_requests": _server_active_requests_count_attrs_new + + _server_active_requests_count_attrs_old, + "http.server.duration": _server_duration_attrs_new + + _server_duration_attrs_old, } @@ -66,6 +70,7 @@ def setUp(self): { "OTEL_PYTHON_FALCON_EXCLUDED_URLS": "ping", "OTEL_PYTHON_FALCON_TRACED_REQUEST_ATTRS": "query_string", + "OTEL_SEMCONV_STABILITY_OPT_IN": "http/dup", }, ) self.env_patch.start() @@ -129,6 +134,7 @@ def _test_method(self, method): SpanAttributes.HTTP_FLAVOR: "1.1", "falcon.resource": "HelloWorldResource", SpanAttributes.HTTP_STATUS_CODE: 201, + SpanAttributes.HTTP_ROUTE: "/hello", }, ) # In falcon<3, NET_PEER_IP is always set by default to 127.0.0.1 @@ -180,10 +186,7 @@ def test_500(self): self.assertEqual(span.name, "GET /error") self.assertFalse(span.status.is_ok) self.assertEqual(span.status.status_code, StatusCode.ERROR) - self.assertEqual( - span.status.description, - "NameError: name 'non_existent_var' is not defined", - ) + self.assertEqual(span.status.description, None) self.assertSpanHasAttributes( span, { @@ -196,6 +199,7 @@ def test_500(self): SpanAttributes.NET_PEER_PORT: 65133, SpanAttributes.HTTP_FLAVOR: "1.1", SpanAttributes.HTTP_STATUS_CODE: 500, + SpanAttributes.HTTP_ROUTE: "/error", }, ) # In falcon<3, NET_PEER_IP is always set by default to 127.0.0.1 @@ -230,6 +234,7 @@ def test_url_template(self): SpanAttributes.HTTP_FLAVOR: "1.1", "falcon.resource": "UserResource", SpanAttributes.HTTP_STATUS_CODE: 200, + SpanAttributes.HTTP_ROUTE: "/user/{user_id}", }, ) @@ -338,6 +343,7 @@ def test_falcon_metric_values(self): "net.host.port": 80, "net.host.name": "falconframework.org", "http.status_code": 404, + "http.response.status_code": 404, } expected_requests_count_attributes = { "http.method": "GET", From 228897da7b5579b9f64755fbd4c2106509dacdd4 Mon Sep 17 00:00:00 2001 From: Filip Nikolovski Date: Fri, 9 Aug 2024 19:11:51 +0200 Subject: [PATCH 02/20] fix tests for falcon 1 & 2 --- .../tests/test_falcon.py | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py b/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py index da2874107b..64848cd169 100644 --- a/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py +++ b/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py @@ -186,7 +186,16 @@ def test_500(self): self.assertEqual(span.name, "GET /error") self.assertFalse(span.status.is_ok) self.assertEqual(span.status.status_code, StatusCode.ERROR) - self.assertEqual(span.status.description, None) + + _parsed_falcon_version = package_version.parse(_falcon_version) + if _parsed_falcon_version < package_version.parse("3.0.0"): + self.assertEqual( + span.status.description, + "NameError: name 'non_existent_var' is not defined", + ) + else: + self.assertEqual(span.status.description, None) + self.assertSpanHasAttributes( span, { From a9a30a80fad9d57f3e0911a4f2bc69f2f58fbe51 Mon Sep 17 00:00:00 2001 From: Filip Nikolovski Date: Mon, 2 Dec 2024 11:15:20 +0100 Subject: [PATCH 03/20] add semconv status as migration for falcon in instrumentations readme --- instrumentation/README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/instrumentation/README.md b/instrumentation/README.md index bff37fde6c..ef14091c3d 100644 --- a/instrumentation/README.md +++ b/instrumentation/README.md @@ -20,7 +20,7 @@ | [opentelemetry-instrumentation-dbapi](./opentelemetry-instrumentation-dbapi) | dbapi | No | experimental | [opentelemetry-instrumentation-django](./opentelemetry-instrumentation-django) | django >= 1.10 | Yes | experimental | [opentelemetry-instrumentation-elasticsearch](./opentelemetry-instrumentation-elasticsearch) | elasticsearch >= 6.0 | No | experimental -| [opentelemetry-instrumentation-falcon](./opentelemetry-instrumentation-falcon) | falcon >= 1.4.1, < 4.0.0 | Yes | experimental +| [opentelemetry-instrumentation-falcon](./opentelemetry-instrumentation-falcon) | falcon >= 1.4.1, < 4.0.0 | Yes | migration | [opentelemetry-instrumentation-fastapi](./opentelemetry-instrumentation-fastapi) | fastapi ~= 0.58 | Yes | migration | [opentelemetry-instrumentation-flask](./opentelemetry-instrumentation-flask) | flask >= 1.0 | Yes | migration | [opentelemetry-instrumentation-grpc](./opentelemetry-instrumentation-grpc) | grpcio >= 1.42.0 | No | experimental @@ -49,4 +49,4 @@ | [opentelemetry-instrumentation-tortoiseorm](./opentelemetry-instrumentation-tortoiseorm) | tortoise-orm >= 0.17.0 | No | 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 \ No newline at end of file +| [opentelemetry-instrumentation-wsgi](./opentelemetry-instrumentation-wsgi) | wsgi | Yes | migration From b3db9084e20e5dd82aae3908f1776c1b0b68e1e2 Mon Sep 17 00:00:00 2001 From: Filip Nikolovski Date: Mon, 2 Dec 2024 11:19:34 +0100 Subject: [PATCH 04/20] set schema_url based on the opt-in mode --- .../src/opentelemetry/instrumentation/falcon/__init__.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py b/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py index 9830646a37..fe2c65f5c5 100644 --- a/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py @@ -200,6 +200,7 @@ def response_hook(span, req, resp): _report_new, _report_old, _set_status, + _get_schema_url, ) from opentelemetry.instrumentation.falcon.package import _instruments from opentelemetry.instrumentation.falcon.version import __version__ @@ -270,13 +271,13 @@ def __init__(self, *args, **kwargs): __name__, __version__, tracer_provider, - schema_url="https://opentelemetry.io/schemas/1.11.0", + schema_url=_get_schema_url(self._sem_conv_opt_in_mode), ) self._otel_meter = get_meter( __name__, __version__, meter_provider, - schema_url="https://opentelemetry.io/schemas/1.11.0", + schema_url=_get_schema_url(self._sem_conv_opt_in_mode), ) self.duration_histogram = self._otel_meter.create_histogram( name=MetricInstruments.HTTP_SERVER_DURATION, From aa5486515a8b328643da76f3037d709b905ea0b4 Mon Sep 17 00:00:00 2001 From: Filip Nikolovski Date: Mon, 2 Dec 2024 15:37:34 +0100 Subject: [PATCH 05/20] add histograms based on opt-in mode; set metrics attributes; update tests --- .../instrumentation/falcon/__init__.py | 51 +++- .../tests/test_falcon.py | 228 ++++++++++++++---- 2 files changed, 220 insertions(+), 59 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py b/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py index fe2c65f5c5..a3b6884106 100644 --- a/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py @@ -216,6 +216,9 @@ def response_hook(span, req, resp): from opentelemetry.metrics import get_meter from opentelemetry.semconv.attributes.error_attributes import ERROR_TYPE from opentelemetry.semconv.metrics import MetricInstruments +from opentelemetry.semconv.metrics.http_metrics import ( + HTTP_SERVER_REQUEST_DURATION, +) from opentelemetry.semconv.trace import SpanAttributes from opentelemetry.trace.status import StatusCode from opentelemetry.util.http import get_excluded_urls, get_traced_request_attrs @@ -279,11 +282,22 @@ def __init__(self, *args, **kwargs): meter_provider, schema_url=_get_schema_url(self._sem_conv_opt_in_mode), ) - self.duration_histogram = self._otel_meter.create_histogram( - name=MetricInstruments.HTTP_SERVER_DURATION, - unit="ms", - description="Measures the duration of inbound HTTP requests.", - ) + + self.duration_histogram_old = None + if _report_old(self._sem_conv_opt_in_mode): + self.duration_histogram_old = self._otel_meter.create_histogram( + name=MetricInstruments.HTTP_SERVER_DURATION, + unit="ms", + description="Measures the duration of inbound HTTP requests.", + ) + self.duration_histogram_new = None + if _report_new(self._sem_conv_opt_in_mode): + self.duration_histogram_new = self._otel_meter.create_histogram( + name=HTTP_SERVER_REQUEST_DURATION, + description="Duration of HTTP server requests.", + unit="s", + ) + self.active_requests_counter = self._otel_meter.create_up_down_counter( name=MetricInstruments.HTTP_SERVER_ACTIVE_REQUESTS, unit="requests", @@ -358,11 +372,10 @@ def __call__(self, env, start_response): context_carrier=env, context_getter=otel_wsgi.wsgi_getter, ) - attributes = otel_wsgi.collect_request_attributes(env) + attributes = otel_wsgi.collect_request_attributes(env, self._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, self._sem_conv_opt_in_mode) ) - duration_attrs = otel_wsgi._parse_duration_attrs(attributes) self.active_requests_counter.add(1, active_requests_count_attrs) if span.is_recording(): @@ -394,12 +407,23 @@ def _start_response(status, response_headers, *args, **kwargs): exception = exc raise finally: - if span.is_recording(): - if _report_old(self._sem_conv_opt_in_mode): + duration_s = default_timer() - start + if self.duration_histogram_old: + duration_attrs = otel_wsgi._parse_duration_attrs( + attributes, _HTTPStabilityMode.DEFAULT + ) + if span.is_recording(): duration_attrs[SpanAttributes.HTTP_STATUS_CODE] = ( span.attributes.get(SpanAttributes.HTTP_STATUS_CODE) ) - if _report_new(self._sem_conv_opt_in_mode): + self.duration_histogram_old.record( + max(round(duration_s * 1000), 0), duration_attrs + ) + if self.duration_histogram_new: + duration_attrs = otel_wsgi._parse_duration_attrs( + attributes, _HTTPStabilityMode.HTTP + ) + if span.is_recording(): duration_attrs[ SpanAttributes.HTTP_RESPONSE_STATUS_CODE ] = span.attributes.get( @@ -409,9 +433,10 @@ def _start_response(status, response_headers, *args, **kwargs): duration_attrs[ERROR_TYPE] = span.attributes.get( ERROR_TYPE ) + self.duration_histogram_new.record( + max(duration_s, 0), duration_attrs + ) - duration = max(round((default_timer() - start) * 1000), 0) - self.duration_histogram.record(duration, duration_attrs) self.active_requests_counter.add(-1, active_requests_count_attrs) if exception is None: activation.__exit__(None, None, None) diff --git a/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py b/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py index 64848cd169..6f4b403da2 100644 --- a/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py +++ b/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py @@ -22,6 +22,8 @@ from opentelemetry import trace from opentelemetry.instrumentation._semconv import ( + OTEL_SEMCONV_STABILITY_OPT_IN, + _OpenTelemetrySemanticConventionStability, _server_active_requests_count_attrs_new, _server_active_requests_count_attrs_old, _server_duration_attrs_new, @@ -54,6 +56,7 @@ "http.server.active_requests", "http.server.duration", ] + _recommended_attrs = { "http.server.active_requests": _server_active_requests_count_attrs_new + _server_active_requests_count_attrs_old, @@ -61,18 +64,50 @@ + _server_duration_attrs_old, } +_recommended_metrics_attrs_old = { + "http.server.active_requests": _server_active_requests_count_attrs_old, + "http.server.duration": _server_duration_attrs_old, +} +_recommended_metrics_attrs_new = { + "http.server.active_requests": _server_active_requests_count_attrs_new, + "http.server.request.duration": _server_duration_attrs_new, +} +_server_active_requests_count_attrs_both = ( + _server_active_requests_count_attrs_old +) +_server_active_requests_count_attrs_both.extend( + _server_active_requests_count_attrs_new +) +_recommended_metrics_attrs_both = { + "http.server.active_requests": _server_active_requests_count_attrs_both, + "http.server.duration": _server_duration_attrs_old, + "http.server.request.duration": _server_duration_attrs_new, +} + class TestFalconBase(TestBase): def setUp(self): super().setUp() + + test_name = "" + if hasattr(self, "_testMethodName"): + test_name = self._testMethodName + sem_conv_mode = "default" + if "new_semconv" in test_name: + sem_conv_mode = "http" + elif "both_semconv" in test_name: + sem_conv_mode = "http/dup" + self.env_patch = patch.dict( "os.environ", { "OTEL_PYTHON_FALCON_EXCLUDED_URLS": "ping", "OTEL_PYTHON_FALCON_TRACED_REQUEST_ATTRS": "query_string", - "OTEL_SEMCONV_STABILITY_OPT_IN": "http/dup", + OTEL_SEMCONV_STABILITY_OPT_IN: sem_conv_mode, }, ) + + _OpenTelemetrySemanticConventionStability._initialized = False self.env_patch.start() FalconInstrumentor().instrument( @@ -95,22 +130,58 @@ class TestFalconInstrumentation(TestFalconBase, WsgiTestBase): def test_get(self): self._test_method("GET") + def test_get_new_semconv(self): + self._test_method("GET", old_semconv=False, new_semconv=True) + + def test_get_both_semconv(self): + self._test_method("GET", old_semconv=True, new_semconv=True) + def test_post(self): self._test_method("POST") + def test_post_new_semconv(self): + self._test_method("POST", old_semconv=False, new_semconv=True) + + def test_post_both_semconv(self): + self._test_method("POST", old_semconv=True, new_semconv=True) + def test_patch(self): self._test_method("PATCH") + def test_patch_new_semconv(self): + self._test_method("PATCH", old_semconv=False, new_semconv=True) + + def test_patch_both_semconv(self): + self._test_method("PATCH", old_semconv=True, new_semconv=True) + def test_put(self): self._test_method("PUT") + def test_put_new_semconv(self): + self._test_method("PUT", old_semconv=False, new_semconv=True) + + def test_put_both_semconv(self): + self._test_method("PUT", old_semconv=True, new_semconv=True) + def test_delete(self): self._test_method("DELETE") + def test_delete_new_semconv(self): + self._test_method("DELETE", old_semconv=False, new_semconv=True) + + def test_delete_both_semconv(self): + self._test_method("DELETE", old_semconv=True, new_semconv=True) + def test_head(self): self._test_method("HEAD") - def _test_method(self, method): + def test_head_new_semconv(self): + self._test_method("HEAD", old_semconv=False, new_semconv=True) + + def test_head_both_semconv(self): + self._test_method("HEAD", old_semconv=True, new_semconv=True) + + def _test_method(self, method, old_semconv=True, new_semconv=False): self.client().simulate_request(method=method, path="/hello") spans = self.memory_exporter.get_finished_spans() self.assertEqual(len(spans), 1) @@ -121,21 +192,41 @@ def _test_method(self, method): span.status.description, None, ) + + expected_attributes = {} + expected_attributes_old = { + SpanAttributes.HTTP_METHOD: method, + SpanAttributes.HTTP_SERVER_NAME: "falconframework.org", + SpanAttributes.HTTP_SCHEME: "http", + SpanAttributes.NET_HOST_PORT: 80, + SpanAttributes.HTTP_HOST: "falconframework.org", + SpanAttributes.HTTP_TARGET: "/", + SpanAttributes.NET_PEER_PORT: 65133, + SpanAttributes.HTTP_FLAVOR: "1.1", + "falcon.resource": "HelloWorldResource", + SpanAttributes.HTTP_STATUS_CODE: 201, + SpanAttributes.HTTP_ROUTE: "/hello", + } + expected_attributes_new = { + SpanAttributes.HTTP_REQUEST_METHOD: method, + SpanAttributes.SERVER_ADDRESS: "falconframework.org", + SpanAttributes.URL_SCHEME: "http", + SpanAttributes.SERVER_PORT: 80, + SpanAttributes.URL_PATH: "/", + SpanAttributes.CLIENT_PORT: 65133, + SpanAttributes.NETWORK_PROTOCOL_VERSION: "1.1", + "falcon.resource": "HelloWorldResource", + SpanAttributes.HTTP_RESPONSE_STATUS_CODE: 201, + SpanAttributes.HTTP_ROUTE: "/hello", + } + + if old_semconv: + expected_attributes.update(expected_attributes_old) + if new_semconv: + expected_attributes.update(expected_attributes_new) + self.assertSpanHasAttributes( - span, - { - SpanAttributes.HTTP_METHOD: method, - SpanAttributes.HTTP_SERVER_NAME: "falconframework.org", - SpanAttributes.HTTP_SCHEME: "http", - SpanAttributes.NET_HOST_PORT: 80, - SpanAttributes.HTTP_HOST: "falconframework.org", - SpanAttributes.HTTP_TARGET: "/", - SpanAttributes.NET_PEER_PORT: 65133, - SpanAttributes.HTTP_FLAVOR: "1.1", - "falcon.resource": "HelloWorldResource", - SpanAttributes.HTTP_STATUS_CODE: 201, - SpanAttributes.HTTP_ROUTE: "/hello", - }, + span, expected_attributes ) # In falcon<3, NET_PEER_IP is always set by default to 127.0.0.1 # In falcon>3, NET_PEER_IP is not set to anything by default to @@ -342,48 +433,93 @@ def test_falcon_metrics(self): ) self.assertTrue(number_data_point_seen and histogram_data_point_seen) - def test_falcon_metric_values(self): - expected_duration_attributes = { - "http.method": "GET", - "http.host": "falconframework.org", - "http.scheme": "http", - "http.flavor": "1.1", - "http.server_name": "falconframework.org", - "net.host.port": 80, - "net.host.name": "falconframework.org", - "http.status_code": 404, - "http.response.status_code": 404, - } - expected_requests_count_attributes = { - "http.method": "GET", - "http.host": "falconframework.org", - "http.scheme": "http", - "http.flavor": "1.1", - "http.server_name": "falconframework.org", - } - start = default_timer() + def test_falcon_metric_values_new_semconv(self): + number_data_point_seen = False + histogram_data_point_seen = False + self.client().simulate_get("/hello/756") - duration = max(round((default_timer() - start) * 1000), 0) metrics_list = self.memory_metrics_reader.get_metrics_data() for resource_metric in metrics_list.resource_metrics: for scope_metric in resource_metric.scope_metrics: for metric in scope_metric.metrics: + data_points = list(metric.data.data_points) + self.assertEqual(len(data_points), 1) for point in list(metric.data.data_points): if isinstance(point, HistogramDataPoint): - self.assertDictEqual( - expected_duration_attributes, - dict(point.attributes), - ) self.assertEqual(point.count, 1) - self.assertAlmostEqual( - duration, point.sum, delta=10 + histogram_data_point_seen = True + if isinstance(point, NumberDataPoint): + self.assertEqual(point.value, 0) + number_data_point_seen = True + for attr in point.attributes: + self.assertIn( + attr, + _recommended_metrics_attrs_new[metric.name], ) + + self.assertTrue(number_data_point_seen and histogram_data_point_seen) + + def test_falcon_metric_values_both_semconv(self): + number_data_point_seen = False + histogram_data_point_seen = False + + self.client().simulate_get("/hello/756") + metrics_list = self.memory_metrics_reader.get_metrics_data() + for resource_metric in metrics_list.resource_metrics: + for scope_metric in resource_metric.scope_metrics: + for metric in scope_metric.metrics: + if metric.unit == "ms": + self.assertEqual(metric.name, "http.server.duration") + elif metric.unit == "s": + self.assertEqual( + metric.name, "http.server.request.duration" + ) + else: + self.assertEqual( + metric.name, "http.server.active_requests" + ) + data_points = list(metric.data.data_points) + self.assertEqual(len(data_points), 1) + for point in list(metric.data.data_points): + if isinstance(point, HistogramDataPoint): + self.assertEqual(point.count, 1) + histogram_data_point_seen = True if isinstance(point, NumberDataPoint): - self.assertDictEqual( - expected_requests_count_attributes, - dict(point.attributes), + self.assertEqual(point.value, 0) + number_data_point_seen = True + for attr in point.attributes: + print(metric.name) + self.assertIn( + attr, + _recommended_metrics_attrs_both[metric.name], ) + self.assertTrue(number_data_point_seen and histogram_data_point_seen) + + def test_falcon_metric_values(self): + number_data_point_seen = False + histogram_data_point_seen = False + + self.client().simulate_get("/hello/756") + metrics_list = self.memory_metrics_reader.get_metrics_data() + for resource_metric in metrics_list.resource_metrics: + for scope_metric in resource_metric.scope_metrics: + for metric in scope_metric.metrics: + data_points = list(metric.data.data_points) + self.assertEqual(len(data_points), 1) + for point in list(metric.data.data_points): + if isinstance(point, HistogramDataPoint): + self.assertEqual(point.count, 1) + histogram_data_point_seen = True + if isinstance(point, NumberDataPoint): self.assertEqual(point.value, 0) + number_data_point_seen = True + for attr in point.attributes: + self.assertIn( + attr, + _recommended_metrics_attrs_old[metric.name], + ) + + self.assertTrue(number_data_point_seen and histogram_data_point_seen) def test_metric_uninstrument(self): self.client().simulate_request(method="POST", path="/hello/756") From 2ed146121618547ab8b8c76ac9462e675eab2b19 Mon Sep 17 00:00:00 2001 From: Filip Nikolovski Date: Mon, 2 Dec 2024 15:40:57 +0100 Subject: [PATCH 06/20] run ruff --- .../opentelemetry/instrumentation/falcon/__init__.py | 10 +++++++--- .../tests/test_falcon.py | 5 +---- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py b/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py index a3b6884106..12aaa832c1 100644 --- a/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py @@ -194,13 +194,13 @@ def response_hook(span, req, resp): import opentelemetry.instrumentation.wsgi as otel_wsgi from opentelemetry import context, trace from opentelemetry.instrumentation._semconv import ( + _get_schema_url, _HTTPStabilityMode, _OpenTelemetrySemanticConventionStability, _OpenTelemetryStabilitySignalType, _report_new, _report_old, _set_status, - _get_schema_url, ) from opentelemetry.instrumentation.falcon.package import _instruments from opentelemetry.instrumentation.falcon.version import __version__ @@ -372,9 +372,13 @@ def __call__(self, env, start_response): context_carrier=env, context_getter=otel_wsgi.wsgi_getter, ) - attributes = otel_wsgi.collect_request_attributes(env, self._sem_conv_opt_in_mode) + attributes = otel_wsgi.collect_request_attributes( + env, self._sem_conv_opt_in_mode + ) active_requests_count_attrs = ( - otel_wsgi._parse_active_request_count_attrs(attributes, self._sem_conv_opt_in_mode) + otel_wsgi._parse_active_request_count_attrs( + attributes, self._sem_conv_opt_in_mode + ) ) self.active_requests_counter.add(1, active_requests_count_attrs) diff --git a/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py b/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py index 6f4b403da2..1b804c9868 100644 --- a/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py +++ b/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py @@ -12,7 +12,6 @@ # See the License for the specific language governing permissions and # limitations under the License. # -from timeit import default_timer from unittest.mock import Mock, patch import pytest @@ -225,9 +224,7 @@ def _test_method(self, method, old_semconv=True, new_semconv=False): if new_semconv: expected_attributes.update(expected_attributes_new) - self.assertSpanHasAttributes( - span, expected_attributes - ) + self.assertSpanHasAttributes(span, expected_attributes) # In falcon<3, NET_PEER_IP is always set by default to 127.0.0.1 # In falcon>3, NET_PEER_IP is not set to anything by default to # https://github.com/falconry/falcon/blob/5233d0abed977d9dab78ebadf305f5abe2eef07c/falcon/testing/helpers.py#L1168-L1172 # noqa From e8d3e6c668c011a439a34438b72244c979bd0d8c Mon Sep 17 00:00:00 2001 From: Filip Nikolovski Date: Mon, 2 Dec 2024 15:48:26 +0100 Subject: [PATCH 07/20] disable too-many-public-methods lint rule for TestFalconInstrumentation --- .../opentelemetry-instrumentation-falcon/tests/test_falcon.py | 1 + 1 file changed, 1 insertion(+) diff --git a/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py b/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py index 1b804c9868..305d4822a0 100644 --- a/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py +++ b/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py @@ -125,6 +125,7 @@ def tearDown(self): self.env_patch.stop() +# pylint: disable=too-many-public-methods class TestFalconInstrumentation(TestFalconBase, WsgiTestBase): def test_get(self): self._test_method("GET") From 181549563dc6aaf01aa887f070fa14555280f32d Mon Sep 17 00:00:00 2001 From: Filip Nikolovski Date: Tue, 3 Dec 2024 11:14:05 +0100 Subject: [PATCH 08/20] add _semconv_status in package.py --- .../src/opentelemetry/instrumentation/falcon/package.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/package.py b/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/package.py index 2fd463739c..101db19b28 100644 --- a/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/package.py +++ b/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/package.py @@ -16,3 +16,5 @@ _instruments = ("falcon >= 1.4.1, < 4.0.0",) _supports_metrics = True + +_semconv_status = "migration" From 7754f0d59ee5f49149d79faae8fccbfce211d61a Mon Sep 17 00:00:00 2001 From: Filip Nikolovski Date: Tue, 10 Dec 2024 10:04:19 +0100 Subject: [PATCH 09/20] cleanup imports --- .../instrumentation/falcon/__init__.py | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py b/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py index 12aaa832c1..41a73aa7b0 100644 --- a/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py @@ -214,13 +214,11 @@ def response_hook(span, req, resp): extract_attributes_from_object, ) from opentelemetry.metrics import get_meter -from opentelemetry.semconv.attributes.error_attributes import ERROR_TYPE from opentelemetry.semconv.metrics import MetricInstruments from opentelemetry.semconv.metrics.http_metrics import ( HTTP_SERVER_REQUEST_DURATION, ) from opentelemetry.semconv.trace import SpanAttributes -from opentelemetry.trace.status import StatusCode from opentelemetry.util.http import get_excluded_urls, get_traced_request_attrs _logger = getLogger(__name__) @@ -255,7 +253,6 @@ class _InstrumentedFalconAPI(getattr(falcon, _instrument_app)): def __init__(self, *args, **kwargs): otel_opts = kwargs.pop("_otel_opts", {}) - _OpenTelemetrySemanticConventionStability._initialize() self._sem_conv_opt_in_mode = _OpenTelemetrySemanticConventionStability._get_opentelemetry_stability_opt_in_mode( _OpenTelemetryStabilitySignalType.HTTP, ) @@ -416,10 +413,6 @@ def _start_response(status, response_headers, *args, **kwargs): duration_attrs = otel_wsgi._parse_duration_attrs( attributes, _HTTPStabilityMode.DEFAULT ) - if span.is_recording(): - duration_attrs[SpanAttributes.HTTP_STATUS_CODE] = ( - span.attributes.get(SpanAttributes.HTTP_STATUS_CODE) - ) self.duration_histogram_old.record( max(round(duration_s * 1000), 0), duration_attrs ) @@ -427,16 +420,6 @@ def _start_response(status, response_headers, *args, **kwargs): duration_attrs = otel_wsgi._parse_duration_attrs( attributes, _HTTPStabilityMode.HTTP ) - if span.is_recording(): - duration_attrs[ - SpanAttributes.HTTP_RESPONSE_STATUS_CODE - ] = span.attributes.get( - SpanAttributes.HTTP_RESPONSE_STATUS_CODE - ) - if span.status.status_code == StatusCode.ERROR: - duration_attrs[ERROR_TYPE] = span.attributes.get( - ERROR_TYPE - ) self.duration_histogram_new.record( max(duration_s, 0), duration_attrs ) From b94156968f2158358aaa3eeedf22f2d9bfa79afa Mon Sep 17 00:00:00 2001 From: Filip Nikolovski Date: Tue, 10 Dec 2024 12:41:03 +0100 Subject: [PATCH 10/20] set request attributes in env, set status code in metric attributes independent from tracing decisions; update tests --- .../instrumentation/falcon/__init__.py | 57 +++++++++++++---- .../tests/test_falcon.py | 64 ++++++++++++++++++- 2 files changed, 108 insertions(+), 13 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py b/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py index 41a73aa7b0..429db76d9f 100644 --- a/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py @@ -214,17 +214,20 @@ def response_hook(span, req, resp): extract_attributes_from_object, ) from opentelemetry.metrics import get_meter +from opentelemetry.semconv.attributes.http_attributes import ( + HTTP_ROUTE, +) from opentelemetry.semconv.metrics import MetricInstruments from opentelemetry.semconv.metrics.http_metrics import ( HTTP_SERVER_REQUEST_DURATION, ) -from opentelemetry.semconv.trace import SpanAttributes from opentelemetry.util.http import get_excluded_urls, get_traced_request_attrs _logger = getLogger(__name__) _ENVIRON_STARTTIME_KEY = "opentelemetry-falcon.starttime_key" _ENVIRON_SPAN_KEY = "opentelemetry-falcon.span_key" +_ENVIRON_REQ_ATTRS = "opentelemetry-falcon.req_attrs" _ENVIRON_ACTIVATION_KEY = "opentelemetry-falcon.activation_key" _ENVIRON_TOKEN = "opentelemetry-falcon.token" _ENVIRON_EXC = "opentelemetry-falcon.exc" @@ -247,6 +250,31 @@ def response_hook(span, req, resp): _falcon_version = 1 +def set_status_code( + span, + status_code, + metric_attributes=None, + sem_conv_opt_in_mode=_HTTPStabilityMode.DEFAULT, +): + """Adds HTTP response attributes to span using the status_code argument.""" + 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=True, + sem_conv_opt_in_mode=sem_conv_opt_in_mode, + ) + + class _InstrumentedFalconAPI(getattr(falcon, _instrument_app)): _instrumented_falcon_apps = set() @@ -393,6 +421,7 @@ def __call__(self, env, start_response): activation.__enter__() env[_ENVIRON_SPAN_KEY] = span env[_ENVIRON_ACTIVATION_KEY] = activation + env[_ENVIRON_REQ_ATTRS] = attributes exception = None def _start_response(status, response_headers, *args, **kwargs): @@ -478,8 +507,9 @@ def process_resource(self, req, resp, resource, params): def process_response(self, req, resp, resource, req_succeeded=None): # pylint:disable=R0201,R0912 span = req.env.get(_ENVIRON_SPAN_KEY) + req_attrs = req.env.get(_ENVIRON_REQ_ATTRS) - if not span or not span.is_recording(): + if not span: return status = resp.status @@ -497,17 +527,22 @@ def process_response(self, req, resp, resource, req_succeeded=None): # pylint:d else: status = "500" - status = status.split(" ")[0] + status_code = status.split(" ")[0] try: - _set_status( + set_status_code( span, - {}, - int(status), - resp.status, - span.kind == trace.SpanKind.SERVER, - self._sem_conv_opt_in_mode, + status_code, + req_attrs, + sem_conv_opt_in_mode=self._sem_conv_opt_in_mode, ) + if ( + _report_new(self._sem_conv_opt_in_mode) + and req.uri_template + and req_attrs is not None + ): + req_attrs[HTTP_ROUTE] = req.uri_template + # Falcon 1 does not support response headers. So # send an empty dict. response_headers = {} @@ -518,9 +553,7 @@ def process_response(self, req, resp, resource, req_succeeded=None): # pylint:d # Check if low-cardinality route is available as per semantic-conventions if req.uri_template: span.update_name(f"{req.method} {req.uri_template}") - span.set_attribute( - SpanAttributes.HTTP_ROUTE, req.uri_template - ) + span.set_attribute(HTTP_ROUTE, req.uri_template) else: span.update_name(f"{req.method}") diff --git a/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py b/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py index 305d4822a0..f5f82216e9 100644 --- a/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py +++ b/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py @@ -308,6 +308,47 @@ def test_500(self): span.attributes[SpanAttributes.NET_PEER_IP], "127.0.0.1" ) + def test_url_template_new_semconv(self): + self.client().simulate_get("/user/123") + spans = self.memory_exporter.get_finished_spans() + metrics_list = self.memory_metrics_reader.get_metrics_data() + + self.assertEqual(len(spans), 1) + self.assertTrue(len(metrics_list.resource_metrics) != 0) + span = spans[0] + self.assertEqual(span.name, "GET /user/{user_id}") + self.assertEqual(span.status.status_code, StatusCode.UNSET) + self.assertEqual( + span.status.description, + None, + ) + self.assertSpanHasAttributes( + span, + { + SpanAttributes.HTTP_REQUEST_METHOD: "GET", + SpanAttributes.SERVER_ADDRESS: "falconframework.org", + SpanAttributes.URL_SCHEME: "http", + SpanAttributes.SERVER_PORT: 80, + SpanAttributes.URL_PATH: "/", + SpanAttributes.CLIENT_PORT: 65133, + SpanAttributes.NETWORK_PROTOCOL_VERSION: "1.1", + "falcon.resource": "UserResource", + SpanAttributes.HTTP_RESPONSE_STATUS_CODE: 200, + SpanAttributes.HTTP_ROUTE: "/user/{user_id}", + }, + ) + + for resource_metric in metrics_list.resource_metrics: + for scope_metric in resource_metric.scope_metrics: + for metric in scope_metric.metrics: + if metric.name == "http.server.request.duration": + data_points = list(metric.data.data_points) + for point in data_points: + self.assertIn( + "http.route", + point.attributes, + ) + def test_url_template(self): self.client().simulate_get("/user/123") spans = self.memory_exporter.get_finished_spans() @@ -391,6 +432,28 @@ def test_traced_not_recording(self): self.assertFalse(mock_span.set_attribute.called) self.assertFalse(mock_span.set_status.called) + metrics_list = self.memory_metrics_reader.get_metrics_data() + self.assertTrue(len(metrics_list.resource_metrics) != 0) + + metrics_list = self.memory_metrics_reader.get_metrics_data() + for resource_metric in metrics_list.resource_metrics: + for scope_metric in resource_metric.scope_metrics: + for metric in scope_metric.metrics: + data_points = list(metric.data.data_points) + self.assertEqual(len(data_points), 1) + for point in list(metric.data.data_points): + if isinstance(point, HistogramDataPoint): + self.assertEqual(point.count, 1) + if isinstance(point, NumberDataPoint): + self.assertEqual(point.value, 0) + for attr in point.attributes: + self.assertIn( + attr, + _recommended_metrics_attrs_old[ + metric.name + ], + ) + def test_uninstrument_after_instrument(self): self.client().simulate_get(path="/hello") spans = self.memory_exporter.get_finished_spans() @@ -486,7 +549,6 @@ def test_falcon_metric_values_both_semconv(self): self.assertEqual(point.value, 0) number_data_point_seen = True for attr in point.attributes: - print(metric.name) self.assertIn( attr, _recommended_metrics_attrs_both[metric.name], From c44244c2015234fcccd74f255b182326a4238735 Mon Sep 17 00:00:00 2001 From: Filip Nikolovski Date: Tue, 10 Dec 2024 13:28:16 +0100 Subject: [PATCH 11/20] fix exceptions to not be masked as 500 on every thrown exception --- .../opentelemetry/instrumentation/falcon/__init__.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py b/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py index 429db76d9f..2069e6e55f 100644 --- a/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py @@ -516,14 +516,21 @@ def process_response(self, req, resp, resource, req_succeeded=None): # pylint:d if resource is None: status = "404" else: + exc_type, exc = None, None if _ENVIRON_EXC in req.env: exc = req.env[_ENVIRON_EXC] exc_type = type(exc) - else: - exc_type, exc = None, None + if exc_type and not req_succeeded: if "HTTPNotFound" in exc_type.__name__: status = "404" + elif isinstance(exc, falcon.HTTPError) or isinstance( + exc, falcon.HTTPStatus + ): + try: + status = exc.title.split(" ")[0] + except ValueError: + status = "500" else: status = "500" From 59586fd90db387e70ed771416bc2557cb2ed5267 Mon Sep 17 00:00:00 2001 From: Filip Nikolovski Date: Tue, 10 Dec 2024 13:40:32 +0100 Subject: [PATCH 12/20] resolve lint issues --- .../src/opentelemetry/instrumentation/falcon/__init__.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py b/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py index 2069e6e55f..c062731fd4 100644 --- a/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py @@ -524,9 +524,7 @@ def process_response(self, req, resp, resource, req_succeeded=None): # pylint:d if exc_type and not req_succeeded: if "HTTPNotFound" in exc_type.__name__: status = "404" - elif isinstance(exc, falcon.HTTPError) or isinstance( - exc, falcon.HTTPStatus - ): + elif isinstance(exc, falcon.HTTPError, falcon.HTTPStatus): try: status = exc.title.split(" ")[0] except ValueError: From 6ac9e9868c2f12be3243a248ed2ad0bb93ac4063 Mon Sep 17 00:00:00 2001 From: Filip Nikolovski Date: Tue, 10 Dec 2024 13:59:21 +0100 Subject: [PATCH 13/20] run tox -e generate; fix typo --- instrumentation/README.md | 2 +- .../src/opentelemetry/instrumentation/falcon/__init__.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/instrumentation/README.md b/instrumentation/README.md index ef14091c3d..232f88ee17 100644 --- a/instrumentation/README.md +++ b/instrumentation/README.md @@ -49,4 +49,4 @@ | [opentelemetry-instrumentation-tortoiseorm](./opentelemetry-instrumentation-tortoiseorm) | tortoise-orm >= 0.17.0 | No | 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 +| [opentelemetry-instrumentation-wsgi](./opentelemetry-instrumentation-wsgi) | wsgi | Yes | migration \ No newline at end of file diff --git a/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py b/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py index c062731fd4..0e79412f35 100644 --- a/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py @@ -524,7 +524,7 @@ def process_response(self, req, resp, resource, req_succeeded=None): # pylint:d if exc_type and not req_succeeded: if "HTTPNotFound" in exc_type.__name__: status = "404" - elif isinstance(exc, falcon.HTTPError, falcon.HTTPStatus): + elif isinstance(exc, (falcon.HTTPError, falcon.HTTPStatus)): try: status = exc.title.split(" ")[0] except ValueError: From 3d5ec5557582547d7635b455d2b4af46d0db775b Mon Sep 17 00:00:00 2001 From: Filip Nikolovski Date: Tue, 31 Dec 2024 12:25:07 +0100 Subject: [PATCH 14/20] fix tests --- .../tests/test_falcon.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py b/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py index c54424b1a3..8761572159 100644 --- a/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py +++ b/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py @@ -211,7 +211,7 @@ def _test_method(self, method, old_semconv=True, new_semconv=False): SpanAttributes.HTTP_HOST: "falconframework.org", SpanAttributes.HTTP_TARGET: "/" if self._has_fixed_http_target - else "/error", + else "/hello", SpanAttributes.NET_PEER_PORT: 65133, SpanAttributes.HTTP_FLAVOR: "1.1", "falcon.resource": "HelloWorldResource", @@ -223,7 +223,9 @@ def _test_method(self, method, old_semconv=True, new_semconv=False): SpanAttributes.SERVER_ADDRESS: "falconframework.org", SpanAttributes.URL_SCHEME: "http", SpanAttributes.SERVER_PORT: 80, - SpanAttributes.URL_PATH: "/", + SpanAttributes.URL_PATH: "/" + if self._has_fixed_http_target + else "/hello", SpanAttributes.CLIENT_PORT: 65133, SpanAttributes.NETWORK_PROTOCOL_VERSION: "1.1", "falcon.resource": "HelloWorldResource", @@ -344,7 +346,9 @@ def test_url_template_new_semconv(self): SpanAttributes.SERVER_ADDRESS: "falconframework.org", SpanAttributes.URL_SCHEME: "http", SpanAttributes.SERVER_PORT: 80, - SpanAttributes.URL_PATH: "/", + SpanAttributes.URL_PATH: "/" + if self._has_fixed_http_target + else "/user/123", SpanAttributes.CLIENT_PORT: 65133, SpanAttributes.NETWORK_PROTOCOL_VERSION: "1.1", "falcon.resource": "UserResource", From 606958a38bb4024f40912f4390e36a633f4848bd Mon Sep 17 00:00:00 2001 From: Filip Nikolovski Date: Tue, 31 Dec 2024 16:50:22 +0100 Subject: [PATCH 15/20] update changelog --- CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8e8d58d1a4..5227318198 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 ([#3111](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3111)) - `opentelemetry-instrumentation-falcon` add support version to v4 ([#3086](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3086)) +- `opentelemetry-instrumentation-falcon` Implement new HTTP semantic convention opt-in for Falcon + ([#2790](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2790)) ### Fixed @@ -51,8 +53,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ([#2942](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2942)) - `opentelemetry-instrumentation-click`: new instrumentation to trace click commands ([#2994](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2994)) -- `opentelemetry-instrumentation-falcon` Implement new HTTP semantic convention opt-in for Falcon - ([#2790](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2790)) ### Fixed From 8b281b263239a8e0067fcd8ab837d6f21f4f4b5b Mon Sep 17 00:00:00 2001 From: Filip Nikolovski Date: Thu, 2 Jan 2025 19:34:02 +0100 Subject: [PATCH 16/20] update tests --- .../tests/test_falcon.py | 80 ++++++++++++++----- 1 file changed, 60 insertions(+), 20 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py b/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py index 8761572159..611d5e02d9 100644 --- a/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py +++ b/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py @@ -12,6 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. # +from timeit import default_timer from unittest.mock import Mock, patch import pytest @@ -39,6 +40,24 @@ NumberDataPoint, ) from opentelemetry.sdk.resources import Resource +from opentelemetry.semconv.attributes.client_attributes import ( + CLIENT_PORT, +) +from opentelemetry.semconv.attributes.http_attributes import ( + HTTP_REQUEST_METHOD, + HTTP_RESPONSE_STATUS_CODE, +) +from opentelemetry.semconv.attributes.network_attributes import ( + NETWORK_PROTOCOL_VERSION, +) +from opentelemetry.semconv.attributes.server_attributes import ( + SERVER_ADDRESS, + SERVER_PORT, +) +from opentelemetry.semconv.attributes.url_attributes import ( + URL_PATH, + URL_SCHEME, +) from opentelemetry.semconv.trace import SpanAttributes from opentelemetry.test.test_base import TestBase from opentelemetry.test.wsgitestutil import WsgiTestBase @@ -219,17 +238,15 @@ def _test_method(self, method, old_semconv=True, new_semconv=False): SpanAttributes.HTTP_ROUTE: "/hello", } expected_attributes_new = { - SpanAttributes.HTTP_REQUEST_METHOD: method, - SpanAttributes.SERVER_ADDRESS: "falconframework.org", - SpanAttributes.URL_SCHEME: "http", - SpanAttributes.SERVER_PORT: 80, - SpanAttributes.URL_PATH: "/" - if self._has_fixed_http_target - else "/hello", - SpanAttributes.CLIENT_PORT: 65133, - SpanAttributes.NETWORK_PROTOCOL_VERSION: "1.1", + HTTP_REQUEST_METHOD: method, + SERVER_ADDRESS: "falconframework.org", + URL_SCHEME: "http", + SERVER_PORT: 80, + URL_PATH: "/" if self._has_fixed_http_target else "/hello", + CLIENT_PORT: 65133, + NETWORK_PROTOCOL_VERSION: "1.1", "falcon.resource": "HelloWorldResource", - SpanAttributes.HTTP_RESPONSE_STATUS_CODE: 201, + HTTP_RESPONSE_STATUS_CODE: 201, SpanAttributes.HTTP_ROUTE: "/hello", } @@ -342,17 +359,15 @@ def test_url_template_new_semconv(self): self.assertSpanHasAttributes( span, { - SpanAttributes.HTTP_REQUEST_METHOD: "GET", - SpanAttributes.SERVER_ADDRESS: "falconframework.org", - SpanAttributes.URL_SCHEME: "http", - SpanAttributes.SERVER_PORT: 80, - SpanAttributes.URL_PATH: "/" - if self._has_fixed_http_target - else "/user/123", - SpanAttributes.CLIENT_PORT: 65133, - SpanAttributes.NETWORK_PROTOCOL_VERSION: "1.1", + HTTP_REQUEST_METHOD: "GET", + SERVER_ADDRESS: "falconframework.org", + URL_SCHEME: "http", + SERVER_PORT: 80, + URL_PATH: "/" if self._has_fixed_http_target else "/user/123", + CLIENT_PORT: 65133, + NETWORK_PROTOCOL_VERSION: "1.1", "falcon.resource": "UserResource", - SpanAttributes.HTTP_RESPONSE_STATUS_CODE: 200, + HTTP_RESPONSE_STATUS_CODE: 200, SpanAttributes.HTTP_ROUTE: "/user/{user_id}", }, ) @@ -519,7 +534,10 @@ def test_falcon_metric_values_new_semconv(self): number_data_point_seen = False histogram_data_point_seen = False + start = default_timer() self.client().simulate_get("/hello/756") + duration = max(default_timer() - start, 0) + metrics_list = self.memory_metrics_reader.get_metrics_data() for resource_metric in metrics_list.resource_metrics: for scope_metric in resource_metric.scope_metrics: @@ -530,6 +548,9 @@ def test_falcon_metric_values_new_semconv(self): if isinstance(point, HistogramDataPoint): self.assertEqual(point.count, 1) histogram_data_point_seen = True + self.assertAlmostEqual( + duration, point.sum, delta=10 + ) if isinstance(point, NumberDataPoint): self.assertEqual(point.value, 0) number_data_point_seen = True @@ -545,7 +566,10 @@ def test_falcon_metric_values_both_semconv(self): number_data_point_seen = False histogram_data_point_seen = False + start = default_timer() self.client().simulate_get("/hello/756") + duration_s = default_timer() - start + metrics_list = self.memory_metrics_reader.get_metrics_data() for resource_metric in metrics_list.resource_metrics: for scope_metric in resource_metric.scope_metrics: @@ -565,6 +589,16 @@ def test_falcon_metric_values_both_semconv(self): for point in list(metric.data.data_points): if isinstance(point, HistogramDataPoint): self.assertEqual(point.count, 1) + if metric.unit == "ms": + self.assertAlmostEqual( + max(round(duration_s * 1000), 0), + point.sum, + delta=10, + ) + elif metric.unit == "s": + self.assertAlmostEqual( + max(duration_s, 0), point.sum, delta=10 + ) histogram_data_point_seen = True if isinstance(point, NumberDataPoint): self.assertEqual(point.value, 0) @@ -580,7 +614,10 @@ def test_falcon_metric_values(self): number_data_point_seen = False histogram_data_point_seen = False + start = default_timer() self.client().simulate_get("/hello/756") + duration = max(round((default_timer() - start) * 1000), 0) + metrics_list = self.memory_metrics_reader.get_metrics_data() for resource_metric in metrics_list.resource_metrics: for scope_metric in resource_metric.scope_metrics: @@ -591,6 +628,9 @@ def test_falcon_metric_values(self): if isinstance(point, HistogramDataPoint): self.assertEqual(point.count, 1) histogram_data_point_seen = True + self.assertAlmostEqual( + duration, point.sum, delta=10 + ) if isinstance(point, NumberDataPoint): self.assertEqual(point.value, 0) number_data_point_seen = True From c662eae5cba3062f58b739fefb9db1c4a26732ad Mon Sep 17 00:00:00 2001 From: Filip Nikolovski Date: Thu, 2 Jan 2025 19:42:19 +0100 Subject: [PATCH 17/20] disable lint check on too-many-nested-blocks in metrics test --- .../opentelemetry-instrumentation-falcon/tests/test_falcon.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py b/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py index 611d5e02d9..120de2f2df 100644 --- a/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py +++ b/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py @@ -571,6 +571,8 @@ def test_falcon_metric_values_both_semconv(self): duration_s = default_timer() - start 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_metric in resource_metric.scope_metrics: for metric in scope_metric.metrics: From 7702a595b141151c700cd6918571561b40c77837 Mon Sep 17 00:00:00 2001 From: Filip Nikolovski Date: Fri, 3 Jan 2025 11:35:01 +0100 Subject: [PATCH 18/20] use add_response_attributes function from wsgi --- .../instrumentation/falcon/__init__.py | 54 ++++++++++--------- 1 file changed, 28 insertions(+), 26 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py b/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py index caf6a6de39..b0acad8f9d 100644 --- a/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py @@ -514,7 +514,7 @@ def process_response(self, req, resp, resource, req_succeeded=None): # pylint:d status = resp.status if resource is None: - status = "404" + status = falcon.HTTP_404 else: exc_type, exc = None, None if _ENVIRON_EXC in req.env: @@ -523,37 +523,39 @@ def process_response(self, req, resp, resource, req_succeeded=None): # pylint:d if exc_type and not req_succeeded: if "HTTPNotFound" in exc_type.__name__: - status = "404" + status = falcon.HTTP_404 elif isinstance(exc, (falcon.HTTPError, falcon.HTTPStatus)): try: - status = exc.title.split(" ")[0] + if _falcon_version > 2: + status = falcon.code_to_http_status(exc.status) + else: + status = exc.status except ValueError: - status = "500" + status = falcon.HTTP_500 else: - status = "500" + status = falcon.HTTP_500 + + # Falcon 1 does not support response headers. So + # send an empty dict. + response_headers = {} + if _falcon_version > 1: + response_headers = resp.headers + + otel_wsgi.add_response_attributes( + span, + status, + response_headers, + req_attrs, + self._sem_conv_opt_in_mode, + ) - status_code = status.split(" ")[0] + if ( + _report_new(self._sem_conv_opt_in_mode) + and req.uri_template + and req_attrs is not None + ): + req_attrs[HTTP_ROUTE] = req.uri_template try: - set_status_code( - span, - status_code, - req_attrs, - sem_conv_opt_in_mode=self._sem_conv_opt_in_mode, - ) - - if ( - _report_new(self._sem_conv_opt_in_mode) - and req.uri_template - and req_attrs is not None - ): - req_attrs[HTTP_ROUTE] = req.uri_template - - # Falcon 1 does not support response headers. So - # send an empty dict. - response_headers = {} - if _falcon_version > 1: - response_headers = resp.headers - if span.is_recording() and span.kind == trace.SpanKind.SERVER: # Check if low-cardinality route is available as per semantic-conventions if req.uri_template: From 241a849771c0b7ba16a7ea740ccb40e60ffdff2c Mon Sep 17 00:00:00 2001 From: Filip Nikolovski Date: Fri, 3 Jan 2025 11:38:49 +0100 Subject: [PATCH 19/20] cleanup unused functions --- .../instrumentation/falcon/__init__.py | 26 ------------------- 1 file changed, 26 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py b/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py index b0acad8f9d..2b26c55cb1 100644 --- a/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py @@ -199,7 +199,6 @@ def response_hook(span, req, resp): _OpenTelemetryStabilitySignalType, _report_new, _report_old, - _set_status, _StabilityMode, ) from opentelemetry.instrumentation.falcon.package import _instruments @@ -250,31 +249,6 @@ def response_hook(span, req, resp): _falcon_version = 1 -def set_status_code( - span, - status_code, - metric_attributes=None, - sem_conv_opt_in_mode=_StabilityMode.DEFAULT, -): - """Adds HTTP response attributes to span using the status_code argument.""" - 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=True, - sem_conv_opt_in_mode=sem_conv_opt_in_mode, - ) - - class _InstrumentedFalconAPI(getattr(falcon, _instrument_app)): _instrumented_falcon_apps = set() From 2a5fac7dfb1cc0da86744ac9311de6632c4e5d22 Mon Sep 17 00:00:00 2001 From: Filip Nikolovski Date: Mon, 13 Jan 2025 11:41:27 +0100 Subject: [PATCH 20/20] cleanup --- .../tests/test_falcon.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py b/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py index 120de2f2df..48cbdbe3f8 100644 --- a/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py +++ b/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py @@ -471,7 +471,6 @@ def test_traced_not_recording(self): metrics_list = self.memory_metrics_reader.get_metrics_data() self.assertTrue(len(metrics_list.resource_metrics) != 0) - metrics_list = self.memory_metrics_reader.get_metrics_data() for resource_metric in metrics_list.resource_metrics: for scope_metric in resource_metric.scope_metrics: for metric in scope_metric.metrics: @@ -544,7 +543,7 @@ def test_falcon_metric_values_new_semconv(self): for metric in scope_metric.metrics: data_points = list(metric.data.data_points) self.assertEqual(len(data_points), 1) - for point in list(metric.data.data_points): + for point in data_points: if isinstance(point, HistogramDataPoint): self.assertEqual(point.count, 1) histogram_data_point_seen = True @@ -588,7 +587,7 @@ def test_falcon_metric_values_both_semconv(self): ) data_points = list(metric.data.data_points) self.assertEqual(len(data_points), 1) - for point in list(metric.data.data_points): + for point in data_points: if isinstance(point, HistogramDataPoint): self.assertEqual(point.count, 1) if metric.unit == "ms":