Prevent duplicate tags in router spans added by dynamic attributes by rohan-b99 · Pull Request #8865 · apollographql/router

rohan-b99 · 2026-02-05T11:40:23Z

When dynamic attributes are added via SpanDynAttribute::insert, SpanDynAttribute::extend, LogAttributes::insert, LogAttributes::extend, EventAttributes::insert or EventAttributes::extend and the key already exists, it will now replace the existing value to avoid duplicate attributes.

Checklist

Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.

Exceptions

Note any exceptions here

Notes

It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this. ↩
Configuration is an important part of many changes. Where applicable please try to document configuration examples. ↩
A lot of (if not most) features benefit from built-in observability and debug-level logs. Please read this guidance on metrics best-practices. ↩
Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions. ↩

apollo-librarian · 2026-02-05T11:40:32Z

✅ Docs preview has no changes

The preview was not built because there were no changes.

Build ID: c020198b5ace5c5d780bc300
Build Logs: View logs

rohan-b99 · 2026-02-05T12:10:02Z

    pub(crate) fn insert(&mut self, kv: KeyValue) {
-        self.attributes.push(kv);
+        // Replace existing attribute with same key, or add new one
+        if let Some(existing) = self.attributes.iter_mut().find(|a| a.key == kv.key) {


I'm not sure if there's any point of keeping this as a Vec<KeyValue> at this point, should probably be changed to a HashMap<Key, Value>

Looks like it used to be a map: 3fa7c11
But was changed to a LinkedList and then a Vec for performance improvements: #4251

I did some brief testing on this against ahash::HashMap and the results are pretty much as expected:

Comparing Vec<KeyValue> vs HashMap<Key, Value> for deduplication --- Insert single attributes (10 unique keys) --- Vec: insert 10 unique 10000 iterations, mean: 458.00ns, min: 125.00ns, max: 37.46µs HashMap: insert 10 unique 10000 iterations, mean: 769.00ns, min: 250.00ns, max: 78.17µs --- Insert with duplicates (initial 10 + 6 with 2 duplicates) --- Vec: initial + dynamic with dupes 10000 iterations, mean: 377.00ns, min: 166.00ns, max: 22.63µs HashMap: initial + dynamic with dupes 10000 iterations, mean: 642.00ns, min: 333.00ns, max: 35.00µs --- Extend with unique attributes (batch of 20) --- Vec: extend 20 unique 10000 iterations, mean: 2.57µs, min: 1.88µs, max: 55.83µs HashMap: extend 20 unique 10000 iterations, mean: 2.12µs, min: 1.79µs, max: 91.17µs --- Extend with 50% duplicates (batch of 20, 10 unique keys) --- Vec: extend 20 with 50% dupes 10000 iterations, mean: 1.77µs, min: 1.54µs, max: 54.17µs HashMap: extend 20 with 50% dupes 10000 iterations, mean: 1.54µs, min: 1.21µs, max: 125.38µs --- Large scale: 50 unique attributes --- Vec: extend 50 unique 10000 iterations, mean: 5.29µs, min: 4.83µs, max: 52.42µs HashMap: extend 50 unique 10000 iterations, mean: 4.33µs, min: 3.83µs, max: 45.08µs --- Large scale: 100 attributes, 50% duplicates --- Vec: extend 100 with 50% dupes 10000 iterations, mean: 10.41µs, min: 9.00µs, max: 876.46µs HashMap: extend 100 with 50% dupes 10000 iterations, mean: 7.34µs, min: 6.58µs, max: 58.17µs --- Realistic: pre-populate 10, then extend with 6 (2 dupes) --- Vec: realistic scenario 10000 iterations, mean: 264.00ns, min: 166.00ns, max: 709.00ns HashMap: realistic scenario 10000 iterations, mean: 306.00ns, min: 208.00ns, max: 458.00ns --- Small scale: 3 attributes --- Vec: insert 3 unique 10000 iterations, mean: 66.00ns, min: 0.00ns, max: 21.42µs HashMap: insert 3 unique 10000 iterations, mean: 83.00ns, min: 0.00ns, max: 209.00ns === Benchmark Complete ===

I think the Vec implementation is worth keeping as the complexity of avoiding duplicates is low

carodewig

Overall looks good - great find! I've added a few minor comments around refactoring but they're not critical

carodewig · 2026-02-10T16:54:42Z

+        let method = attrs
+            .attributes()
+            .iter()
+            .find(|kv| kv.key.as_str() == "http.method")
+            .unwrap();
+        assert_eq!(method.value.as_str(), Cow::Borrowed("POST"));
+
+        let route = attrs
+            .attributes()
+            .iter()
+            .find(|kv| kv.key.as_str() == "http.route")
+            .unwrap();
+        assert_eq!(route.value.as_str(), Cow::Borrowed("/new"));
+
+        let status = attrs
+            .attributes()
+            .iter()
+            .find(|kv| kv.key.as_str() == "http.status")
+            .unwrap();
+        assert_eq!(status.value.as_str(), Cow::Borrowed("200"));


Suggested change

let method = attrs

.attributes()

.iter()

.find(|kv| kv.key.as_str() == "http.method")

.unwrap();

assert_eq!(method.value.as_str(), Cow::Borrowed("POST"));

let route = attrs

.attributes()

.iter()

.find(|kv| kv.key.as_str() == "http.route")

.unwrap();

assert_eq!(route.value.as_str(), Cow::Borrowed("/new"));

let status = attrs

.attributes()

.iter()

.find(|kv| kv.key.as_str() == "http.status")

.unwrap();

assert_eq!(status.value.as_str(), Cow::Borrowed("200"));

let expected_kvs = [

("http.method", "POST"),

("http.route", "/new"),

("http.status", "200"),

];

for (key, expected_value) in expected_kvs {

let value = attrs.attributes().iter().find(|kv| kv.key.as_str() == key).unwrap().value;

assert_eq!(value.as_str(), Cow::Borrowed(expected_value), "key = {}", key);

}

Nit: thoughts on something like this? I don't have super strong feelings about it but do think it's a bit easier to read.
(Would also apply to test_event_attributes_extend_replaces_existing)

carodewig · 2026-02-10T16:56:28Z

+            if let Some(existing) = self.attributes.iter_mut().find(|a| a.key == kv.key) {
+                *existing = kv;
+            } else {
+                self.attributes.push(kv);
+            }


Should this just use self.insert? You could annotate the insert function with #[inline] if the function call overhead is concerning.

Same comment RE the various uses of self.attributes.iter_mut below - minor, but it would be nice to centralize the logic so that we don't inadvertently reintroduce this bug later

Good catch, I've introduced the upsert_attribute fn to reduce the duplication

…r to read

carodewig

That refactor made a big difference IMO - looks great!

Prevent duplicate tags in router spans added by dynamic attributes

5442a9f

rohan-b99 requested a review from a team February 5, 2026 11:40

This comment has been minimized.

Sign in to view

Include EventAttributes as well

5060e9a

rohan-b99 commented Feb 5, 2026

View reviewed changes

rohan-b99 added 4 commits February 5, 2026 12:57

Format

6b1f141

Add note about why Vec is used

f4d8de0

Format

2d28167

Add changeset

a0e3c58

rohan-b99 requested a review from a team as a code owner February 6, 2026 12:22

Merge branch 'dev' into rohan-b99/duplicate-router-span-keys

67f2175

carodewig reviewed Feb 10, 2026

View reviewed changes

rohan-b99 added 2 commits February 10, 2026 17:37

Use upsert_attribute function to reduce duplication, make tests easie…

b482149

…r to read

Merge branch 'dev' into rohan-b99/duplicate-router-span-keys

8e19c56

rohan-b99 requested a review from a team as a code owner February 10, 2026 17:37

Merge branch 'dev' into rohan-b99/duplicate-router-span-keys

6aca5a5

carodewig approved these changes Feb 11, 2026

View reviewed changes

Merge branch 'dev' into rohan-b99/duplicate-router-span-keys

3bb646a

pragl approved these changes Feb 12, 2026

View reviewed changes

Merge branch 'dev' into rohan-b99/duplicate-router-span-keys

ece5f4d

rohan-b99 enabled auto-merge (squash) February 12, 2026 09:57

rohan-b99 merged commit 83e974a into dev Feb 12, 2026
15 checks passed

rohan-b99 deleted the rohan-b99/duplicate-router-span-keys branch February 12, 2026 10:02

abernix mentioned this pull request Feb 24, 2026

prep release: v2.12.0 #8912

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent duplicate tags in router spans added by dynamic attributes#8865

Prevent duplicate tags in router spans added by dynamic attributes#8865
rohan-b99 merged 12 commits intodevfrom
rohan-b99/duplicate-router-span-keys

rohan-b99 commented Feb 5, 2026 •

edited

Loading

Uh oh!

apollo-librarian bot commented Feb 5, 2026 •

edited

Loading

Uh oh!

This comment has been minimized.

rohan-b99 Feb 5, 2026

Uh oh!

rohan-b99 Feb 5, 2026

Uh oh!

rohan-b99 Feb 5, 2026

Uh oh!

carodewig left a comment

Uh oh!

carodewig Feb 10, 2026

Uh oh!

carodewig Feb 10, 2026

Uh oh!

carodewig Feb 10, 2026

Uh oh!

rohan-b99 Feb 10, 2026

Uh oh!

carodewig left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

rohan-b99 commented Feb 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Footnotes

Uh oh!

apollo-librarian bot commented Feb 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Docs preview has no changes

Uh oh!

This comment has been minimized.

rohan-b99 Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

rohan-b99 Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

rohan-b99 Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

carodewig left a comment

Choose a reason for hiding this comment

Uh oh!

carodewig Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

carodewig Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

carodewig Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

rohan-b99 Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

carodewig left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rohan-b99 commented Feb 5, 2026 •

edited

Loading

apollo-librarian bot commented Feb 5, 2026 •

edited

Loading