-
Notifications
You must be signed in to change notification settings - Fork 443
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
Make LogRecord fields private and add getters for encapsulation #2314
base: main
Are you sure you want to change the base?
Conversation
#[inline] | ||
pub fn target(&self) -> Option<&Cow<'static, str>> { | ||
self.target.as_ref() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning &str
would simplify the API, but want to preserve the ability for consumers(i.e., exporter) to optimize based on whether the target is a static string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could favor simplicity of the API here and use &str
as the return type. The current exporters that we have cannot utilize the static
str reference to their benefit anyway.
Only exporters that would benefit from Cow<'static, str>
would be the ones that hold these values in memory for some reason as they could then avoid the cost to clone a static str.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a strong opinion here as it's mainly used by exporters and not end users. We can expect exporter authors to work with Cow<'static,str>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a strong opinion here as it's mainly used by exporters and not end users. We can expect exporter authors to work with Cow<'static,str>.
Yes, that's what I thought - keeping it for the custom export authors if they want to reap the benefit of static target. Existing exporter (in main and contrib repo) won't get the benefit as they serialize the data to be sent across.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2314 +/- ##
=====================================
Coverage 79.5% 79.5%
=====================================
Files 123 123
Lines 21258 21275 +17
=====================================
+ Hits 16905 16924 +19
+ Misses 4353 4351 -2 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
Changes
The PR refactors the
LogRecord
struct by making all its fields crate-private for better abstraction. Public getter methods have been added to provide controlled access to the fields.TBD - Will add changelog, if the changes are agreed.
Merge requirement checklist
CHANGELOG.md
files updated for non-trivial, user-facing changes