-
Notifications
You must be signed in to change notification settings - Fork 829
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
Do simple logging for failed export. #1480
Do simple logging for failed export. #1480
Conversation
Will that solve #1480 ? |
@iNikem You referenced this same PR ;) |
Bah! I mean #1418 |
I think this should fix #1418 yes, mostly because we are already logging the other important failure case (failure to properly shutdown). The rest of the errors do not seem unrecoverable (they are all parameters-handling related), so I think this simple PR is enough. |
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.
Instead of logging in each exporter how about checking the result code in the processor? We already log for throwable there, this seems to be very similar logging.
opentelemetry-java/sdk/src/main/java/io/opentelemetry/sdk/trace/export/BatchSpanProcessor.java
Line 288 in 643d25d
logger.log(Level.WARNING, "Exception thrown by the export.", t); |
@@ -126,6 +126,7 @@ public ResultCode export(Collection<SpanData> spans) { | |||
stub.postSpans(request); | |||
return ResultCode.SUCCESS; | |||
} catch (Throwable e) { | |||
logger.log(Level.SEVERE, "Failed to export spans", e); |
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.
We don't seem to use SEVERE
anywhere else yet - spans can fail to export for transient reasons (backend is restarting), and observability is not critical to the app anyways, so I'd go with WARNING if not INFO.
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.
Please at least WARN :)
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.
WARN works for me. Will update.
@anuraaga Taking into account open-telemetry/opentelemetry-specification#716, I would say logging in each exporter makes more sense :) |
@iNikem Well I'm definitely ok with removing resultcode instead :) But it seems like one or the other makes sense, either exporter is left to their own ways or are controlled by the span processor. |
On top of the possibility of having the export result code being removed, there's the case of using a |
The exception could be propagated up too - it seems weird to return a failure status without the reason. Shouldn't we either leave everything to the exporter, or otherwise have it propagate everything? I personally think either is fine, but a mix seems strange, it's hard to know who has responsibility when it comes to exporting spans. |
Going forward, we should 'normalize' things with Exception/Exporter Code being returned or being totally gone. For the time being, I suggest we keep these warnings and tune this part once we decide how to proceed (most likely after we come to an agreement on whether we want #716 or not). |
Codecov Report
@@ Coverage Diff @@
## master #1480 +/- ##
============================================
+ Coverage 92.50% 92.56% +0.05%
Complexity 939 939
============================================
Files 117 117
Lines 3348 3348
Branches 270 270
============================================
+ Hits 3097 3099 +2
+ Misses 169 168 -1
+ Partials 82 81 -1
Continue to review full report at Codecov.
|
We already log other situations (such as failing to do a clean shutdown), so it makes sense to add logging for failed export.