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

Improve API around transforming loggers #854

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

morgen-peschke
Copy link
Contributor

Inspired by the tedium of #835

@rossabaker
Copy link
Member

I have imagined something like this, but it does result in a switch statement on every logging operation, at least for the common slf4j backend. Does anybody have an inkling of the performance impact of this?

@morgen-peschke
Copy link
Contributor Author

morgen-peschke commented Jun 17, 2024

I have imagined something like this, but it does result in a switch statement on every logging operation, at least for the common slf4j backend. Does anybody have an inkling of the performance impact of this?

I'm not sure, though I'm optimistic.

Since there's already at least 1-2 conditionals per logging operation (checking the log level, and if to restore or clear the MDC, in addition to whatever goes on inside the MDC and Log4J) my gut-check is it won't be enough to move the needle 🤞🏻

I don't think we have timing checks on the log messages performance tests, though #850 would be a great place to add those (not sure what that would look like, how many messages do we expect to be able to reliable log in a tight loop within N seconds?)

@rossabaker
Copy link
Member

Yeah, any performance metrics here risk falling into the same trap we see in the IO world: the speed of successive flatMaps can matter while the IO is trivial, but the speed of a gajillion flatMaps is insignificant in the vastness of a real network IO.

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.

2 participants