Do simple logging for failed export. #1480
Conversation
carlosalberto
requested review from
arminru,
bogdandrutu,
jkwatson,
pavolloffay,
thisthat and
tylerbenson
as code owners
|
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. |
| @@ -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.
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.
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.