From b697f4ab9a3cf648ae004ef743b6e4564eaae6a2 Mon Sep 17 00:00:00 2001 From: Daniel Hochman Date: Fri, 12 Jul 2024 11:38:40 -0500 Subject: [PATCH 1/5] opentelemetry-instrumentation-asgi: always set status code on duration attributes (#2627) --- CHANGELOG.md | 2 + .../instrumentation/asgi/__init__.py | 50 +++++++-------- .../tests/test_asgi_middleware.py | 63 ++++++++++++++++++- .../opentelemetry/instrumentation/_semconv.py | 32 ++++++---- 4 files changed, 104 insertions(+), 43 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3eddf58cb8..76d77f06e0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -61,6 +61,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ([#2153](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2153)) - `opentelemetry-instrumentation-asgi` Removed `NET_HOST_NAME` AND `NET_HOST_PORT` from active requests count attribute ([#2610](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2610)) +- `opentelemetry-instrumentation-asgi` Bugfix: Middleware did not set status code attribute on duration metrics for non-recording spans. + ([#2627](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2627)) ## Version 1.25.0/0.46b0 (2024-05-31) diff --git a/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py b/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py index f006f9b0c9..7e51837d4d 100644 --- a/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py @@ -438,8 +438,6 @@ def set_status_code( sem_conv_opt_in_mode=_HTTPStabilityMode.DEFAULT, ): """Adds HTTP response attributes to span using the status_code argument.""" - if not span.is_recording(): - return status_code_str = str(status_code) try: @@ -836,36 +834,16 @@ async def otel_send(message: dict[str, Any]): ) as send_span: if callable(self.client_response_hook): self.client_response_hook(send_span, scope, message) + + status_code = None + if message["type"] == "http.response.start": + status_code = message["status"] + elif message["type"] == "websocket.send": + status_code = 200 + if send_span.is_recording(): if message["type"] == "http.response.start": - status_code = message["status"] - # We record metrics only once - set_status_code( - server_span, - status_code, - duration_attrs, - self._sem_conv_opt_in_mode, - ) - set_status_code( - send_span, - status_code, - None, - self._sem_conv_opt_in_mode, - ) expecting_trailers = message.get("trailers", False) - elif message["type"] == "websocket.send": - set_status_code( - server_span, - 200, - duration_attrs, - self._sem_conv_opt_in_mode, - ) - set_status_code( - send_span, - 200, - None, - self._sem_conv_opt_in_mode, - ) send_span.set_attribute("asgi.event.type", message["type"]) if ( server_span.is_recording() @@ -886,6 +864,20 @@ async def otel_send(message: dict[str, Any]): server_span.set_attributes( custom_response_attributes ) + if status_code: + # We record metrics only once + set_status_code( + server_span, + status_code, + duration_attrs, + self._sem_conv_opt_in_mode, + ) + set_status_code( + send_span, + status_code, + None, + self._sem_conv_opt_in_mode, + ) propagator = get_global_response_propagator() if propagator: diff --git a/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py b/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py index ff266cb5bf..5bb04adb25 100644 --- a/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py +++ b/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py @@ -514,7 +514,9 @@ def test_asgi_not_recording(self): mock_span = mock.Mock() mock_span.is_recording.return_value = False mock_tracer.start_as_current_span.return_value = mock_span - mock_tracer.start_as_current_span.return_value.__enter__ = mock_span + mock_tracer.start_as_current_span.return_value.__enter__ = mock.Mock( + return_value=mock_span + ) mock_tracer.start_as_current_span.return_value.__exit__ = mock_span with mock.patch("opentelemetry.trace.get_tracer") as tracer: tracer.return_value = mock_tracer @@ -1342,6 +1344,65 @@ def test_basic_metric_success(self): ) self.assertEqual(point.value, 0) + def test_basic_metric_success_nonrecording_span(self): + mock_tracer = mock.Mock() + mock_span = mock.Mock() + mock_span.is_recording.return_value = False + mock_tracer.start_as_current_span.return_value = mock_span + mock_tracer.start_as_current_span.return_value.__enter__ = mock.Mock( + return_value=mock_span + ) + mock_tracer.start_as_current_span.return_value.__exit__ = mock_span + with mock.patch("opentelemetry.trace.get_tracer") as tracer: + tracer.return_value = mock_tracer + app = otel_asgi.OpenTelemetryMiddleware(simple_asgi) + self.seed_app(app) + start = default_timer() + self.send_default_request() + duration = max(round((default_timer() - start) * 1000), 0) + expected_duration_attributes = { + "http.method": "GET", + "http.host": "127.0.0.1", + "http.scheme": "http", + "http.flavor": "1.0", + "net.host.port": 80, + "http.status_code": 200, + } + expected_requests_count_attributes = { + "http.method": "GET", + "http.host": "127.0.0.1", + "http.scheme": "http", + "http.flavor": "1.0", + } + metrics_list = self.memory_metrics_reader.get_metrics_data() + # pylint: disable=too-many-nested-blocks + for resource_metric in metrics_list.resource_metrics: + for scope_metrics in resource_metric.scope_metrics: + for metric in scope_metrics.metrics: + for point in list(metric.data.data_points): + if isinstance(point, HistogramDataPoint): + self.assertDictEqual( + expected_duration_attributes, + dict(point.attributes), + ) + self.assertEqual(point.count, 1) + if metric.name == "http.server.duration": + self.assertAlmostEqual( + duration, point.sum, delta=5 + ) + elif ( + metric.name == "http.server.response.size" + ): + self.assertEqual(1024, point.sum) + elif metric.name == "http.server.request.size": + self.assertEqual(128, point.sum) + elif isinstance(point, NumberDataPoint): + self.assertDictEqual( + expected_requests_count_attributes, + dict(point.attributes), + ) + self.assertEqual(point.value, 0) + def test_basic_metric_success_new_semconv(self): app = otel_asgi.OpenTelemetryMiddleware(simple_asgi) self.seed_app(app) diff --git a/opentelemetry-instrumentation/src/opentelemetry/instrumentation/_semconv.py b/opentelemetry-instrumentation/src/opentelemetry/instrumentation/_semconv.py index 85b8e2e3ec..7e81692a98 100644 --- a/opentelemetry-instrumentation/src/opentelemetry/instrumentation/_semconv.py +++ b/opentelemetry-instrumentation/src/opentelemetry/instrumentation/_semconv.py @@ -355,29 +355,35 @@ def _set_status( sem_conv_opt_in_mode, ): if status_code < 0: - if _report_new(sem_conv_opt_in_mode): - span.set_attribute(ERROR_TYPE, status_code_str) - metrics_attributes[ERROR_TYPE] = status_code_str - - span.set_status( - Status( - StatusCode.ERROR, - "Non-integer HTTP status: " + status_code_str, + metrics_attributes[ERROR_TYPE] = status_code_str + if span.is_recording(): + if _report_new(sem_conv_opt_in_mode): + span.set_attribute(ERROR_TYPE, status_code_str) + span.set_status( + Status( + StatusCode.ERROR, + "Non-integer HTTP status: " + status_code_str, + ) ) - ) else: status = http_status_to_status_code(status_code, server_span=True) if _report_old(sem_conv_opt_in_mode): - span.set_attribute(SpanAttributes.HTTP_STATUS_CODE, status_code) + if span.is_recording(): + span.set_attribute( + SpanAttributes.HTTP_STATUS_CODE, status_code + ) metrics_attributes[SpanAttributes.HTTP_STATUS_CODE] = status_code if _report_new(sem_conv_opt_in_mode): - span.set_attribute(HTTP_RESPONSE_STATUS_CODE, status_code) + if span.is_recording(): + span.set_attribute(HTTP_RESPONSE_STATUS_CODE, status_code) metrics_attributes[HTTP_RESPONSE_STATUS_CODE] = status_code if status == StatusCode.ERROR: - span.set_attribute(ERROR_TYPE, status_code_str) + if span.is_recording(): + span.set_attribute(ERROR_TYPE, status_code_str) metrics_attributes[ERROR_TYPE] = status_code_str - span.set_status(Status(status)) + if span.is_recording(): + span.set_status(Status(status)) # Get schema version based off of opt-in mode From 5a2794692052deae7b70f66a95dbaafbe6dcba6c Mon Sep 17 00:00:00 2001 From: Leighton Chen Date: Fri, 12 Jul 2024 10:49:48 -0700 Subject: [PATCH 2/5] =?UTF-8?q?Add=20Em=C3=ADdio=20Neto=20as=20an=20approv?= =?UTF-8?q?er=20(#2703)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index 6c019239ae..dcb2ebbba5 100644 --- a/README.md +++ b/README.md @@ -110,6 +110,7 @@ Meeting notes are available as a public [Google doc](https://docs.google.com/doc Approvers ([@open-telemetry/python-approvers](https://github.com/orgs/open-telemetry/teams/python-approvers)): - [Aaron Abbott](https://github.com/aabmass), Google +- [Emídio Neto](https://github.com/emdneto), Zenvia - [Jeremy Voss](https://github.com/jeremydvoss), Microsoft - [Owais Lone](https://github.com/owais), Splunk - [Pablo Collins](https://github.com/pmcollins), Splunk From 6293d6a991d283b6d754d4736916db2d60ce8e5c Mon Sep 17 00:00:00 2001 From: Leighton Chen Date: Fri, 12 Jul 2024 11:48:04 -0700 Subject: [PATCH 3/5] HTTP semantic convention stability migration for fastapi (#2682) --- CHANGELOG.md | 7 +- instrumentation/README.md | 2 +- .../instrumentation/fastapi/__init__.py | 34 +- .../instrumentation/fastapi/package.py | 2 + .../tests/test_fastapi_instrumentation.py | 514 +++++++++++++++++- .../instrumentation/flask/__init__.py | 8 +- 6 files changed, 527 insertions(+), 40 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 76d77f06e0..b6882cbe57 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ([#2638](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2638)) - `opentelemetry-instrumentation-asgi` Implement new semantic convention opt-in with stable http semantic conventions ([#2610](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2610)) +- `opentelemetry-instrumentation-fastapi` Implement new semantic convention opt-in with stable http semantic conventions + ([#2682](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2682)) - `opentelemetry-instrumentation-httpx` Implement new semantic convention opt-in migration with stable http semantic conventions ([#2631](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2631)) - `opentelemetry-instrumentation-system-metrics` Permit to use psutil 6.0+. @@ -32,9 +34,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `opentelemetry-instrumentation-asgi`, `opentelemetry-instrumentation-fastapi`, `opentelemetry-instrumentation-starlette` Use `tracer` and `meter` of originating components instead of one from `asgi` middleware ([#2580](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2580)) -- Populate `{method}` as `HTTP` on `_OTHER` methods from scope +- Populate `{method}` as `HTTP` on `_OTHER` methods from scope for `asgi` middleware ([#2610](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2610)) - + - Populate `{method}` as `HTTP` on `_OTHER` methods from scope for `fastapi` middleware + ([#2682](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2682)) ### Fixed diff --git a/instrumentation/README.md b/instrumentation/README.md index eb21717843..9add6d0228 100644 --- a/instrumentation/README.md +++ b/instrumentation/README.md @@ -19,7 +19,7 @@ | [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-fastapi](./opentelemetry-instrumentation-fastapi) | fastapi ~= 0.58 | Yes | experimental +| [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.27 | No | experimental | [opentelemetry-instrumentation-httpx](./opentelemetry-instrumentation-httpx) | httpx >= 0.18.0 | No | migration diff --git a/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py b/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py index 4c673d214a..2f9bbe7314 100644 --- a/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py @@ -177,6 +177,12 @@ def client_response_hook(span: Span, scope: dict[str, Any], message: dict[str, A import fastapi from starlette.routing import Match +from opentelemetry.instrumentation._semconv import ( + _get_schema_url, + _HTTPStabilityMode, + _OpenTelemetrySemanticConventionStability, + _OpenTelemetryStabilitySignalType, +) from opentelemetry.instrumentation.asgi import OpenTelemetryMiddleware from opentelemetry.instrumentation.asgi.types import ( ClientRequestHook, @@ -189,7 +195,11 @@ def client_response_hook(span: Span, scope: dict[str, Any], message: dict[str, A from opentelemetry.metrics import get_meter from opentelemetry.semconv.trace import SpanAttributes from opentelemetry.trace import get_tracer -from opentelemetry.util.http import get_excluded_urls, parse_excluded_urls +from opentelemetry.util.http import ( + get_excluded_urls, + parse_excluded_urls, + sanitize_method, +) _excluded_urls_from_env = get_excluded_urls("FASTAPI") _logger = logging.getLogger(__name__) @@ -218,6 +228,11 @@ def instrument_app( app._is_instrumented_by_opentelemetry = False if not getattr(app, "_is_instrumented_by_opentelemetry", False): + # initialize semantic conventions opt-in if needed + _OpenTelemetrySemanticConventionStability._initialize() + sem_conv_opt_in_mode = _OpenTelemetrySemanticConventionStability._get_opentelemetry_stability_opt_in_mode( + _OpenTelemetryStabilitySignalType.HTTP, + ) if excluded_urls is None: excluded_urls = _excluded_urls_from_env else: @@ -226,13 +241,13 @@ def instrument_app( __name__, __version__, tracer_provider, - schema_url="https://opentelemetry.io/schemas/1.11.0", + schema_url=_get_schema_url(sem_conv_opt_in_mode), ) meter = get_meter( __name__, __version__, meter_provider, - schema_url="https://opentelemetry.io/schemas/1.11.0", + schema_url=_get_schema_url(sem_conv_opt_in_mode), ) app.add_middleware( @@ -303,6 +318,7 @@ class _InstrumentedFastAPI(fastapi.FastAPI): _client_request_hook: ClientRequestHook = None _client_response_hook: ClientResponseHook = None _instrumented_fastapi_apps = set() + _sem_conv_opt_in_mode = _HTTPStabilityMode.DEFAULT def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) @@ -310,13 +326,17 @@ def __init__(self, *args, **kwargs): __name__, __version__, _InstrumentedFastAPI._tracer_provider, - schema_url="https://opentelemetry.io/schemas/1.11.0", + schema_url=_get_schema_url( + _InstrumentedFastAPI._sem_conv_opt_in_mode + ), ) meter = get_meter( __name__, __version__, _InstrumentedFastAPI._meter_provider, - schema_url="https://opentelemetry.io/schemas/1.11.0", + schema_url=_get_schema_url( + _InstrumentedFastAPI._sem_conv_opt_in_mode + ), ) self.add_middleware( OpenTelemetryMiddleware, @@ -373,8 +393,10 @@ def _get_default_span_details(scope): A tuple of span name and attributes """ route = _get_route_details(scope) - method = scope.get("method", "") + method = sanitize_method(scope.get("method", "").strip()) attributes = {} + if method == "_OTHER": + method = "HTTP" if route: attributes[SpanAttributes.HTTP_ROUTE] = route if method and route: # http diff --git a/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/package.py b/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/package.py index 8df84fc931..d95a2cf6d5 100644 --- a/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/package.py +++ b/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/package.py @@ -16,3 +16,5 @@ _instruments = ("fastapi ~= 0.58",) _supports_metrics = True + +_semconv_status = "migration" diff --git a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py index 0ad63164d5..69c5d312e5 100644 --- a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py +++ b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py @@ -11,6 +11,9 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. + +# pylint: disable=too-many-lines + import unittest from timeit import default_timer from unittest.mock import patch @@ -20,39 +23,77 @@ from fastapi.testclient import TestClient import opentelemetry.instrumentation.fastapi as otel_fastapi +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, + _server_duration_attrs_old, +) from opentelemetry.instrumentation.asgi import OpenTelemetryMiddleware from opentelemetry.sdk.metrics.export import ( HistogramDataPoint, NumberDataPoint, ) from opentelemetry.sdk.resources import Resource +from opentelemetry.semconv.attributes.http_attributes import ( + HTTP_REQUEST_METHOD, + HTTP_RESPONSE_STATUS_CODE, + HTTP_ROUTE, +) +from opentelemetry.semconv.attributes.network_attributes import ( + NETWORK_PROTOCOL_VERSION, +) +from opentelemetry.semconv.attributes.url_attributes import URL_SCHEME from opentelemetry.semconv.trace import SpanAttributes from opentelemetry.test.test_base import TestBase -from opentelemetry.util.http import ( - _active_requests_count_attrs, - _duration_attrs, - get_excluded_urls, -) +from opentelemetry.util.http import get_excluded_urls -_expected_metric_names = [ +_expected_metric_names_old = [ "http.server.active_requests", "http.server.duration", "http.server.response.size", "http.server.request.size", ] -_recommended_attrs = { - "http.server.active_requests": _active_requests_count_attrs, - "http.server.duration": {*_duration_attrs, SpanAttributes.HTTP_TARGET}, +_expected_metric_names_new = [ + "http.server.active_requests", + "http.server.request.duration", + "http.server.response.body.size", + "http.server.request.body.size", +] +_expected_metric_names_both = _expected_metric_names_old +_expected_metric_names_both.extend(_expected_metric_names_new) + +_recommended_attrs_old = { + "http.server.active_requests": _server_active_requests_count_attrs_old, + "http.server.duration": { + *_server_duration_attrs_old, + SpanAttributes.HTTP_TARGET, + }, "http.server.response.size": { - *_duration_attrs, + *_server_duration_attrs_old, SpanAttributes.HTTP_TARGET, }, "http.server.request.size": { - *_duration_attrs, + *_server_duration_attrs_old, SpanAttributes.HTTP_TARGET, }, } +_recommended_attrs_new = { + "http.server.active_requests": _server_active_requests_count_attrs_new, + "http.server.request.duration": _server_duration_attrs_new, + "http.server.response.body.size": _server_duration_attrs_new, + "http.server.request.body.size": _server_duration_attrs_new, +} + +_recommended_attrs_both = _recommended_attrs_old.copy() +_recommended_attrs_both.update(_recommended_attrs_new) +_recommended_attrs_both["http.server.active_requests"].extend( + _server_active_requests_count_attrs_old +) + class TestBaseFastAPI(TestBase): def _create_app(self): @@ -88,10 +129,23 @@ def setUpClass(cls): 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_FASTAPI_EXCLUDED_URLS": "/exclude/123,healthzz"}, + { + "OTEL_PYTHON_FASTAPI_EXCLUDED_URLS": "/exclude/123,healthzz", + OTEL_SEMCONV_STABILITY_OPT_IN: sem_conv_mode, + }, ) + _OpenTelemetrySemanticConventionStability._initialized = False self.env_patch.start() self.exclude_patch = patch( "opentelemetry.instrumentation.fastapi._excluded_urls_from_env", @@ -142,7 +196,6 @@ async def _(): class TestBaseManualFastAPI(TestBaseFastAPI): - @classmethod def setUpClass(cls): if cls is TestBaseManualFastAPI: @@ -196,7 +249,6 @@ def test_sub_app_fastapi_call(self): class TestBaseAutoFastAPI(TestBaseFastAPI): - @classmethod def setUpClass(cls): if cls is TestBaseAutoFastAPI: @@ -259,6 +311,7 @@ def test_sub_app_fastapi_call(self): ) +# pylint: disable=too-many-public-methods class TestFastAPIManualInstrumentation(TestBaseManualFastAPI): def test_instrument_app_with_instrument(self): if not isinstance(self, TestAutoInstrumentation): @@ -358,7 +411,71 @@ def test_fastapi_metrics(self): ) self.assertTrue(len(scope_metric.metrics) == 3) for metric in scope_metric.metrics: - self.assertIn(metric.name, _expected_metric_names) + self.assertIn(metric.name, _expected_metric_names_old) + data_points = list(metric.data.data_points) + self.assertEqual(len(data_points), 1) + for point in data_points: + if isinstance(point, HistogramDataPoint): + self.assertEqual(point.count, 3) + histogram_data_point_seen = True + if isinstance(point, NumberDataPoint): + number_data_point_seen = True + for attr in point.attributes: + self.assertIn( + attr, _recommended_attrs_old[metric.name] + ) + self.assertTrue(number_data_point_seen and histogram_data_point_seen) + + def test_fastapi_metrics_new_semconv(self): + self._client.get("/foobar") + self._client.get("/foobar") + self._client.get("/foobar") + metrics_list = self.memory_metrics_reader.get_metrics_data() + number_data_point_seen = False + histogram_data_point_seen = False + self.assertTrue(len(metrics_list.resource_metrics) == 1) + for resource_metric in metrics_list.resource_metrics: + self.assertTrue(len(resource_metric.scope_metrics) == 1) + for scope_metric in resource_metric.scope_metrics: + self.assertEqual( + scope_metric.scope.name, + "opentelemetry.instrumentation.fastapi", + ) + self.assertTrue(len(scope_metric.metrics) == 3) + for metric in scope_metric.metrics: + self.assertIn(metric.name, _expected_metric_names_new) + data_points = list(metric.data.data_points) + self.assertEqual(len(data_points), 1) + for point in data_points: + if isinstance(point, HistogramDataPoint): + self.assertEqual(point.count, 3) + histogram_data_point_seen = True + if isinstance(point, NumberDataPoint): + number_data_point_seen = True + for attr in point.attributes: + self.assertIn( + attr, _recommended_attrs_new[metric.name] + ) + self.assertTrue(number_data_point_seen and histogram_data_point_seen) + + def test_fastapi_metrics_both_semconv(self): + self._client.get("/foobar") + self._client.get("/foobar") + self._client.get("/foobar") + metrics_list = self.memory_metrics_reader.get_metrics_data() + number_data_point_seen = False + histogram_data_point_seen = False + self.assertTrue(len(metrics_list.resource_metrics) == 1) + for resource_metric in metrics_list.resource_metrics: + self.assertTrue(len(resource_metric.scope_metrics) == 1) + for scope_metric in resource_metric.scope_metrics: + self.assertEqual( + scope_metric.scope.name, + "opentelemetry.instrumentation.fastapi", + ) + self.assertTrue(len(scope_metric.metrics) == 5) + for metric in scope_metric.metrics: + self.assertIn(metric.name, _expected_metric_names_both) data_points = list(metric.data.data_points) self.assertEqual(len(data_points), 1) for point in data_points: @@ -369,7 +486,7 @@ def test_fastapi_metrics(self): number_data_point_seen = True for attr in point.attributes: self.assertIn( - attr, _recommended_attrs[metric.name] + attr, _recommended_attrs_both[metric.name] ) self.assertTrue(number_data_point_seen and histogram_data_point_seen) @@ -378,21 +495,216 @@ def test_basic_metric_success(self): self._client.get("/foobar") duration = max(round((default_timer() - start) * 1000), 0) expected_duration_attributes = { - "http.method": "GET", - "http.host": "testserver:443", - "http.scheme": "https", - "http.flavor": "1.1", - "http.server_name": "testserver", - "net.host.port": 443, - "http.status_code": 200, - "http.target": "/foobar", + SpanAttributes.HTTP_METHOD: "GET", + SpanAttributes.HTTP_HOST: "testserver:443", + SpanAttributes.HTTP_SCHEME: "https", + SpanAttributes.HTTP_FLAVOR: "1.1", + SpanAttributes.HTTP_SERVER_NAME: "testserver", + SpanAttributes.NET_HOST_PORT: 443, + SpanAttributes.HTTP_STATUS_CODE: 200, + SpanAttributes.HTTP_TARGET: "/foobar", + } + expected_requests_count_attributes = { + SpanAttributes.HTTP_METHOD: "GET", + SpanAttributes.HTTP_HOST: "testserver:443", + SpanAttributes.HTTP_SCHEME: "https", + SpanAttributes.HTTP_FLAVOR: "1.1", + SpanAttributes.HTTP_SERVER_NAME: "testserver", + } + metrics_list = self.memory_metrics_reader.get_metrics_data() + for metric in ( + metrics_list.resource_metrics[0].scope_metrics[0].metrics + ): + 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=40) + if isinstance(point, NumberDataPoint): + self.assertDictEqual( + expected_requests_count_attributes, + dict(point.attributes), + ) + self.assertEqual(point.value, 0) + + def test_basic_metric_success_new_semconv(self): + start = default_timer() + self._client.get("/foobar") + duration_s = max(default_timer() - start, 0) + expected_duration_attributes = { + HTTP_REQUEST_METHOD: "GET", + URL_SCHEME: "https", + NETWORK_PROTOCOL_VERSION: "1.1", + HTTP_RESPONSE_STATUS_CODE: 200, + HTTP_ROUTE: "/foobar", + } + expected_requests_count_attributes = { + HTTP_REQUEST_METHOD: "GET", + URL_SCHEME: "https", + } + metrics_list = self.memory_metrics_reader.get_metrics_data() + for metric in ( + metrics_list.resource_metrics[0].scope_metrics[0].metrics + ): + 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) + if metric.name == "http.server.request.duration": + self.assertAlmostEqual(duration_s, point.sum, places=1) + elif metric.name == "http.server.response.body.size": + self.assertEqual(25, point.sum) + elif metric.name == "http.server.request.body.size": + self.assertEqual(25, point.sum) + if isinstance(point, NumberDataPoint): + self.assertDictEqual( + expected_requests_count_attributes, + dict(point.attributes), + ) + self.assertEqual(point.value, 0) + + def test_basic_metric_success_both_semconv(self): + start = default_timer() + self._client.get("/foobar") + duration = max(round((default_timer() - start) * 1000), 0) + duration_s = max(default_timer() - start, 0) + expected_duration_attributes_old = { + SpanAttributes.HTTP_METHOD: "GET", + SpanAttributes.HTTP_HOST: "testserver:443", + SpanAttributes.HTTP_SCHEME: "https", + SpanAttributes.HTTP_FLAVOR: "1.1", + SpanAttributes.HTTP_SERVER_NAME: "testserver", + SpanAttributes.NET_HOST_PORT: 443, + SpanAttributes.HTTP_STATUS_CODE: 200, + SpanAttributes.HTTP_TARGET: "/foobar", + } + expected_duration_attributes_new = { + HTTP_REQUEST_METHOD: "GET", + URL_SCHEME: "https", + NETWORK_PROTOCOL_VERSION: "1.1", + HTTP_RESPONSE_STATUS_CODE: 200, + HTTP_ROUTE: "/foobar", + } + expected_requests_count_attributes = { + SpanAttributes.HTTP_METHOD: "GET", + SpanAttributes.HTTP_HOST: "testserver:443", + SpanAttributes.HTTP_SCHEME: "https", + SpanAttributes.HTTP_FLAVOR: "1.1", + SpanAttributes.HTTP_SERVER_NAME: "testserver", + HTTP_REQUEST_METHOD: "GET", + URL_SCHEME: "https", + } + metrics_list = self.memory_metrics_reader.get_metrics_data() + for metric in ( + metrics_list.resource_metrics[0].scope_metrics[0].metrics + ): + for point in list(metric.data.data_points): + if isinstance(point, HistogramDataPoint): + self.assertEqual(point.count, 1) + self.assertAlmostEqual(duration, point.sum, delta=40) + if metric.name == "http.server.request.duration": + self.assertAlmostEqual(duration_s, point.sum, places=1) + self.assertDictEqual( + expected_duration_attributes_new, + dict(point.attributes), + ) + elif metric.name == "http.server.response.body.size": + self.assertEqual(25, point.sum) + self.assertDictEqual( + expected_duration_attributes_new, + dict(point.attributes), + ) + elif metric.name == "http.server.request.body.size": + self.assertEqual(25, point.sum) + self.assertDictEqual( + expected_duration_attributes_new, + dict(point.attributes), + ) + elif metric.name == "http.server.duration": + self.assertAlmostEqual(duration, point.sum, delta=40) + self.assertDictEqual( + expected_duration_attributes_old, + dict(point.attributes), + ) + elif metric.name == "http.server.response.size": + self.assertEqual(25, point.sum) + self.assertDictEqual( + expected_duration_attributes_old, + dict(point.attributes), + ) + elif metric.name == "http.server.request.size": + self.assertEqual(25, point.sum) + self.assertDictEqual( + expected_duration_attributes_old, + dict(point.attributes), + ) + if isinstance(point, NumberDataPoint): + self.assertDictEqual( + expected_requests_count_attributes, + dict(point.attributes), + ) + self.assertEqual(point.value, 0) + + def test_basic_metric_nonstandard_http_method_success(self): + start = default_timer() + self._client.request("NONSTANDARD", "/foobar") + duration = max(round((default_timer() - start) * 1000), 0) + expected_duration_attributes = { + SpanAttributes.HTTP_METHOD: "_OTHER", + SpanAttributes.HTTP_HOST: "testserver:443", + SpanAttributes.HTTP_SCHEME: "https", + SpanAttributes.HTTP_FLAVOR: "1.1", + SpanAttributes.HTTP_SERVER_NAME: "testserver", + SpanAttributes.NET_HOST_PORT: 443, + SpanAttributes.HTTP_STATUS_CODE: 405, + SpanAttributes.HTTP_TARGET: "/foobar", + } + expected_requests_count_attributes = { + SpanAttributes.HTTP_METHOD: "_OTHER", + SpanAttributes.HTTP_HOST: "testserver:443", + SpanAttributes.HTTP_SCHEME: "https", + SpanAttributes.HTTP_FLAVOR: "1.1", + SpanAttributes.HTTP_SERVER_NAME: "testserver", + } + metrics_list = self.memory_metrics_reader.get_metrics_data() + for metric in ( + metrics_list.resource_metrics[0].scope_metrics[0].metrics + ): + 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=40) + if isinstance(point, NumberDataPoint): + self.assertDictEqual( + expected_requests_count_attributes, + dict(point.attributes), + ) + self.assertEqual(point.value, 0) + + def test_basic_metric_nonstandard_http_method_success_new_semconv(self): + start = default_timer() + self._client.request("NONSTANDARD", "/foobar") + duration_s = max(default_timer() - start, 0) + expected_duration_attributes = { + HTTP_REQUEST_METHOD: "_OTHER", + URL_SCHEME: "https", + NETWORK_PROTOCOL_VERSION: "1.1", + HTTP_RESPONSE_STATUS_CODE: 405, + HTTP_ROUTE: "/foobar", } expected_requests_count_attributes = { - "http.method": "GET", - "http.host": "testserver:443", - "http.scheme": "https", - "http.flavor": "1.1", - "http.server_name": "testserver", + HTTP_REQUEST_METHOD: "_OTHER", + URL_SCHEME: "https", } metrics_list = self.memory_metrics_reader.get_metrics_data() for metric in ( @@ -404,8 +716,95 @@ def test_basic_metric_success(self): expected_duration_attributes, dict(point.attributes), ) + self.assertEqual(point.count, 1) + if metric.name == "http.server.request.duration": + self.assertAlmostEqual(duration_s, point.sum, places=1) + elif metric.name == "http.server.response.body.size": + self.assertEqual(31, point.sum) + elif metric.name == "http.server.request.body.size": + self.assertEqual(25, point.sum) + if isinstance(point, NumberDataPoint): + self.assertDictEqual( + expected_requests_count_attributes, + dict(point.attributes), + ) + self.assertEqual(point.value, 0) + + def test_basic_metric_nonstandard_http_method_success_both_semconv(self): + start = default_timer() + self._client.request("NONSTANDARD", "/foobar") + duration = max(round((default_timer() - start) * 1000), 0) + duration_s = max(default_timer() - start, 0) + expected_duration_attributes_old = { + SpanAttributes.HTTP_METHOD: "_OTHER", + SpanAttributes.HTTP_HOST: "testserver:443", + SpanAttributes.HTTP_SCHEME: "https", + SpanAttributes.HTTP_FLAVOR: "1.1", + SpanAttributes.HTTP_SERVER_NAME: "testserver", + SpanAttributes.NET_HOST_PORT: 443, + SpanAttributes.HTTP_STATUS_CODE: 405, + SpanAttributes.HTTP_TARGET: "/foobar", + } + expected_duration_attributes_new = { + HTTP_REQUEST_METHOD: "_OTHER", + URL_SCHEME: "https", + NETWORK_PROTOCOL_VERSION: "1.1", + HTTP_RESPONSE_STATUS_CODE: 405, + HTTP_ROUTE: "/foobar", + } + expected_requests_count_attributes = { + SpanAttributes.HTTP_METHOD: "_OTHER", + SpanAttributes.HTTP_HOST: "testserver:443", + SpanAttributes.HTTP_SCHEME: "https", + SpanAttributes.HTTP_FLAVOR: "1.1", + SpanAttributes.HTTP_SERVER_NAME: "testserver", + HTTP_REQUEST_METHOD: "_OTHER", + URL_SCHEME: "https", + } + metrics_list = self.memory_metrics_reader.get_metrics_data() + for metric in ( + metrics_list.resource_metrics[0].scope_metrics[0].metrics + ): + for point in list(metric.data.data_points): + if isinstance(point, HistogramDataPoint): self.assertEqual(point.count, 1) self.assertAlmostEqual(duration, point.sum, delta=40) + if metric.name == "http.server.request.duration": + self.assertAlmostEqual(duration_s, point.sum, places=1) + self.assertDictEqual( + expected_duration_attributes_new, + dict(point.attributes), + ) + elif metric.name == "http.server.response.body.size": + self.assertEqual(31, point.sum) + self.assertDictEqual( + expected_duration_attributes_new, + dict(point.attributes), + ) + elif metric.name == "http.server.request.body.size": + self.assertEqual(25, point.sum) + self.assertDictEqual( + expected_duration_attributes_new, + dict(point.attributes), + ) + elif metric.name == "http.server.duration": + self.assertAlmostEqual(duration, point.sum, delta=40) + self.assertDictEqual( + expected_duration_attributes_old, + dict(point.attributes), + ) + elif metric.name == "http.server.response.size": + self.assertEqual(31, point.sum) + self.assertDictEqual( + expected_duration_attributes_old, + dict(point.attributes), + ) + elif metric.name == "http.server.request.size": + self.assertEqual(25, point.sum) + self.assertDictEqual( + expected_duration_attributes_old, + dict(point.attributes), + ) if isinstance(point, NumberDataPoint): self.assertDictEqual( expected_requests_count_attributes, @@ -438,6 +837,63 @@ def test_basic_post_request_metric_success(self): if isinstance(point, NumberDataPoint): self.assertEqual(point.value, 0) + def test_basic_post_request_metric_success_new_semconv(self): + start = default_timer() + response = self._client.post( + "/foobar", + json={"foo": "bar"}, + ) + duration_s = max(default_timer() - start, 0) + response_size = int(response.headers.get("content-length")) + request_size = int(response.request.headers.get("content-length")) + metrics_list = self.memory_metrics_reader.get_metrics_data() + for metric in ( + metrics_list.resource_metrics[0].scope_metrics[0].metrics + ): + for point in list(metric.data.data_points): + if isinstance(point, HistogramDataPoint): + self.assertEqual(point.count, 1) + if metric.name == "http.server.request.duration": + self.assertAlmostEqual(duration_s, point.sum, places=1) + elif metric.name == "http.server.response.body.size": + self.assertEqual(response_size, point.sum) + elif metric.name == "http.server.request.body.size": + self.assertEqual(request_size, point.sum) + if isinstance(point, NumberDataPoint): + self.assertEqual(point.value, 0) + + def test_basic_post_request_metric_success_both_semconv(self): + start = default_timer() + response = self._client.post( + "/foobar", + json={"foo": "bar"}, + ) + duration = max(round((default_timer() - start) * 1000), 0) + duration_s = max(default_timer() - start, 0) + response_size = int(response.headers.get("content-length")) + request_size = int(response.request.headers.get("content-length")) + metrics_list = self.memory_metrics_reader.get_metrics_data() + for metric in ( + metrics_list.resource_metrics[0].scope_metrics[0].metrics + ): + for point in list(metric.data.data_points): + if isinstance(point, HistogramDataPoint): + self.assertEqual(point.count, 1) + if metric.name == "http.server.request.duration": + self.assertAlmostEqual(duration_s, point.sum, places=1) + elif metric.name == "http.server.response.body.size": + self.assertEqual(response_size, point.sum) + elif metric.name == "http.server.request.body.size": + self.assertEqual(request_size, point.sum) + elif metric.name == "http.server.duration": + self.assertAlmostEqual(duration, point.sum, delta=40) + elif metric.name == "http.server.response.size": + self.assertEqual(response_size, point.sum) + elif metric.name == "http.server.request.size": + self.assertEqual(request_size, point.sum) + if isinstance(point, NumberDataPoint): + self.assertEqual(point.value, 0) + def test_metric_uninstrument_app(self): self._client.get("/foobar") self._instrumentor.uninstrument_app(self._app) diff --git a/instrumentation/opentelemetry-instrumentation-flask/src/opentelemetry/instrumentation/flask/__init__.py b/instrumentation/opentelemetry-instrumentation-flask/src/opentelemetry/instrumentation/flask/__init__.py index 34e9b5ea50..eaf6c79506 100644 --- a/instrumentation/opentelemetry-instrumentation-flask/src/opentelemetry/instrumentation/flask/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-flask/src/opentelemetry/instrumentation/flask/__init__.py @@ -543,7 +543,9 @@ def __init__(self, *args, **kwargs): __name__, __version__, _InstrumentedFlask._meter_provider, - schema_url="https://opentelemetry.io/schemas/1.11.0", + schema_url=_get_schema_url( + _InstrumentedFlask._sem_conv_opt_in_mode + ), ) duration_histogram_old = None if _report_old(_InstrumentedFlask._sem_conv_opt_in_mode): @@ -579,7 +581,9 @@ def __init__(self, *args, **kwargs): __name__, __version__, _InstrumentedFlask._tracer_provider, - schema_url="https://opentelemetry.io/schemas/1.11.0", + schema_url=_get_schema_url( + _InstrumentedFlask._sem_conv_opt_in_mode + ), ) _before_request = _wrapped_before_request( From 15f3b97d5a49a3c616e3691af2b31d3f0013bb65 Mon Sep 17 00:00:00 2001 From: Gustavo Carvalho <99373133+gustavosett@users.noreply.github.com> Date: Fri, 12 Jul 2024 19:44:44 -0300 Subject: [PATCH 4/5] Improved Test Coverage for HTTP Utility's IP Setting Functionality (#2693) --- .../tests/test_try_set_ip.py | 67 +++++++++++++++++++ 1 file changed, 67 insertions(+) create mode 100644 util/opentelemetry-util-http/tests/test_try_set_ip.py diff --git a/util/opentelemetry-util-http/tests/test_try_set_ip.py b/util/opentelemetry-util-http/tests/test_try_set_ip.py new file mode 100644 index 0000000000..c87bf990a6 --- /dev/null +++ b/util/opentelemetry-util-http/tests/test_try_set_ip.py @@ -0,0 +1,67 @@ +# Copyright The OpenTelemetry Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import logging +import socket +import unittest +from http.client import HTTPConnection +from unittest.mock import MagicMock, patch + +from opentelemetry.util.http.httplib import trysetip + + +class TestTrySetIP(unittest.TestCase): + def setUp(self): + # Setup a mock HTTPConnection + self.conn = MagicMock(spec=HTTPConnection) + self.conn.sock = MagicMock(spec=socket.socket) + + # Mock state function and Span class + self.mock_state = {"need_ip": [MagicMock()]} + self.mock_getstate = patch( + "opentelemetry.util.http.httplib._getstate", + return_value=self.mock_state, + ) + self.mock_getstate.start() + + def test_ip_set_successfully(self): + self.conn.sock.getpeername.return_value = ("192.168.1.1", 8080) + + success = trysetip(self.conn, loglevel=logging.DEBUG) + + # Verify that the IP was set correctly + for span in self.mock_state["need_ip"]: + span.set_attribute.assert_called_once_with( + "net.peer.ip", "192.168.1.1" + ) + self.assertTrue(success) + + def test_no_socket_connection(self): + # Setup the connection with no socket + self.conn.sock = None + + success = trysetip(self.conn, loglevel=logging.DEBUG) + + self.assertFalse(success) + + def test_exception_during_ip_retrieval(self): + self.conn.sock.getpeername.side_effect = Exception("Test Exception") + + with self.assertLogs(level=logging.WARNING) as warning: + success = trysetip(self.conn, loglevel=logging.WARNING) + self.assertEqual(len(warning.records), 1) + self.assertIn( + "Failed to get peer address", warning.records[0].message + ) + self.assertTrue(success) From 6bc48be45af45769ed071b14d684d1d40b3d155e Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Sat, 13 Jul 2024 01:34:34 +0200 Subject: [PATCH 5/5] exporter/prometheus-remote-write: bump certifi to latest (#2701) Co-authored-by: Leighton Chen --- .../test-requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/exporter/opentelemetry-exporter-prometheus-remote-write/test-requirements.txt b/exporter/opentelemetry-exporter-prometheus-remote-write/test-requirements.txt index ec8f90336d..f7e66ddd13 100644 --- a/exporter/opentelemetry-exporter-prometheus-remote-write/test-requirements.txt +++ b/exporter/opentelemetry-exporter-prometheus-remote-write/test-requirements.txt @@ -1,5 +1,5 @@ asgiref==3.7.2 -certifi==2024.2.2 +certifi==2024.7.4 charset-normalizer==3.3.2 # We can drop this after bumping baseline to pypy-39 cramjam==2.1.0; platform_python_implementation == "PyPy"