-
Notifications
You must be signed in to change notification settings - Fork 472
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
API to modify/remove an existing entry from LogRecord attributes #2103
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2103 +/- ##
=======================================
+ Coverage 76.9% 77.0% +0.1%
=======================================
Files 122 122
Lines 22213 22439 +226
=======================================
+ Hits 17082 17296 +214
- Misses 5131 5143 +12 ☔ View full report in Codecov by Sentry. |
Co-authored-by: Cijo Thomas <cithomas@microsoft.com>
…ry-rust into log-attributes-update
if let Some(attr) = self | ||
.attributes | ||
.iter_mut() | ||
.find(|opt| opt.as_ref().map(|(k, _)| k == key).unwrap_or(false)) | ||
{ | ||
// Take the old value and update the attribute | ||
let old_value = attr.take().map(|(_, v)| v); | ||
*attr = Some((key.clone(), value)); | ||
return old_value; | ||
} | ||
|
||
// If not found, add a new attribute | ||
self.add_attribute(key.clone(), value.clone()); | ||
None |
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.
Small nit on code style
maybe easier to read if we do
match self
.attributes
.iter_mut()
.find(|opt| opt.as_ref().map(|(k, _)| k == key).unwrap_or(false)) {
Some(attr) => {
// Take the old value and update the attribute
let old_value = attr.take().map(|(_, v)| v);
*attr = Some((key.clone(), value));
old_value
}
None => self.add_attribute(key.clone(), value.clone());
}
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 was little confused why we need to add_attribute
regardless until I see the early return
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.
Or you have already tried that and compiler complains because the we are still holding mutable reference to self when iterator found nothing?
Techiniqually if we returned None
, by that point the mutable reference to self
should be dropped, allowing us to call add_attribute
. But not sure if the refernece checker is smart enough
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.
Or you have already tried that and compiler complains because the we are still holding mutable reference to self when iterator found nothing?
@TommyCpp yes, i initially tried that approach, and got the error for still holding the mutable reference to self. Looks like the borrow checker is bit conservative and doesn't automatically drop iterators/temporaries until the end of the scope where they are used.
This is important, but trying to wrap up critical metric stuff/internal logging for the upcoming release, and will get back to this right after that. |
Fixes #1986
Changes
This PR adds two new methods,
update_attribute
anddelete_attribute
, to the public API of theLogRecord
struct.Merge requirement checklist
CHANGELOG.md
files updated for non-trivial, user-facing changes