feat(sdk): add SdkLogRecord::set_attribute and remove_attribute by valkum · Pull Request #3783 · open-telemetry/opentelemetry-rust

valkum · 2026-10-09T15:24:04Z

Fixes #1986

Changes

This PR continues #2103 by @lalitb. That PR was closed because it had no activity. The first commit is #2103, squashed and rebased onto main, with @lalitb as the author. The second commit fixes the review comments from #2103. The later comments clean up the tests a bit to keep the diffs easier to follow.

New public methods on SdkLogRecord:

pub fn set_attribute<K, V>(&mut self, key: K, value: V) -> Option<AnyValue>
where
    K: AsRef<str> + Into<Key>,
    V: Into<AnyValue>;

pub fn remove_attribute(&mut self, key: impl AsRef<str>) -> usize;

pub fn retain_attributes<F>(&mut self, mut f: F) -> usize
where
    F: FnMut(&Key, &mut AnyValue) -> bool;

set_attribute gives the first occurrence of the key the new value and returns the old value. It removes all other occurrences of the key. If the key is not present, it adds the attribute and returns None.
remove_attribute removes all occurrences of the key and returns the number of removed attributes.

A LogProcessor can use these methods to change or redact attributes. After a call, no copy of the old value stays on the record.

Changes for the review comments on #2103:

update_attribute is now set_attribute, because the method also adds the attribute when the key is not present. Span::set_attribute uses the same name for the same behavior.
set_attribute removes duplicate keys, so a redaction processor does not keep a copy of the old value.
set_attribute takes K: Into<Key> and V: Into<AnyValue>, as add_attribute does. The value is moved and not cloned.
GrowableArray::remove_at and GrowableArray::iter_mut are replaced by one GrowableArray::retain_mut. It moves the elements and does not clone them. It refills the inline array from the overflow vector one time for each call, not one time for each removed element. get and push expect a full inline array while the overflow vector has elements, so the refill is necessary.

In the previous PR it was asked why the LogRecord trait would not also gain these methods. Here is my take on it:

Only log bridges use the trait, currently. A bridge sets the attributes itself, so it does not have to change or remove them.
A LogProcessor gets &mut SdkLogRecord. In the specification, changes to a record after emit are part of the SDK ReadWriteLogRecord, not of the Bridge API or Log Appender.
Reduce breaking changes as noone currently asked for these on the LogRecord trait.

Merge requirement checklist

CONTRIBUTING guidelines followed
Unit tests added/updated (if applicable)
Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
Changes in public API reviewed (if applicable)

Disclosure: I had Claude Code revive the closed PR and then cleaned up the PR further myself.

linux-foundation-easycla · 2026-10-09T15:24:17Z

The committers listed above are authorized under a signed CLA.

✅ login: lalitb / name: Lalit Kumar Bhasin (5d5f476)
✅ login: valkum / name: Rudi Floren (0ffb2fe, 96b5a0e, d0de055)

codecov · 2026-10-09T15:27:19Z

Codecov Report

❌ Patch coverage is 97.05882% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.4%. Comparing base (02b2f15) to head (e8df720).

Files with missing lines	Patch %	Lines
opentelemetry-sdk/src/logs/log_processor.rs	86.9%	6 Missing ⚠️
opentelemetry-sdk/src/logs/record.rs	98.5%	2 Missing ⚠️
opentelemetry-sdk/src/growable_array.rs	99.2%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #3783     +/-   ##
=======================================
+ Coverage   84.2%   84.4%   +0.1%     
=======================================
  Files        123     123             
  Lines      30484   30789    +305     
=======================================
+ Hits       25686   25989    +303     
- Misses      4798    4800      +2

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

opentelemetry-pr-dashboard · 2026-10-09T15:45:40Z

Pull request dashboard status

Waiting on reviewers · refreshed 2026-10-23 20:38 UTC

Review the latest changes.

Status above doesn't look right?

Just replied or pushed? Anything around or after the refresh time above may not be picked up yet — give it a few minutes.
Anything look wrong? Report it with what you expected; it helps us improve the dashboard.

darkraider01 · 2026-10-09T18:46:34Z

