Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
error handling proposal #153
error handling proposal #153
Changes from 12 commits
1b6737a
ac14996
f90437e
d30f90a
b4eee5a
58179c6
03e937e
55bbad2
dcb740c
6c87edf
3812350
8c01355
669052e
4109c10
a10d8ea
ea993fd
872ebd4
de7cb15
a8451f1
e4ddb0c
eb4bc45
30bb855
6fed7dc
83f4477
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This line is new, from @reyang's comment at open-telemetry/opentelemetry-python#11 (comment).
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 originally changed this to read:
to address comments at 1b6737a#r296402616, but on reflection I think it is actually better not to have any checked exceptions in the API. It's also more consistent with @SergeyKanzhelev's original text.
The big unresolved problem here is NPEs.
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.
"Throw unhandled exceptions" is kind of redundant, but may help distinguish between checked exceptions listed in docs/signatures and unchecked. Since I'm proposing outlawing checked exceptions too this might not be necessary.
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.
This doc is great! Should we add how those exceptions can effect the span status ?
Use case: As a user, I provide a callback in order to add custom attributes to my spans. This callback throws an exception. However, beside this exception, everything is ok. In Zipkin/Jaeger, Should I see:
OK
)UNKNOWN
with error attributesOK
OK
I think that user should know that everything is slowing down his application. Let me know! Thanks in advance.
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 think that such "internal instrumentation error" information on spans that describes errors not in the instrumented code but the instrumentation itself might be very useful but have not been considered at all yet.
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.
@OlivierAlbertini SDK will never know the intention of a callback to add an attribute. I think SDK design must try to avoid those kind of uncertainties. Errors can be exposed as language-specific logs.
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.
Also #275
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.
New line, added for @iredelmeier's comment at 1b6737a#r296935968.
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.
For me this contradicts the statement above:
Do we really want to crash the application on startup if there's an issue with the configuration or if the monitoring backend can't be reached?
I'd rather provide means for the user to proactively check if OTel is correctly initialized. I think this check is something we can expect application developers to add to their startup routine. There they can add appropriate error handling (e.g. reporting the misconfiguration to their logging service - or crashing their app if they really want to).
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.
@arminru I think the motivation is that, if something fails at start time, you can easily/quickly check what's going on, opposed to suddenly get errors once the application has been running for a while.
That being said, we can also probably get more relaxed there, and only fail for fatal cases (again, only at the start).
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 think yes if we've got a malformed config, no if the backend can't be reached -- and hopefully we're not making any network requests at startup.
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 removed
because performance guarantees seem out of scope for this doc.
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.
To me it sounds like it was an example of an Exception, instead of that actual
OutOfMemoryException
(then again, it's probably redundant and we can get rid of that, after your recent changes :) )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 took it to mean that exporters should be careful about catching unchecked exceptions that they might cause, in this case by causing an OOM because the export queue grows unbounded.
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.
New line to expand on the point above.
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.
This is a new line, I thought logging was worth mentioning separately from callbacks.
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.
New line for @jmacd and @arminru's comments at #153 (comment).
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.
Removed
since we say we're allowed to fail fast on configuration errors above.