From 560fd04962f11c2cfa68407c88c51442bc864fa6 Mon Sep 17 00:00:00 2001 From: Jordan Gibbings Date: Mon, 19 Aug 2024 22:59:24 +0800 Subject: [PATCH 1/2] fix(tornado): ensure reading future.result() won't throw an exception in client.py _finish_tracing_callback (#2563) --- CHANGELOG.md | 2 + .../instrumentation/tornado/client.py | 56 ++++++++++++------- .../tests/test_instrumentation.py | 47 +++++++++++++++- 3 files changed, 83 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 39236a33f3..8c31e01235 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,6 +36,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ([#2385](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2385)) - `opentelemetry-instrumentation-asyncio` Fixes async generator coroutines not being awaited ([#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)) ## Version 1.26.0/0.47b0 (2024-07-23) diff --git a/instrumentation/opentelemetry-instrumentation-tornado/src/opentelemetry/instrumentation/tornado/client.py b/instrumentation/opentelemetry-instrumentation-tornado/src/opentelemetry/instrumentation/tornado/client.py index 090f87a88b..fa0e53bf95 100644 --- a/instrumentation/opentelemetry-instrumentation-tornado/src/opentelemetry/instrumentation/tornado/client.py +++ b/instrumentation/opentelemetry-instrumentation-tornado/src/opentelemetry/instrumentation/tornado/client.py @@ -21,7 +21,7 @@ from opentelemetry.instrumentation.utils import http_status_to_status_code from opentelemetry.propagate import inject from opentelemetry.semconv.trace import SpanAttributes -from opentelemetry.trace.status import Status +from opentelemetry.trace.status import Status, StatusCode from opentelemetry.util.http import remove_url_credentials @@ -105,37 +105,53 @@ def _finish_tracing_callback( request_size_histogram, response_size_histogram, ): + response = None status_code = None + status = None description = None - exc = future.exception() - - response = future.result() - if span.is_recording() and exc: + exc = future.exception() + if exc: + description = f"{type(exc).__qualname__}: {exc}" if isinstance(exc, HTTPError): + response = exc.response status_code = exc.code - description = f"{type(exc).__name__}: {exc}" + status = Status( + status_code=http_status_to_status_code(status_code), + description=description, + ) + else: + status = Status( + status_code=StatusCode.ERROR, + description=description, + ) + span.record_exception(exc) else: + response = future.result() status_code = response.code + status = Status( + status_code=http_status_to_status_code(status_code), + description=description, + ) if status_code is not None: span.set_attribute(SpanAttributes.HTTP_STATUS_CODE, status_code) - span.set_status( - Status( - status_code=http_status_to_status_code(status_code), - description=description, - ) - ) + span.set_status(status) - metric_attributes = _create_metric_attributes(response) - request_size = int(response.request.headers.get("Content-Length", 0)) - response_size = int(response.headers.get("Content-Length", 0)) + if response is not None: + metric_attributes = _create_metric_attributes(response) + request_size = int(response.request.headers.get("Content-Length", 0)) + response_size = int(response.headers.get("Content-Length", 0)) - duration_histogram.record( - response.request_time, attributes=metric_attributes - ) - request_size_histogram.record(request_size, attributes=metric_attributes) - response_size_histogram.record(response_size, attributes=metric_attributes) + duration_histogram.record( + response.request_time, attributes=metric_attributes + ) + request_size_histogram.record( + request_size, attributes=metric_attributes + ) + response_size_histogram.record( + response_size, attributes=metric_attributes + ) if response_hook: response_hook(span, future) diff --git a/instrumentation/opentelemetry-instrumentation-tornado/tests/test_instrumentation.py b/instrumentation/opentelemetry-instrumentation-tornado/tests/test_instrumentation.py index 01cdddceed..daf2ddd846 100644 --- a/instrumentation/opentelemetry-instrumentation-tornado/tests/test_instrumentation.py +++ b/instrumentation/opentelemetry-instrumentation-tornado/tests/test_instrumentation.py @@ -16,6 +16,7 @@ from unittest.mock import Mock, patch from http_server_mock import HttpServerMock +from tornado.httpclient import HTTPClientError from tornado.testing import AsyncHTTPTestCase from opentelemetry import trace @@ -32,7 +33,7 @@ from opentelemetry.semconv.trace import SpanAttributes from opentelemetry.test.test_base import TestBase from opentelemetry.test.wsgitestutil import WsgiTestBase -from opentelemetry.trace import SpanKind +from opentelemetry.trace import SpanKind, StatusCode from opentelemetry.util.http import ( OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_REQUEST, OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_RESPONSE, @@ -493,7 +494,6 @@ def test_response_headers(self): self.assertEqual(len(spans), 3) self.assertTraceResponseHeaderMatchesSpan(response.headers, spans[1]) - self.memory_exporter.clear() set_global_response_propagator(orig) def test_credential_removal(self): @@ -602,6 +602,49 @@ def client_response_hook(span, response): self.memory_exporter.clear() +class TestTornadoHTTPClientInstrumentation(TornadoTest, WsgiTestBase): + def test_http_client_success_response(self): + response = self.fetch("/") + self.assertEqual(response.code, 201) + + spans = self.memory_exporter.get_finished_spans() + self.assertEqual(len(spans), 3) + manual, server, client = self.sorted_spans(spans) + self.assertEqual(manual.name, "manual") + self.assertEqual(server.name, "GET /") + self.assertEqual(client.name, "GET") + self.assertEqual(client.status.status_code, StatusCode.UNSET) + self.memory_exporter.clear() + + def test_http_client_failed_response(self): + # when an exception isn't thrown + response = self.fetch("/some-404") + self.assertEqual(response.code, 404) + + spans = self.memory_exporter.get_finished_spans() + self.assertEqual(len(spans), 2) + server, client = self.sorted_spans(spans) + self.assertEqual(server.name, "GET /some-404") + self.assertEqual(client.name, "GET") + self.assertEqual(client.status.status_code, StatusCode.ERROR) + self.memory_exporter.clear() + + # when an exception is thrown + try: + response = self.fetch("/some-404", raise_error=True) + self.assertEqual(response.code, 404) + except HTTPClientError: + pass # expected exception - continue + + spans = self.memory_exporter.get_finished_spans() + self.assertEqual(len(spans), 2) + server, client = self.sorted_spans(spans) + self.assertEqual(server.name, "GET /some-404") + self.assertEqual(client.name, "GET") + self.assertEqual(client.status.status_code, StatusCode.ERROR) + self.memory_exporter.clear() + + class TestTornadoUninstrument(TornadoTest): def test_uninstrument(self): response = self.fetch("/") From dda369b7247919b8d4351b6a2535c7ad9e7f0fc0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Em=C3=ADdio=20Neto?= <9735060+emdneto@users.noreply.github.com> Date: Mon, 19 Aug 2024 12:39:17 -0300 Subject: [PATCH 2/2] bump webob to 1.8.8 in test requirements (#2804) --- .../opentelemetry-instrumentation-pyramid/test-requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/instrumentation/opentelemetry-instrumentation-pyramid/test-requirements.txt b/instrumentation/opentelemetry-instrumentation-pyramid/test-requirements.txt index 56f89f8e8e..aa387f8177 100644 --- a/instrumentation/opentelemetry-instrumentation-pyramid/test-requirements.txt +++ b/instrumentation/opentelemetry-instrumentation-pyramid/test-requirements.txt @@ -15,7 +15,7 @@ tomli==2.0.1 translationstring==1.4 typing_extensions==4.9.0 venusian==3.1.0 -WebOb==1.8.7 +WebOb==1.8.8 Werkzeug==3.0.3 wrapt==1.16.0 zipp==3.19.2