-
Notifications
You must be signed in to change notification settings - Fork 599
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
[opentelemetry-instrumentation-django] Handle exceptions from request/response hooks #2153
Conversation
I think we need to first answer the question: do we want to capture and handle exceptions and exception information for errors that occur in user supplied hooks? Is span information pertaining to misconfigured hooks something of substance to capture in a span and as telemetry? @aabmass @ocelotl @srikanthccv Thoughts? |
Looks like there’s some guidance on this in the OpenTelemetry spec: https://github.com/open-telemetry/opentelemetry-specification/blob/6fd4f0809bcff0ea5c4abe161803b4ad8628375e/specification/error-handling.md?plain=1#L24 At the very least I would expect spans created by an instrumentor to always be closed by that instrumentor regardless of whether an exception occurs. |
I agree with the "handling exception" portion, I am just not sure about the "capturing telemetry information" part about the exception itself in the span. |
I think we should log the exception and move on. This is our approach to all unexpected things that happen during instrumentation/export/recording metrics et al. |
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.
If you could remove the portions about recording unexpected behavior in the span used for the original request we can move forward with the pr. We can just handle the exception with a log.
@ebracho Please also update the branch with latest main to add the changelog entry to Unreleased and not 1.23.0. |
...nstrumentation-django/src/opentelemetry/instrumentation/django/middleware/otel_middleware.py
Outdated
Show resolved
Hide resolved
...nstrumentation-django/src/opentelemetry/instrumentation/django/middleware/otel_middleware.py
Outdated
Show resolved
Hide resolved
...nstrumentation-django/src/opentelemetry/instrumentation/django/middleware/otel_middleware.py
Outdated
Show resolved
Hide resolved
For some reason I'm having trouble running tox locally after one of the merges 🤔
|
@ebracho tox -e lint works fine here with tox 4.14.1. What version do you have? are you on macos? |
Ensure that we are cleaning up spans and contexts in the case that user-provided hooks raise an exception.
@ebracho I fixed lint issues and solved conflicts with main, one test case is failing, please take a look. |
we're no longer recording a span event for the exception
@ocelotl thank you, I fixed the unit tests and an outdated comment. Also I figured out my tox issues - I'm not exactly sure what what going on but installing and running tox under a fresh virtualenv did the trick. |
thanks everyone 🙏 |
Ensure that we are cleaning up spans and contexts in the case that user-provided hooks raise an exception.
Description
This change ensures that we are properly cleaning up spans and context tokens in the case that user-provided request/response hooks raise an exception. The motivation is a recent issue we had in one of our services where our request_hook unexpectedly raised an exception due to a type error and caused spans to leak.
slack thread with more context
Fixes # (issue): N/A
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Added new test cases to ensure that request/response hook exceptions don't cause spans to leak and are handled appropriately.
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.