-
Notifications
You must be signed in to change notification settings - Fork 612
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.
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
set error.type
to asgi-instrumentation attributes when having exceptions
#2719
base: main
Are you sure you want to change the base?
Conversation
error.type
to asgi-instrumentation attributeserror.type
to asgi-instrumentation attributes when having exceptions
finally: | ||
if exception is not None and _report_new( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
error.type
to asgi-instrumentation attributes when having exceptionserror.type
to asgi-instrumentation attributes when having exceptions
_set_status( | ||
span=span, | ||
metrics_attributes=attributes, | ||
status_code=-1, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Description
Right now we only have the
error.type
for HTTP status code, but we should handle the exceptions and set it inerror.type
as wellFixes #2699
Type of change
How Has This Been Tested?
Does This PR Require a Core Repo Change?
Checklist: