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

utils: floating-point number support in size-to-bytes conversion #8767

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Meetp369
Copy link

@Meetp369 Meetp369 commented Apr 26, 2024

This PR addresses an issue, which is that the flb_utils_size_to_bytes function is only processing non-floating numbers even after providing a floating-number in configuration.

It would address one aspect of the performance issues (Ref: #8673)


  • Consider a case where the input plugin is winevtlog and the output plugin is stdout.
  1. If read_limit_per_cycle is set to 1.8 M,
  • Configuration file for the above scenario
[SERVICE]
    flush        1
    daemon       Off
    log_level    debug
    parsers_file parsers.conf
    plugins_file plugins.con
    http_server  Off
    http_listen  0.0.0.0
    http_port    2020
    storage.metrics on

[INPUT]
    Name         winevtlog
    Channels     Setup,Windows PowerShell
    Interval_Sec 60
    read_existing_events true
    render_event_as_xml true
    read_limit_per_cycle 1.8M

[OUTPUT]
    name  stdout
    match *
  • Output for the above configuration in debug mode to check, which value is set up for the read_limit_per_cycle parameter.

[2024/04/23 18:34:22] [debug] [input:winevtlog:winevtlog.0] read limit per cycle is set up as 976.6K

  • It only sets 976.6k (human_readable_size) as the read_limit_per_cycle parameter value, but it should be 1.7M (human_readable_size).
  1. If read_limit_per_cycle is set to 1.2M
  • Output for the above configuration in debug mode to check which value is set up for the read_limit_per_cycle parameter if read_limit_per_cycle is set to 1.2M

[2024/04/23 18:42:00] [debug] [input:winevtlog:winevtlog.0] read limit per cycle is set up as 976.6K

  • In this case also, the read_limit_per_cycle parameter value sets 976.6k (human_readable_size), but it should be 1.1M (human_readable_size).

Enter [N/A] in the box, if an item is not applicable to your change.

Testing
Before we can approve your change; please submit the following in a comment:

  • 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

If this is a change to packaging of containers or native binaries then please confirm it works for all targets.

  • [N/A] Run local packaging test showing all targets (including any new ones) build.
  • [N/A] 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.

Signed-off-by: Meet <meetp0878@gmail.com>
Signed-off-by: Meet <meetp0878@gmail.com>
Copy link
Contributor

@cosmo0920 cosmo0920 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically, the change looks attractive. But, is there any chance to add unit tests to verify whether this change is effective or not?
You can add your adding test(s) in https://github.com/fluent/fluent-bit/blob/master/tests/internal/utils.c.

Signed-off-by: Meet <meetp0878@gmail.com>
@Meetp369
Copy link
Author

Basically, the change looks attractive. But, is there any chance to add unit tests to verify whether this change is effective or not? You can add your adding test(s) in https://github.com/fluent/fluent-bit/blob/master/tests/internal/utils.c.

Okay, I've added a unit test for the change!!

@Meetp369 Meetp369 requested a review from cosmo0920 May 16, 2024 19:15
Copy link
Contributor

@cosmo0920 cosmo0920 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks attractive. 👍

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.

None yet

2 participants