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

input_chunk: fix input plugin metrics not counting records re-emitted by filters downstream #9487

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mindw
Copy link

@mindw mindw commented Oct 13, 2024

move input metrics collections before applying filters.

Fixes #8729


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:

  • [N/A] Example configuration file for the change
  • [N/A] 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

  • 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.

* move metrics collections before applying filters.

Signed-off-by: Gabi Davar <grizzly.nyo@gmail.com>
@patrick-stephens
Copy link
Contributor

Should this be targeting the 3.0 or 3.1 branches? master is currently going to be the next 3.2 release: https://github.com/fluent/fluent-bit/wiki/Fluent-Bit-Roadmap

@mindw
Copy link
Author

mindw commented Oct 14, 2024

@patrick-stephens not sure how backporting works in the project. Typically master is first followed by backport PRs.

@patrick-stephens
Copy link
Contributor

@patrick-stephens not sure how backporting works in the project. Typically master is first followed by backport PRs.

Yup, just trying to understand if this affects just 3.0.1 or more widely. I think you need to tick the backport checkbox then as well and ideally provide the related PRs for those backports.

@mindw
Copy link
Author

mindw commented Oct 14, 2024

@patrick-stephens not sure how backporting works in the project. Typically master is first followed by backport PRs.

Yup, just trying to understand if this affects just 3.0.1 or more widely. I think you need to tick the backport checkbox then as well and ideally provide the related PRs for those backports.

This affects 2.2.0+. 2.1.10 was the last version without this bug.

@edsiper
Copy link
Member

edsiper commented Oct 14, 2024

checking on this

@edsiper
Copy link
Member

edsiper commented Oct 14, 2024

@braydonk would you please check this PR since it seems related to the change introduced in 1a8dd39 ?

@mindw mindw changed the title Fixes #8729 Unexpected behavior for metrics after the migration to 3.0.1 Fixes 8729 Unexpected behavior for metrics after the migration to 3.0.1 Oct 15, 2024
@braydonk
Copy link
Contributor

braydonk commented Oct 15, 2024

This bug was introduced by me in #8223 and #8229. Because of the way the multiline and rewrite_tag Filter plugins work, the input_records and input_bytes metrics would end up getting recorded only for the emitter plugins associated with each of those plugins and not the original input source.

The reason I did that at the time was that this commit ac4ec1f at a glance looked like metrics were supposed to be recorded after writing to the input chunk. However that was a mistaken interpretation on my part at the time, and the important detail was that metrics were recorded before filters were applied.

Thank you @mindw for catching this, sorry for the inconvenience.

@mindw mindw changed the title Fixes 8729 Unexpected behavior for metrics after the migration to 3.0.1 input_chunk: fix input plugin metrics not counting records re-emitted by filters downstream (issue #8729) Oct 16, 2024
@mindw mindw changed the title input_chunk: fix input plugin metrics not counting records re-emitted by filters downstream (issue #8729) input_chunk: fix input plugin metrics not counting records re-emitted by filters downstream Oct 16, 2024
@mindw
Copy link
Author

mindw commented Oct 16, 2024

@braydonk @edsiper, thank you for the quick turn around. I believe the PRs' title should pass the checker.

@mindw
Copy link
Author

mindw commented Oct 17, 2024

@braydonk , @edsiper as CI is green across the board. Is anything left for me to do here?

@braydonk
Copy link
Contributor

Approving the PR is the extent of my authority, it's up to maintainers from here.

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.

Makes sense.

@mindw
Copy link
Author

mindw commented Oct 26, 2024

@edsiper @braydonk it would be great to have this fix part the upcoming 3.2.0 release :)

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

Successfully merging this pull request may close these issues.

Unexpected behaviour for metrics after the migration to 3.0.1
5 participants