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

cmt_math: fix metric conversions #8762

Closed
wants to merge 1 commit into from

Conversation

aidanleuck
Copy link

@aidanleuck aidanleuck commented Apr 25, 2024

Fixed issue with metric conversions where garbage values were being returned from a union. This method now implicitly does a cast as seemed to be intended by the original method. This issue was found because I was having an issue where fluent bit was spitting out garbage values for my metric counters when using the opentelemetry_input function.

Fixes #8083

  • Example configuration file for the change
  • Debug log output from testing the change
  • [ N/A ] Attached Valgrind output that shows no leaks or memory corruption was found
  • Run local packaging test showing all targets (including any new ones) build.
  • Set ok-package-test label to test for all targets (requires maintainer to do).

Documentation

  • [ N/A ] Documentation required for this feature

Backporting

  • [ N/A ] Backport to latest stable release.

Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.

@aidanleuck
Copy link
Author

aidanleuck commented Apr 25, 2024

Example Configuration with change

[SERVICE]
    Flush        1
    Daemon       Off
    Log_level    debug
    Http_Server  On
    Config_Watch On
    Parsers_File parsers.conf
    Parsers_File custom_parsers.conf
    Health_Check Off
[INPUT]
    name opentelemetry
    listen 0.0.0.0
    port 4318
    successful_response_code 200
    tag otel
[OUTPUT]
    Name stdout
    Match otel

@aidanleuck
Copy link
Author

Debug output for change:
Before:

[2024/04/25 16:01:45] [debug] [task] created task=0x7ffff0048c70 id=0 OK
[2024/04/25 16:01:45] [debug] [output:stdout:stdout.0] task_id=0 assigned to thread #0
2024-04-25T16:01:45.653451200Z _Weather_Temperature{summary="Warm"} = 2.4703282292062327e-322
2024-04-25T16:01:45.653451200Z _Weather_Temperature{summary="Cool"} = 4.001931731314097e-322
2024-04-25T16:01:45.653451200Z _Weather_Temperature{summary="Scorching"} = 6.4228533959362051e-323
2024-04-25T16:01:45.653451200Z _Weather_Temperature{summary="Bracing"} = 2.1738888417014848e-322
[2024/04/25 16:01:45] [debug] [out flush] cb_destroy coro_id=0
[2024/04/25 16:01:45] [debug] [task] destroy task=0x7ffff0048c70 (task_id=0)

After:

[2024/04/25 15:54:45] [debug] [output:stdout:stdout.0] task_id=0 assigned to thread #0
2024-04-25T15:54:45.653401200Z _Weather_Temperature{summary="Warm"} = 50
2024-04-25T15:54:45.653401200Z _Weather_Temperature{summary="Cool"} = 81
2024-04-25T15:54:45.653401200Z _Weather_Temperature{summary="Scorching"} = 13
2024-04-25T15:54:45.653401200Z _Weather_Temperature{summary="Bracing"} = 44
[2024/04/25 15:54:45] [debug] [out flush] cb_destroy coro_id=26
[2024/04/25 15:54:45] [debug] [task] destroy task=0x7ffff0047c40 (task_id=0)

@cosmo0920
Copy link
Contributor

Hi, thanks for fixing this!

However, we're tracking down for cmetrics' issues in https://github.com/fluent/cmetrics.
Could you send your patch there instead of this repo?

edsiper added a commit to fluent/cmetrics that referenced this pull request Apr 26, 2024
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 <[email protected]>
@edsiper
Copy link
Member

edsiper commented Apr 26, 2024

@aidanleuck thanks for triagging and proposing a solution for this issue.

Some context: for fixes that lives in lib/, they should be handled in a separate repository and then we sync up the project here in this one.

To speed up things, I noticed the code I original wrote uses an union when is not needed since we can do a direct cast, not sure why the code suggested here works but I suspect it can be something around how the compiler is working differently behind the scenes.

With the intention to speed up a fix and provide a cleaner version with also limit checks, I have submitted this PR:

fluent/cmetrics#204

My hope is that the issue goes away, but if still persist we will need to investigate more.

edsiper added a commit to fluent/cmetrics that referenced this pull request Apr 26, 2024
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 <[email protected]>
@aidanleuck
Copy link
Author

@edsiper Thank you for opening up that PR in the lib repository, I appreciate it! I will close this pull request.

@aidanleuck aidanleuck closed this Apr 26, 2024
patrick-stephens pushed a commit to fluent/cmetrics that referenced this pull request Dec 19, 2024
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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

opentelemetry metrics are not decoded / encoded as expected
3 participants