Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

set error.type to asgi-instrumentation attributes when having exceptions #2719

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
+172 −9
Draft

set error.type to asgi-instrumentation attributes when having exceptions #2719

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ffbd606
add error.type for asgi attributes
emdneto Jul 17, 2024
056f956
make set_status call more clear
emdneto Jul 17, 2024
dd5d613
add changelog
emdneto Jul 17, 2024
0c57674
Merge branch 'main' into asgi-error-type
emdneto Jul 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
([#2673](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2673))
- `opentelemetry-instrumentation-django` Add `http.target` to Django duration metric attributes
([#2624](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2624))
- `opentelemetry-instrumentation-asgi` Add `error.type` attribute in case of exceptions
([#2719](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2719))

### Breaking changes

Expand Down Expand Up @@ -73,7 +75,7 @@ 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.
- `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))


Expand Down
28 changes: 20 additions & 8 deletions ...ion/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -549,23 +549,18 @@ def __init__(
sem_conv_opt_in_mode = _OpenTelemetrySemanticConventionStability._get_opentelemetry_stability_opt_in_mode(
_OpenTelemetryStabilitySignalType.HTTP,
)
schema_url = _get_schema_url(sem_conv_opt_in_mode)
self.app = guarantee_single_callable(app)
self.tracer = (
trace.get_tracer(
__name__,
__version__,
tracer_provider,
schema_url=_get_schema_url(sem_conv_opt_in_mode),
__name__, __version__, tracer_provider, schema_url=schema_url
)
if tracer is None
else tracer
)
self.meter = (
get_meter(
__name__,
__version__,
meter_provider,
schema_url=_get_schema_url(sem_conv_opt_in_mode),
__name__, __version__, meter_provider, schema_url=schema_url
)
if meter is None
else meter
Expand Down Expand Up @@ -693,6 +688,7 @@ async def __call__(

if scope["type"] == "http":
self.active_requests_counter.add(1, active_requests_count_attrs)
exception = None
try:
with trace.use_span(span, end_on_exit=False) as current_span:
if current_span.is_recording():
Expand Down Expand Up @@ -729,7 +725,20 @@ async def __call__(
)

await self.app(scope, otel_receive, otel_send)
except Exception as exc: # pylint: disable=broad-except
exception = exc
finally:
if exception is not None and _report_new(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a style thing, but I'm wondering if it would be worthwhile to break this method into functions for readability. Perhaps those functions could stand to be further modularized, but this would be a step in that direction. Something like

        try:
            await self.instrument_app(attributes, receive, scope, send, span, span_name)
        except Exception as exc:  # pylint: disable=broad-except
            exception = exc
        finally:
            await self.cleanup(active_requests_count_attrs, attributes, exception, scope, span, start, token)

Not a blocking comment, just a suggestion while we're in here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another suggestion would also to wait and implement this for the various instrumentations that still require this (like flask, wsgi (for metrics), django, etc) and see if there are any commonalities we can refactor out. Difficult to say what we can abstract/modularize without seeing more implementation.

Copy link
Member Author

@emdneto emdneto Jul 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Indeed, while reviewing the transition PRs I identified several instrumentations we are not handling exceptions and setting the exception in error.type, even for the ones that are already migrated to the new semconv. I will mark this as draft for now while I think in a better way to handle this. Maybe we'll have a better idea on the commonalities after more transitions.

self._sem_conv_opt_in_mode
):
_set_status(
span=span,
metrics_attributes=attributes,
status_code=-1,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The -1 status code is a bit awkward. The current change is fine but I would like to brainstorm some ways we can abstract all of this logic out somehow. The following are probably required in some way or another and should be shared across all instrumentations (especially wsgi and asgi) : 1. Setting status code on span 2. Setting error attributes on span if recording and if status code is erroneous 3. Setting error attributes on metrics attributes if status code is erroneous 4. Setting error attributes on span/metrics in general.

Copy link
Member Author

@emdneto emdneto Jul 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't say I like it either. We are actually doing that in several places where we try to int the status_code, and if it fails, we set status_code as -1. Let me see what can I do here to improve the way we are handling this.

status_code_str=type(exception).__qualname__,
server_span=True,
sem_conv_opt_in_mode=self._sem_conv_opt_in_mode,
)
if scope["type"] == "http":
target = _collect_target_attribute(scope)
if target:
Expand Down Expand Up @@ -791,6 +800,9 @@ async def __call__(
if span.is_recording():
span.end()

if exception is not None:
raise exception.with_traceback(exception.__traceback__)

# pylint: enable=too-many-branches

def _get_otel_receive(self, server_span_name, scope, receive):
Expand Down
149 changes: 149 additions & 0 deletions instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
from timeit import default_timer
from unittest import mock

from asgiref.testing import ApplicationCommunicator

import opentelemetry.instrumentation.asgi as otel_asgi
from opentelemetry import trace as trace_api
from opentelemetry.instrumentation._semconv import (
Expand All @@ -46,6 +48,7 @@
CLIENT_ADDRESS,
CLIENT_PORT,
)
from opentelemetry.semconv.attributes.error_attributes import ERROR_TYPE
from opentelemetry.semconv.attributes.http_attributes import (
HTTP_REQUEST_METHOD,
HTTP_RESPONSE_STATUS_CODE,
Expand Down Expand Up @@ -278,6 +281,15 @@ async def error_asgi(scope, receive, send):
await send({"type": "http.response.body", "body": b"*"})


async def exception_app(scope, receive, send):
raise ValueError("An unexpected error occurred")


async def send_simple_input_and_receive(communicator: ApplicationCommunicator):
await communicator.send_input({"type": "http.request", "body": b""})
await communicator.receive_output(0)


# pylint: disable=too-many-public-methods
class TestAsgiApplication(AsgiTestBase):
def setUp(self):
Expand Down Expand Up @@ -509,6 +521,143 @@ def test_basic_asgi_call_both_semconv(self):
outputs = self.get_all_output()
self.validate_outputs(outputs, old_sem_conv=True, new_sem_conv=True)

def test_basic_asgi_exception(self):
"""Test that an exception is properly handled."""

app = otel_asgi.OpenTelemetryMiddleware(exception_app)
communicator = ApplicationCommunicator(app, self.scope)

with self.assertRaises(ValueError) as ctx:
asyncio.get_event_loop().run_until_complete(
send_simple_input_and_receive(communicator)
)

self.assertEqual(str(ctx.exception), "An unexpected error occurred")

span_list = self.memory_exporter.get_finished_spans()
self.assertEqual(len(span_list), 1)
span = span_list[0]
self.assertEqual(span.kind, trace_api.SpanKind.SERVER)
self.assertEqual(span.status.status_code, trace_api.StatusCode.ERROR)
self.assertEqual(
span.status.description, "ValueError: An unexpected error occurred"
)
self.assertEqual(len(span.events), 1)
self.assertEqual(span.events[0].name, "exception")
self.assertIn(
"ValueError: An unexpected error occurred",
span.events[0].attributes["exception.stacktrace"],
)
self.assertDictEqual(
dict(span.attributes),
{
SpanAttributes.HTTP_METHOD: "GET",
SpanAttributes.HTTP_SCHEME: "http",
SpanAttributes.NET_HOST_PORT: 80,
SpanAttributes.HTTP_HOST: "127.0.0.1",
SpanAttributes.HTTP_FLAVOR: "1.0",
SpanAttributes.HTTP_TARGET: "/",
SpanAttributes.HTTP_URL: "http://127.0.0.1/",
SpanAttributes.NET_PEER_IP: "127.0.0.1",
SpanAttributes.NET_PEER_PORT: 32767,
},
)

def test_basic_asgi_exception_new_semconv(self):
"""Test that an exception is properly handled."""

app = otel_asgi.OpenTelemetryMiddleware(exception_app)
communicator = ApplicationCommunicator(app, self.scope)

with self.assertRaises(ValueError) as ctx:
asyncio.get_event_loop().run_until_complete(
send_simple_input_and_receive(communicator)
)

self.assertEqual(str(ctx.exception), "An unexpected error occurred")

span_list = self.memory_exporter.get_finished_spans()
span = span_list[0]
self.assertEqual(len(span_list), 1)

self.assertEqual(span.kind, trace_api.SpanKind.SERVER)
self.assertEqual(span.status.status_code, trace_api.StatusCode.ERROR)
self.assertEqual(
span.status.description, "Non-integer HTTP status: ValueError"
)
self.assertEqual(len(span.events), 1)
self.assertEqual(span.events[0].name, "exception")
self.assertIn(
"ValueError: An unexpected error occurred",
span.events[0].attributes["exception.stacktrace"],
)
self.assertDictEqual(
dict(span.attributes),
{
HTTP_REQUEST_METHOD: "GET",
URL_SCHEME: "http",
SERVER_PORT: 80,
SERVER_ADDRESS: "127.0.0.1",
NETWORK_PROTOCOL_VERSION: "1.0",
URL_PATH: "/",
URL_FULL: "http://127.0.0.1/",
CLIENT_ADDRESS: "127.0.0.1",
CLIENT_PORT: 32767,
ERROR_TYPE: "ValueError",
},
)

def test_basic_asgi_exception_both_semconv(self):
"""Test that an exception is properly handled."""

app = otel_asgi.OpenTelemetryMiddleware(exception_app)
communicator = ApplicationCommunicator(app, self.scope)

with self.assertRaises(ValueError) as ctx:
asyncio.get_event_loop().run_until_complete(
send_simple_input_and_receive(communicator)
)
self.assertEqual(str(ctx.exception), "An unexpected error occurred")

span_list = self.memory_exporter.get_finished_spans()
self.assertEqual(len(span_list), 1)
span = span_list[0]
self.assertEqual(span.kind, trace_api.SpanKind.SERVER)
self.assertEqual(span.status.status_code, trace_api.StatusCode.ERROR)
self.assertEqual(
span.status.description, "Non-integer HTTP status: ValueError"
)
self.assertEqual(len(span.events), 1)
self.assertEqual(span.events[0].name, "exception")
self.assertIn(
"ValueError: An unexpected error occurred",
span.events[0].attributes["exception.stacktrace"],
)
self.assertDictEqual(
dict(span.attributes),
{
SpanAttributes.HTTP_METHOD: "GET",
SpanAttributes.HTTP_SCHEME: "http",
SpanAttributes.NET_HOST_PORT: 80,
SpanAttributes.HTTP_HOST: "127.0.0.1",
SpanAttributes.HTTP_FLAVOR: "1.0",
SpanAttributes.HTTP_TARGET: "/",
SpanAttributes.HTTP_URL: "http://127.0.0.1/",
SpanAttributes.NET_PEER_IP: "127.0.0.1",
SpanAttributes.NET_PEER_PORT: 32767,
HTTP_REQUEST_METHOD: "GET",
URL_SCHEME: "http",
SERVER_PORT: 80,
SERVER_ADDRESS: "127.0.0.1",
NETWORK_PROTOCOL_VERSION: "1.0",
URL_PATH: "/",
URL_FULL: "http://127.0.0.1/",
CLIENT_ADDRESS: "127.0.0.1",
CLIENT_PORT: 32767,
ERROR_TYPE: "ValueError",
},
)

def test_asgi_not_recording(self):
mock_tracer = mock.Mock()
mock_span = mock.Mock()
Expand Down
Loading