adding warning to XRay extractor when traceId/spanId missing #4420
Conversation
ghost
requested review from
anuraaga,
jack-berg,
jkwatson,
jsuereth and
Oberon00
as code owners
Codecov Report
@@ Coverage Diff @@
## main #4420 +/- ##
============================================
- Coverage 90.01% 89.99% -0.03%
- Complexity 4846 4887 +41
============================================
Files 557 563 +6
Lines 15047 15164 +117
Branches 1451 1460 +9
============================================
+ Hits 13545 13647 +102
- Misses 1033 1045 +12
- Partials 469 472 +3
Continue to review full report at Codecov.
|
| @@ -228,6 +228,10 @@ private static <C> Context getContextFromHeader( | |||
| return context; | |||
| } | |||
|
|
|||
| if (spanId == null || traceId == null) { | |||
| logger.warning("Both traceId and spanId are required to extract a valid span context. "); | |||
There was a problem hiding this comment.
this seems like it might spam the logs under some circumstances. Perhaps even the normal circumstances if you're running in the environment where this would happen at all. We should definitely not spam the logs in the hot path of people's code.
There was a problem hiding this comment.
This is to address #4259 issue.
TLDR: currently users are creating issues expecting that traceId is sufficient (as per XRay behaviour) to createe otel context, which isn't the case. To avoid further confusion, the idea was to add this log warning. Do you have sth better in mind? Perhaps not warning but info level?
There was a problem hiding this comment.
This log message will happen for every request from ELB unfortunately, and with no recourse since ELB actually needs to be updated. ThrottlingLogger may actually be good in addition to a lower log level, but do think we need to not log by default for something with no actionable item.
There was a problem hiding this comment.
The existing implementation is in the SDK, so not available to propagator implementations. We'd need to put that into some part of the API, which is possible, but I'm not sure where it would live, exactly.
There was a problem hiding this comment.
Switched to FINEST as suggested by @anuraaga
Whole change is really only for end user's convenience, so I believe FINEST is a good solution here.
There was a problem hiding this comment.
Let's use FINEST log for this - we generally don't enable logs by default when the app has no practical fix, eg since it doesn't have control over what header is sent. In this case since AWS is sending the header there is literally nothing a user can do so even FINE is too high IMO
As agreed, fixes #4259