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

Target maybe static #477

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

Conversation

jelmansouri
Copy link

Our telemetry module, enables us to pool static strings and send them once instead of copying and writing a string at each log line. The target is part of the information that gets sent to do additional filtering when logs are consumed.
The goal is to introduce this optimization without breaking the public API, so we can benefit from the optimization in all of our dependencies.
The code is less DRY than I would have hoped so I'm very open to suggestions regarding how to write this is a better fashion!

@jelmansouri
Copy link
Author

Regarding the MSRV, the additional code is a further optimization in cases like info!(target: "my_target", "my log line") I can either remove it or try some trickery with cfg(version("1.when_it_was_introduced")). Maybe overkill and we can revisit when the MSRV is bumped.

@KodrAus
Copy link
Contributor

KodrAus commented Jan 17, 2022

Thanks for your patience and for working on this one @jelmansouri! It's a bit unfortunate that

info!(target: static_str(), "Some message");

won't produce a static target, since it won't match the literal arm in the macro. Before accepting this I think I'd like to see some more exploration of the motivations and your specific use-case, what lead to this particular approach, and what its benefits and drawbacks are. If you're already pooling static strings then a single allocation to convert any &str into owned Strings the first time you see them doesn't sound prohibitive to me, but you might have already been down that path 🙂

EFanZh pushed a commit to EFanZh/log that referenced this pull request Jul 23, 2023
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.

None yet

2 participants