+        let mut collection = GrowableArray::<i32>::new();
+
+        // Try to modify values in an empty list
+        collection.retain_mut(|value| {


This doesn’t check that the closure never runs. Making it panic should the trick.

Changed. Thanks!

darkraider01 · 2026-10-09T18:49:16Z

+    #[test]
+    fn test_retain_mut_modify_all_elements() {
+        // Add more elements and modify them
+        let mut collection = collection_of(10);


With 10 elements, this only covers the inline array. Using DEFAULT_MAX_INLINE_CAPACITY + 3 and checking the full resulting sequence would cover overflow too.

darkraider01 · 2026-10-09T18:54:22Z


+    /// Retains only the elements for which `f` returns `true`, and preserves their order.
+    ///
+    /// This function performs the following operations in one pass:


IMO the method’s behavior and why we refill the inline array seem enough; the rest mostly walks through the code.

I rephrased the comment. Tried to stick to the original PR as much as possible.

darkraider01 · 2026-10-09T18:56:45Z

+            );
+            assert!(!record.attributes_contains(
+                &Key::from_static_str("key1"),
+                &AnyValue::String("value1".into())


This checks "value1", but we added "val1" above. Probably better to check that "key1" is gone altogether.

Added a commit that checks for the absence of key1.

herin049 · 2026-10-10T03:39:25Z

+    pub fn set_attribute<K, V>(&mut self, key: K, value: V) -> Option<AnyValue>
+    where
+        K: Into<Key>,
+        V: Into<AnyValue>,
+    {
+        let key = key.into();
+        let mut new_value = Some(value.into());
+        let mut old_value = None;
+
+        self.attributes.retain_mut(|attr| match attr {
+            Some((k, v)) if *k == key => match new_value.take() {
+                Some(new_value) => {
+                    old_value = Some(std::mem::replace(v, new_value));
+                    true
+                }
+                None => false,
+            },
+            _ => true,
+        });
+
+        if let Some(new_value) = new_value {
+            self.attributes.push(Some((key, new_value)));
+        }
+        old_value
+    }


We should consider avoiding unconditional allocations of the key (i.e. if one is performing an update)

Suggested change

pub fn set_attribute<K, V>(&mut self, key: K, value: V) -> Option<AnyValue>

where

K: Into<Key>,

V: Into<AnyValue>,

{

let key = key.into();

let mut new_value = Some(value.into());

let mut old_value = None;

self.attributes.retain_mut(|attr| match attr {

Some((k, v)) if *k == key => match new_value.take() {

Some(new_value) => {

old_value = Some(std::mem::replace(v, new_value));

true

}

None => false,

},

_ => true,

});

if let Some(new_value) = new_value {

self.attributes.push(Some((key, new_value)));

}

old_value

}

pub fn set_attribute<K, V>(&mut self, key: K, value: V) -> Option<AnyValue>

where

K: AsRef<str> + Into<Key>,

V: Into<AnyValue>,

{

let mut new_value = Some(value.into());

let mut old_value = None;

self.attributes.retain_mut(|attr| match attr {

Some((k, v)) if k.as_str() == key.as_ref() => {

match new_value.take() {

Some(new_value) => {

old_value = Some(std::mem::replace(v, new_value));

true

}

None => false,

}

}

_ => true,

});

if let Some(new_value) = new_value {

self.attributes.push(Some((key.into(), new_value)));

}

old_value

}

herin049 · 2026-10-15T15:20:38Z

+    ///   occurrences of the key are removed. After the call, the key occurs exactly once.
+    /// - If the key is not found, the attribute is added.
+    ///
+    /// A `LogProcessor` can use this method to redact a value: no copy of the old value


A LogProcessor can use this method to redact a value: no copy of the old value stays on the record.

Given that the primary use-case for these changes are centered around redaction, I wonder if it makes more sense to just introduce a function like this

/// Retains only the attributes for which `f` returns `true`. /// Returns the number of removed attributes. pub fn retain_attributes<F>(&mut self, mut f: F) -> usize where F: FnMut(&Key, &AnyValue) -> bool,

This will make it easier for more general filtering logic (e.g. using pattern matching).

Likewise for updating:

pub fn update_attributes<F>(&mut self, mut f: F) -> usize where F: FnMut(&Key, &AnyValue) -> Option<AnyValue>

Looking at the stdlib, many map types have a mutating retain, an overwriting insert, and a remove.

I think in that case we should just keep the shorthands as is and add

pub fn retain_attributes<F>(&mut self, mut f: F) -> usize where F: FnMut(&Key, &mut AnyValue) -> bool,

The shorthands make sure to keep allocations to a minimum now. With just the two closure-based methods, it would be up to the caller to keep allocations to a minimum.

Do we think adding update_attributes would be valuable?

Update: Disregard, I see you've opted for a retain-mut style retain attributes method.

lalitb · 2026-10-15T22:33:37Z

+    ///
+    /// - The number of removed occurrences of the key.
+    ///
+    pub fn remove_attribute(&mut self, key: &Key) -> usize {


Could we accept impl AsRef<str> here? Removal only needs the key name. This would let callers pass a borrowed string without creating an owned Key, and existing &Key arguments would still work.

lalitb · 2026-10-15T22:53:31Z

Small nit: could you update the set_attribute signature in the PR description? The implementation now requires K: AsRef<str> + Into<Key>.

Squash of open-telemetry#2103, rebased onto main.

valkum · 2026-10-16T12:06:11Z

SdkLogRecord::remove_attribute now takes impl AsRef<str> to compare without the need to construct a key.
I added SdkLogRecord::retain_attributes in the mutating form I suggested. It returns the number of removed attributes, the same as remove_attribute.

I also align some comments better with the style of the std lib docs.

- Rename `update_attribute` to `set_attribute`. The name now matches the upsert behavior, as in `Span::set_attribute`. - `set_attribute` removes other occurrences of the key, so a redaction processor removes all copies of a value. - Replace `remove_at` and `iter_mut` with one `retain_mut` pass. It moves the elements and does not clone them.

valkum · 2026-10-16T12:49:02Z

Ah, sorry. Had to push again. A normal cargo clippy run did not complain about the now unnecessary borrow in the tests.

valkum · 2026-10-23T14:47:16Z

/dashboard route:reviewers

valkum · 2026-10-23T14:47:51Z

Small nit: could you update the set_attribute signature in the PR description? The implementation now requires K: AsRef<str> + Into<Key>.

Done

opentelemetry-pr-dashboard · 2026-10-23T14:47:56Z

@valkum, this pull request was routed to reviewers. The handoff remains active across pushes until newer actionable human feedback arrives. Top-level feedback through this request will not return; unresolved review threads remain open.

valkum requested a review from a team as a code owner October 9, 2026 15:24

valkum force-pushed the feat/log-record-set-remove-attribute branch from ae15bb5 to 96b5a0e Compare October 9, 2026 15:29

opentelemetry-pr-dashboard Bot mentioned this pull request Oct 9, 2026

Pull Request Dashboard #3735

Open

valkum force-pushed the feat/log-record-set-remove-attribute branch from 96b5a0e to 35c8d62 Compare October 9, 2026 18:07

darkraider01 reviewed Oct 9, 2026

View reviewed changes

valkum force-pushed the feat/log-record-set-remove-attribute branch from 35c8d62 to d3434e7 Compare October 9, 2026 19:23

herin049 reviewed Oct 10, 2026

View reviewed changes

herin049 reviewed Oct 15, 2026

View reviewed changes

lalitb reviewed Oct 15, 2026

View reviewed changes

valkum force-pushed the feat/log-record-set-remove-attribute branch from 8ea2624 to a24d71b Compare October 16, 2026 11:51

feat(sdk): add API to modify and remove LogRecord attributes

5d25f5f

Squash of open-telemetry#2103, rebased onto main.

valkum force-pushed the feat/log-record-set-remove-attribute branch 2 times, most recently from d4d80c4 to e8df720 Compare October 16, 2026 12:05

valkum added 6 commits October 16, 2026 14:47

test(sdk): add collection_of helper to retain_mut tests

1e7c91e

test(sdk): split multi-case retain_mut and set_attribute tests

78ab6d3

test(sdk): check for absence of key1 in attributes

d059db3

fix(sdk): avoid allocation in SdkLogRecord::set_attribute

28fcf5d

feat(sdk): add SdkLogRecord::retain_attributes

0057127

valkum force-pushed the feat/log-record-set-remove-attribute branch from e8df720 to 0057127 Compare October 16, 2026 12:47

This comment has been minimized.

Sign in to view

Conversation

valkum commented Oct 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Merge requirement checklist

Uh oh!

linux-foundation-easycla Bot commented Oct 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Oct 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

opentelemetry-pr-dashboard Bot commented Oct 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull request dashboard status

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

herin049 Oct 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lalitb commented Oct 15, 2026

Uh oh!

valkum commented Oct 16, 2026

Uh oh!

valkum commented Oct 16, 2026

Uh oh!

This comment has been minimized.

valkum commented Oct 23, 2026

Uh oh!

valkum commented Oct 23, 2026

Uh oh!

opentelemetry-pr-dashboard Bot commented Oct 23, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

valkum commented Oct 9, 2026 •

edited

Loading

linux-foundation-easycla Bot commented Oct 9, 2026 •

edited

Loading

codecov Bot commented Oct 9, 2026 •

edited

Loading

opentelemetry-pr-dashboard Bot commented Oct 9, 2026 •

edited

Loading

herin049 Oct 16, 2026 •

edited

Loading