From 560fd04962f11c2cfa68407c88c51442bc864fa6 Mon Sep 17 00:00:00 2001 From: Jordan Gibbings Date: Mon, 19 Aug 2024 22:59:24 +0800 Subject: [PATCH] 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 | 58 ++++++++++++------- .../tests/test_instrumentation.py | 47 ++++++++++++++- 3 files changed, 84 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 39236a33f..8c31e0123 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 090f87a88..fa0e53bf9 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 import trace 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: + 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}" - else: - status_code = response.code - - if status_code is not None: - span.set_attribute(SpanAttributes.HTTP_STATUS_CODE, status_code) - span.set_status( - Status( + 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, ) - 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 status_code is not None: + span.set_attribute(SpanAttributes.HTTP_STATUS_CODE, status_code) + span.set_status(status) - 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 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 + ) 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 01cdddcee..daf2ddd84 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.instrumentation.tornado import ( 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 @@ class TestTornadoInstrumentation(TornadoTest, WsgiTestBase): 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 @@ class TornadoHookTest(TornadoTest): 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("/")