Skip to content
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(exporter-collector): wrong data type for numbers #1938

Merged

Conversation

kudlatyamroth
Copy link
Contributor

Which problem is this PR solving?

It corrects data type send to collector
as of now it sends:

attributes:
 http.status_code: DOUBLE(200)

and should be:

attributes:
 http.status_code: INT(200)

Short description of the changes

Add way to select intValue in toCollectorAnyValue only for numbers that qualify to range of INT and Number.isInteger()

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Feb 17, 2021

CLA Signed

The committers are authorized under a signed CLA.

@kudlatyamroth kudlatyamroth changed the title Bugfix/wrong data type for numbers fix: wrong data type for numbers Feb 17, 2021
@kudlatyamroth kudlatyamroth changed the title fix: wrong data type for numbers fix(exporter-collector): wrong data type for numbers Feb 17, 2021
@codecov
Copy link

codecov bot commented Feb 17, 2021

Codecov Report

Merging #1938 (8f359dc) into main (167bfd8) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1938   +/-   ##
=======================================
  Coverage   92.89%   92.90%           
=======================================
  Files         152      152           
  Lines        5914     5922    +8     
  Branches     1240     1245    +5     
=======================================
+ Hits         5494     5502    +8     
  Misses        420      420           
Impacted Files Coverage Δ
.../opentelemetry-exporter-collector/src/transform.ts 88.69% <100.00%> (+0.84%) ⬆️

@kudlatyamroth kudlatyamroth force-pushed the bugfix/wrong-data-type-for-numbers branch from ab13881 to 2817149 Compare February 17, 2021 17:12
@kudlatyamroth kudlatyamroth force-pushed the bugfix/wrong-data-type-for-numbers branch from 6931c64 to 28dcf39 Compare February 17, 2021 23:55
@Flarna
Copy link
Member

Flarna commented Feb 18, 2021

I think also that the problem here is the backend which seems to take data and just display it.
It seems elastic apm is aware that that it displays a http.statusCode which is known to be an integer therefore it should be no big deal to convert it.

Assume other attributes which are real doubles. They may be sent sometimes as int and sometimes as double now. This may result in similar issues as you see it now - just reversed.

@kudlatyamroth
Copy link
Contributor Author

@Flarna thats why i check also with Number.isInteger which should eliminate problem with 1.32, as this will be send as double

@Flarna
Copy link
Member

Flarna commented Feb 18, 2021

Consider an attribute some.probability where double values in range [0, 1] are used.
0 and 1 are sent as integer now and all the rest as double.

Therefore a receiver has to be prepared to get mixed data even with this change and a receiver has to need to convert data in some cases.

Anyhow, this shouldn't be seen as blocking.

@kudlatyamroth
Copy link
Contributor Author

maybe use Decimal.js, if value is decimal.js always send as double?

@kudlatyamroth
Copy link
Contributor Author

should i do anything else to have it merged?

if it will be merged, can you release fix version 0.17.1 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants