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

buffer: Avoid calling dump_unique_id_hex if log level is not trace #4331

Merged
merged 1 commit into from
Jan 24, 2024

Conversation

Garfield96
Copy link
Contributor

@Garfield96 Garfield96 commented Oct 22, 2023

Which issue(s) this PR fixes:
N/A

What this PR does / why we need it:
This PR adapts the logging for buffers to avoid calls of dump_unique_id_hex if the current log level is not trace. During high-load situations with a small chunk size, this makes a noticeable difference.

Docs Changes:

Release Note:
buffer: Avoid unnecessary log processing. It will improve performance.

Signed-off-by: Christian Norbert Menges <christian.norbert.menges@sap.com>
Copy link

This PR has been automatically marked as stale because it has been open 30 days with no activity. Remove stale label or comment or this PR will be closed in 7 days

Copy link
Contributor

@daipom daipom left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM.
Sorry for the late response.

@daipom daipom added this to the v1.16.4 milestone Jan 24, 2024
@daipom
Copy link
Contributor

daipom commented Jan 24, 2024

This fix is not a bug fix, but a performance improvement.
So, I'm wondering if I should put this in v1.16.4 or v1.17.0.

For now, I'm thinking that we can put this in v1.16.4 because this changes codes for logging only.
If anyone has an opinion, please tell me. If not, I will merge this to the v1.16 branch.

@ashie
Copy link
Member

ashie commented Jan 24, 2024

All fixes should be merged into master at first, then consider whether or not back port it to v1.16 branch.
I agree backporting it to v1.16.

@ashie ashie merged commit 175d866 into fluent:master Jan 24, 2024
12 of 16 checks passed
@daipom
Copy link
Contributor

daipom commented Jan 24, 2024

Thanks! Sorry for the confusion. Yes. All fixes should be merged to the master branch first.

I wanted to say that if I merged this, I would forget that I was wondering whether to backport this or not, so I wanted to merge this after deciding it.

I'll wait a few days and backport this if there is no other opinion.

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

3 participants