-
Notifications
You must be signed in to change notification settings - Fork 832
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
Fix NaN
and Infinity
values in logs being coerced into null
and causing exception in OTEL collector
#4435
base: main
Are you sure you want to change the base?
Conversation
NaN
and Infinity
values in logs being coerced into null
and causing exception in OTEL collector
ef0279d
to
cbaed2d
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4435 +/- ##
==========================================
- Coverage 92.23% 92.23% -0.01%
==========================================
Files 336 336
Lines 9533 9525 -8
Branches 2022 2020 -2
==========================================
- Hits 8793 8785 -8
Misses 740 740
|
cbaed2d
to
6a9d5f4
Compare
f17141b
to
29bc0bf
Compare
Fix seems straightforward enough, but it seems to me that we are dropping important information here. Is NaN and Infinity really not representable? Is this only affecting JSON or does it also affect protobuf? If it is JSON only, it should be handled somewhere that it doesn't affect the protobuf version, similar to how trace and span ids are handled. |
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 code is also used for protobuf where it's possible to represent both NaN and +/-Infinity.
As @dyladan said, we're likely loosing some data here and we need to ensure we don't affect the protobuf exporters with this change.
This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days. |
This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days. |
This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days. |
Which problem is this PR solving?
OpenTelemetry JS logging SDK had a bug where if the log had a
NaN
,Infinity
or-Infinity
value, it was treated as a number and returnedto Collector, which caused an exception.
Short description of the changes
This PR uses
Number.isFinite
method to check if the value is in fact a number, and won't be coerced intonull
byJSON.stringify
.Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist: