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

math: simplify types conversion #204

Merged
merged 2 commits into from
Apr 26, 2024
Merged

math: simplify types conversion #204

merged 2 commits into from
Apr 26, 2024

Conversation

edsiper
Copy link
Member

@edsiper edsiper commented Apr 26, 2024

This PR aims to address some issues presented in Fluent Bit repo:

Note that internally we store everything as uint64_t, to deal with floating point numbers we need to avoid losing precision, so this PR consider changes by using memcpy() when storing data.

Other pending thing, we need decide how to handle NaN and Inf values (encoding/decoding)

This PR aims to address some issues presented in Fluent Bit
repo:

 - fluent/fluent-bit#8083
 - fluent/fluent-bit#8762

Signed-off-by: Eduardo Silva <eduardo@calyptia.com>
Signed-off-by: Eduardo Silva <eduardo@calyptia.com>
@edsiper edsiper changed the title math: simplify type conversion with casts and limits checks math: simplify types conversion Apr 26, 2024
@edsiper edsiper merged commit 651ee53 into master Apr 26, 2024
18 checks passed
@edsiper edsiper deleted the math-fixes branch April 26, 2024 18:05
@aidanleuck
Copy link

aidanleuck commented Apr 26, 2024

@edsiper I tested this change locally, I am still seeing a similar issue. Is there any reason we are not just casting from uint64_t to a double? We are going to lose precision, but we can just let the compiler handle doing the conversion rather than copying arbitrary memory. It seems to me the issue is that a double is a floating point value and a uint64_t is not.

Debug Output:

2024-04-26T19:08:59.360517800Z _Weather_Temperature{summary="Sweltering"} = 1.6304166312761136e-322
2024-04-26T19:08:59.360517800Z _Weather_Temperature{summary="Hot"} = 1.4821969375237396e-322
2024-04-26T19:08:59.360517800Z _Weather_Temperature{summary="Cool"} = 3.1620201333839779e-322
2024-04-26T19:08:59.360517800Z _Weather_Temperature{summary="Freezing"} = 9.8813129168249309e-323

@edsiper
Copy link
Member Author

edsiper commented Apr 26, 2024

Do u have the original payload that I can use for testing? Ideally we don't want to lose precision since we convert between different formats

@aidanleuck
Copy link

aidanleuck commented Apr 26, 2024

@edsiper
The payload is just a number (uint64_t) between 10 and 55. I just have a mock application that sends fake OpenTelemetry data periodically. When running in the debugger I can validate that the value coming into the function is correct.

@edsiper
Copy link
Member Author

edsiper commented Apr 26, 2024

Are the original values integers or floats/double?

@aidanleuck
Copy link

They are integers.

@edsiper
Copy link
Member Author

edsiper commented Apr 26, 2024

So maybe the problem is on another area. Can u provide the code/tool you use for testing?

@edsiper
Copy link
Member Author

edsiper commented Apr 26, 2024

Great, we look into that in a couple of hours.

So maybe the cast thing can still be handled as it was before

@aidanleuck
Copy link

Yes you are correct @edsiper the issue is not with the encode/decode functions in math.h but actually cmt_decode_opentelemetry.c:455
Changing the line to value = data_point->as_int and removing the conversion works for me!

@aidanleuck
Copy link

aidanleuck commented Apr 26, 2024

I also checked with the union implementation in cmt_math.h. I can confirm that the issue is with that line as opposed to the original implementation. This PR can be reverted.

@edsiper
Copy link
Member Author

edsiper commented Apr 26, 2024

Thanks, I will update the code shortly

@edsiper
Copy link
Member Author

edsiper commented Apr 27, 2024

FYI: #205

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants