From bc4d2c5b75e3e523c4be2da2fa01f2c02e4f9453 Mon Sep 17 00:00:00 2001 From: Leighton Chen Date: Fri, 23 Aug 2024 16:56:36 -0700 Subject: [PATCH] Fix `http.host` and `net.peer.ip` new http semconv mapping (#2814) --- CHANGELOG.md | 2 + .../instrumentation/asgi/__init__.py | 10 ++-- .../tests/test_asgi_middleware.py | 19 ++----- .../instrumentation/httpx/__init__.py | 4 +- .../instrumentation/requests/__init__.py | 6 +- .../instrumentation/urllib3/__init__.py | 6 +- .../instrumentation/wsgi/__init__.py | 4 +- .../opentelemetry/instrumentation/_semconv.py | 56 ++++++++++++------- 8 files changed, 59 insertions(+), 48 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5abca8227a..c5ce56423a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,6 +40,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ([#2792](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2792)) - `opentelemetry-instrumentation-tornado` Handle http client exception and record exception info into span ([#2563](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2563)) +- `opentelemetry-instrumentation` fix `http.host` new http semantic convention mapping to depend on `kind` of span + ([#2814](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2814)) ## Version 1.26.0/0.47b0 (2024-07-23) 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 8e3199cef8..295eb2a043 100644 --- a/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py @@ -215,10 +215,10 @@ def client_response_hook(span: Span, scope: dict[str, Any], message: dict[str, A _server_duration_attrs_new, _server_duration_attrs_old, _set_http_flavor_version, - _set_http_host, + _set_http_host_server, _set_http_method, _set_http_net_host_port, - _set_http_peer_ip, + _set_http_peer_ip_server, _set_http_peer_port_server, _set_http_scheme, _set_http_target, @@ -342,7 +342,7 @@ def collect_request_attributes( if scheme: _set_http_scheme(result, scheme, sem_conv_opt_in_mode) if server_host: - _set_http_host(result, server_host, sem_conv_opt_in_mode) + _set_http_host_server(result, server_host, sem_conv_opt_in_mode) if port: _set_http_net_host_port(result, port, sem_conv_opt_in_mode) flavor = scope.get("http_version") @@ -380,7 +380,9 @@ def collect_request_attributes( _set_http_user_agent(result, http_user_agent[0], sem_conv_opt_in_mode) if "client" in scope and scope["client"] is not None: - _set_http_peer_ip(result, scope.get("client")[0], sem_conv_opt_in_mode) + _set_http_peer_ip_server( + result, scope.get("client")[0], sem_conv_opt_in_mode + ) _set_http_peer_port_server( result, scope.get("client")[1], sem_conv_opt_in_mode ) diff --git a/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py b/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py index af51faa808..d89f4a54e0 100644 --- a/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py +++ b/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py @@ -53,10 +53,7 @@ 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.server_attributes import SERVER_PORT from opentelemetry.semconv.attributes.url_attributes import ( URL_PATH, URL_QUERY, @@ -406,7 +403,6 @@ def validate_outputs( HTTP_REQUEST_METHOD: "GET", URL_SCHEME: "http", SERVER_PORT: 80, - SERVER_ADDRESS: "127.0.0.1", NETWORK_PROTOCOL_VERSION: "1.0", URL_PATH: "/", CLIENT_ADDRESS: "127.0.0.1", @@ -442,7 +438,6 @@ def validate_outputs( HTTP_REQUEST_METHOD: "GET", URL_SCHEME: "http", SERVER_PORT: 80, - SERVER_ADDRESS: "127.0.0.1", NETWORK_PROTOCOL_VERSION: "1.0", URL_PATH: "/", CLIENT_ADDRESS: "127.0.0.1", @@ -688,7 +683,7 @@ def test_behavior_with_scope_server_as_none_new_semconv(self): def update_expected_server(expected): expected[3]["attributes"].update( { - SERVER_ADDRESS: "0.0.0.0", + CLIENT_ADDRESS: "0.0.0.0", SERVER_PORT: 80, } ) @@ -715,7 +710,7 @@ def update_expected_server(expected): SpanAttributes.HTTP_HOST: "0.0.0.0", SpanAttributes.NET_HOST_PORT: 80, SpanAttributes.HTTP_URL: "http://0.0.0.0/", - SERVER_ADDRESS: "0.0.0.0", + CLIENT_ADDRESS: "0.0.0.0", SERVER_PORT: 80, } ) @@ -1001,7 +996,6 @@ def test_websocket_new_semconv(self): "attributes": { URL_SCHEME: self.scope["scheme"], SERVER_PORT: self.scope["server"][1], - SERVER_ADDRESS: self.scope["server"][0], NETWORK_PROTOCOL_VERSION: self.scope["http_version"], URL_PATH: self.scope["path"], CLIENT_ADDRESS: self.scope["client"][0], @@ -1086,7 +1080,6 @@ def test_websocket_both_semconv(self): SpanAttributes.HTTP_METHOD: self.scope["method"], URL_SCHEME: self.scope["scheme"], SERVER_PORT: self.scope["server"][1], - SERVER_ADDRESS: self.scope["server"][0], NETWORK_PROTOCOL_VERSION: self.scope["http_version"], URL_PATH: self.scope["path"], CLIENT_ADDRESS: self.scope["client"][0], @@ -1629,7 +1622,6 @@ def test_request_attributes_new_semconv(self): attrs, { HTTP_REQUEST_METHOD: "GET", - SERVER_ADDRESS: "127.0.0.1", URL_PATH: "/", URL_QUERY: "foo=bar", SERVER_PORT: 80, @@ -1665,7 +1657,6 @@ def test_request_attributes_both_semconv(self): SpanAttributes.NET_PEER_IP: "127.0.0.1", SpanAttributes.NET_PEER_PORT: 32767, HTTP_REQUEST_METHOD: "GET", - SERVER_ADDRESS: "127.0.0.1", URL_PATH: "/", URL_QUERY: "foo=bar", SERVER_PORT: 80, @@ -1690,7 +1681,7 @@ def test_query_string_new_semconv(self): _HTTPStabilityMode.HTTP, ) self.assertEqual(attrs[URL_SCHEME], "http") - self.assertEqual(attrs[SERVER_ADDRESS], "127.0.0.1") + self.assertEqual(attrs[CLIENT_ADDRESS], "127.0.0.1") self.assertEqual(attrs[URL_PATH], "/") self.assertEqual(attrs[URL_QUERY], "foo=bar") @@ -1704,7 +1695,7 @@ def test_query_string_both_semconv(self): attrs[SpanAttributes.HTTP_URL], "http://127.0.0.1/?foo=bar" ) self.assertEqual(attrs[URL_SCHEME], "http") - self.assertEqual(attrs[SERVER_ADDRESS], "127.0.0.1") + self.assertEqual(attrs[CLIENT_ADDRESS], "127.0.0.1") self.assertEqual(attrs[URL_PATH], "/") self.assertEqual(attrs[URL_QUERY], "foo=bar") diff --git a/instrumentation/opentelemetry-instrumentation-httpx/src/opentelemetry/instrumentation/httpx/__init__.py b/instrumentation/opentelemetry-instrumentation-httpx/src/opentelemetry/instrumentation/httpx/__init__.py index f2a18a2770..43db6c3a82 100644 --- a/instrumentation/opentelemetry-instrumentation-httpx/src/opentelemetry/instrumentation/httpx/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-httpx/src/opentelemetry/instrumentation/httpx/__init__.py @@ -202,7 +202,7 @@ async def async_response_hook(span, request, response): _OpenTelemetrySemanticConventionStability, _OpenTelemetryStabilitySignalType, _report_new, - _set_http_host, + _set_http_host_client, _set_http_method, _set_http_network_protocol_version, _set_http_peer_port_client, @@ -342,7 +342,7 @@ def _apply_request_client_attributes_to_span( if _report_new(semconv): if url.host: # http semconv transition: http.host -> server.address - _set_http_host(span_attributes, url.host, semconv) + _set_http_host_client(span_attributes, url.host, semconv) # http semconv transition: net.sock.peer.addr -> network.peer.address span_attributes[NETWORK_PEER_ADDRESS] = url.host if url.port: diff --git a/instrumentation/opentelemetry-instrumentation-requests/src/opentelemetry/instrumentation/requests/__init__.py b/instrumentation/opentelemetry-instrumentation-requests/src/opentelemetry/instrumentation/requests/__init__.py index 3aa1b476f5..db67d378d9 100644 --- a/instrumentation/opentelemetry-instrumentation-requests/src/opentelemetry/instrumentation/requests/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-requests/src/opentelemetry/instrumentation/requests/__init__.py @@ -92,7 +92,7 @@ def response_hook(span, request_obj, response) _OpenTelemetryStabilitySignalType, _report_new, _report_old, - _set_http_host, + _set_http_host_client, _set_http_method, _set_http_net_peer_name_client, _set_http_network_protocol_version, @@ -212,14 +212,14 @@ def get_or_create_headers(): metric_labels, parsed_url.scheme, sem_conv_opt_in_mode ) if parsed_url.hostname: - _set_http_host( + _set_http_host_client( metric_labels, parsed_url.hostname, sem_conv_opt_in_mode ) _set_http_net_peer_name_client( metric_labels, parsed_url.hostname, sem_conv_opt_in_mode ) if _report_new(sem_conv_opt_in_mode): - _set_http_host( + _set_http_host_client( span_attributes, parsed_url.hostname, sem_conv_opt_in_mode, diff --git a/instrumentation/opentelemetry-instrumentation-urllib3/src/opentelemetry/instrumentation/urllib3/__init__.py b/instrumentation/opentelemetry-instrumentation-urllib3/src/opentelemetry/instrumentation/urllib3/__init__.py index 4bcd0816fd..1c83f3f447 100644 --- a/instrumentation/opentelemetry-instrumentation-urllib3/src/opentelemetry/instrumentation/urllib3/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-urllib3/src/opentelemetry/instrumentation/urllib3/__init__.py @@ -103,7 +103,7 @@ def response_hook( _OpenTelemetryStabilitySignalType, _report_new, _report_old, - _set_http_host, + _set_http_host_client, _set_http_method, _set_http_net_peer_name_client, _set_http_network_protocol_version, @@ -491,7 +491,9 @@ def _set_metric_attributes( sem_conv_opt_in_mode: _HTTPStabilityMode = _HTTPStabilityMode.DEFAULT, ) -> None: - _set_http_host(metric_attributes, instance.host, sem_conv_opt_in_mode) + _set_http_host_client( + metric_attributes, instance.host, sem_conv_opt_in_mode + ) _set_http_scheme(metric_attributes, instance.scheme, sem_conv_opt_in_mode) _set_http_method( metric_attributes, diff --git a/instrumentation/opentelemetry-instrumentation-wsgi/src/opentelemetry/instrumentation/wsgi/__init__.py b/instrumentation/opentelemetry-instrumentation-wsgi/src/opentelemetry/instrumentation/wsgi/__init__.py index 355b1d7458..88704f35ab 100644 --- a/instrumentation/opentelemetry-instrumentation-wsgi/src/opentelemetry/instrumentation/wsgi/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-wsgi/src/opentelemetry/instrumentation/wsgi/__init__.py @@ -231,7 +231,7 @@ def response_hook(span: Span, environ: WSGIEnvironment, status: str, response_he _set_http_net_host, _set_http_net_host_port, _set_http_net_peer_name_server, - _set_http_peer_ip, + _set_http_peer_ip_server, _set_http_peer_port_server, _set_http_scheme, _set_http_target, @@ -360,7 +360,7 @@ def collect_request_attributes( remote_addr = environ.get("REMOTE_ADDR") if remote_addr: - _set_http_peer_ip(result, remote_addr, sem_conv_opt_in_mode) + _set_http_peer_ip_server(result, remote_addr, sem_conv_opt_in_mode) peer_port = environ.get("REMOTE_PORT") if peer_port: diff --git a/opentelemetry-instrumentation/src/opentelemetry/instrumentation/_semconv.py b/opentelemetry-instrumentation/src/opentelemetry/instrumentation/_semconv.py index 33668333ce..c4e720fd04 100644 --- a/opentelemetry-instrumentation/src/opentelemetry/instrumentation/_semconv.py +++ b/opentelemetry-instrumentation/src/opentelemetry/instrumentation/_semconv.py @@ -252,16 +252,32 @@ def _set_http_scheme(result, scheme, sem_conv_opt_in_mode): set_string_attribute(result, URL_SCHEME, scheme) -def _set_http_host(result, host, sem_conv_opt_in_mode): +def _set_http_flavor_version(result, version, sem_conv_opt_in_mode): if _report_old(sem_conv_opt_in_mode): - set_string_attribute(result, SpanAttributes.HTTP_HOST, host) + set_string_attribute(result, SpanAttributes.HTTP_FLAVOR, version) if _report_new(sem_conv_opt_in_mode): - set_string_attribute(result, SERVER_ADDRESS, host) + set_string_attribute(result, NETWORK_PROTOCOL_VERSION, version) + + +def _set_http_user_agent(result, user_agent, sem_conv_opt_in_mode): + if _report_old(sem_conv_opt_in_mode): + set_string_attribute( + result, SpanAttributes.HTTP_USER_AGENT, user_agent + ) + if _report_new(sem_conv_opt_in_mode): + set_string_attribute(result, USER_AGENT_ORIGINAL, user_agent) # Client +def _set_http_host_client(result, host, sem_conv_opt_in_mode): + if _report_old(sem_conv_opt_in_mode): + set_string_attribute(result, SpanAttributes.HTTP_HOST, host) + if _report_new(sem_conv_opt_in_mode): + set_string_attribute(result, SERVER_ADDRESS, host) + + def _set_http_net_peer_name_client(result, peer_name, sem_conv_opt_in_mode): if _report_old(sem_conv_opt_in_mode): set_string_attribute(result, SpanAttributes.NET_PEER_NAME, peer_name) @@ -310,27 +326,32 @@ def _set_http_target(result, target, path, query, sem_conv_opt_in_mode): set_string_attribute(result, URL_QUERY, query) -def _set_http_peer_ip(result, ip, sem_conv_opt_in_mode): +def _set_http_host_server(result, host, sem_conv_opt_in_mode): if _report_old(sem_conv_opt_in_mode): - set_string_attribute(result, SpanAttributes.NET_PEER_IP, ip) + set_string_attribute(result, SpanAttributes.HTTP_HOST, host) if _report_new(sem_conv_opt_in_mode): - set_string_attribute(result, CLIENT_ADDRESS, ip) + set_string_attribute(result, CLIENT_ADDRESS, host) -def _set_http_peer_port_server(result, port, sem_conv_opt_in_mode): +# net.peer.ip -> net.sock.peer.addr +# https://github.com/open-telemetry/semantic-conventions/blob/40db676ca0e735aa84f242b5a0fb14e49438b69b/schemas/1.15.0#L18 +# net.sock.peer.addr -> client.socket.address for server spans (TODO) AND client.address if missing +# https://github.com/open-telemetry/semantic-conventions/blob/v1.21.0/CHANGELOG.md#v1210-2023-07-13 +# https://github.com/open-telemetry/semantic-conventions/blob/main/docs/non-normative/http-migration.md#common-attributes-across-http-client-and-server-spans +def _set_http_peer_ip_server(result, ip, sem_conv_opt_in_mode): if _report_old(sem_conv_opt_in_mode): - set_int_attribute(result, SpanAttributes.NET_PEER_PORT, port) + set_string_attribute(result, SpanAttributes.NET_PEER_IP, ip) if _report_new(sem_conv_opt_in_mode): - set_int_attribute(result, CLIENT_PORT, port) + # Only populate if not already populated + if not result.get(CLIENT_ADDRESS): + set_string_attribute(result, CLIENT_ADDRESS, ip) -def _set_http_user_agent(result, user_agent, sem_conv_opt_in_mode): +def _set_http_peer_port_server(result, port, sem_conv_opt_in_mode): if _report_old(sem_conv_opt_in_mode): - set_string_attribute( - result, SpanAttributes.HTTP_USER_AGENT, user_agent - ) + set_int_attribute(result, SpanAttributes.NET_PEER_PORT, port) if _report_new(sem_conv_opt_in_mode): - set_string_attribute(result, USER_AGENT_ORIGINAL, user_agent) + set_int_attribute(result, CLIENT_PORT, port) def _set_http_net_peer_name_server(result, name, sem_conv_opt_in_mode): @@ -340,13 +361,6 @@ def _set_http_net_peer_name_server(result, name, sem_conv_opt_in_mode): set_string_attribute(result, CLIENT_ADDRESS, name) -def _set_http_flavor_version(result, version, sem_conv_opt_in_mode): - if _report_old(sem_conv_opt_in_mode): - set_string_attribute(result, SpanAttributes.HTTP_FLAVOR, version) - if _report_new(sem_conv_opt_in_mode): - set_string_attribute(result, NETWORK_PROTOCOL_VERSION, version) - - def _set_status( span, metrics_attributes: dict,