Fix ECS label attribute handling in TranslateResourceMetadata and Remap* functions#1226
Conversation
lahsivjar
changed the title
TranslateResourceMetadata and Remap* functions
| // When false (OTel path), all unsupported attributes — including any that | ||
| // already carry a labels.* prefix — are treated as raw keys and re-normalized | ||
| // from scratch. |
There was a problem hiding this comment.
[For reviewers] This is validated in apm-data, see https://github.com/lahsivjar/apm-data/pull/new/mOTLP-test
| - key: labels.unsupported_key | ||
| value: | ||
| stringValue: foo | ||
| - key: labels.labels_my_value |
There was a problem hiding this comment.
[For reviewers] The double normalization (labels.labels*) is expected, see the prior comment about apm-data behaviour for MIS OTel mode - I dond't change the input data so this is emulating OTel data sent to MIS with attributes set as labels.xxx.
| if truncated != value.Str() { | ||
| attributes.PutStr(key, truncated) | ||
| if truncated := TruncateToECSMaxLength(value.Str()); truncated != value.Str() { | ||
| value.SetStr(truncated) |
There was a problem hiding this comment.
[For reviewers] We are only changing the value here - this is okay but what we were doing previously was not okay as we were inserting a new attribute while iterating over the pcommon.Map
| { | ||
| // Map-typed attributes have no supported label representation and are silently dropped. | ||
| name: "map-typed attribute dropped", | ||
| setAttrs: func(attrs pcommon.Map) { | ||
| for _, k := range []string{"a0", "a1", "a2", "a3", "a4", "a5", "a6", "a7", "a8", "a9"} { | ||
| attrs.PutStr(k, "v") | ||
| } | ||
| attrs.PutEmptyMap("nested.attr") | ||
| }, | ||
| want: map[string]any{ | ||
| "labels.a0": "v", "labels.a1": "v", "labels.a2": "v", | ||
| "labels.a3": "v", "labels.a4": "v", "labels.a5": "v", | ||
| "labels.a6": "v", "labels.a7": "v", "labels.a8": "v", | ||
| "labels.a9": "v", | ||
| }, | ||
| wantAbsent: []string{"a0", "a1", "a2", "a3", "a4", "a5", "a6", "a7", "a8", "a9", "nested.attr"}, | ||
| }, | ||
| { | ||
| // Two attributes that reduce to the same sanitized key both map to the same | ||
| // output label; the second insertion overwrites the first (last writer wins). | ||
| name: "key collision last writer wins", | ||
| setAttrs: func(attrs pcommon.Map) { | ||
| for _, k := range []string{"a0", "a1", "a2", "a3", "a4", "a5", "a6", "a7", "a8"} { | ||
| attrs.PutStr(k, "v") | ||
| } | ||
| attrs.PutStr("foo.bar", "first") | ||
| attrs.PutStr("foo_bar", "second") // both sanitize to "labels.foo_bar" | ||
| }, | ||
| want: map[string]any{ | ||
| "labels.a0": "v", "labels.a1": "v", "labels.a2": "v", | ||
| "labels.a3": "v", "labels.a4": "v", "labels.a5": "v", | ||
| "labels.a6": "v", "labels.a7": "v", "labels.a8": "v", | ||
| "labels.foo_bar": "second", | ||
| }, | ||
| wantAbsent: []string{"a0", "a1", "a2", "a3", "a4", "a5", "a6", "a7", "a8", "foo.bar", "foo_bar"}, | ||
| }, | ||
| { | ||
| // "labels.foo.bar" and "labels.foo_bar" both sanitize to the same output key | ||
| // "labels.labels_foo_bar". Last writer wins. | ||
| name: "labels-prefixed key collision last writer wins", | ||
| setAttrs: func(attrs pcommon.Map) { | ||
| for _, k := range []string{"a0", "a1", "a2", "a3", "a4", "a5", "a6", "a7", "a8"} { | ||
| attrs.PutStr(k, "v") | ||
| } | ||
| attrs.PutStr("labels.foo.bar", "dotted") | ||
| attrs.PutStr("labels.foo_bar", "plain") // both sanitize to "labels.labels_foo_bar" | ||
| }, | ||
| want: map[string]any{ | ||
| "labels.a0": "v", "labels.a1": "v", "labels.a2": "v", | ||
| "labels.a3": "v", "labels.a4": "v", "labels.a5": "v", | ||
| "labels.a6": "v", "labels.a7": "v", "labels.a8": "v", | ||
| "labels.labels_foo_bar": "plain", | ||
| }, | ||
| wantAbsent: []string{"a0", "a1", "a2", "a3", "a4", "a5", "a6", "a7", "a8", "labels.foo.bar", "labels.foo_bar"}, | ||
| }, | ||
| { | ||
| // Regression: mutating pcommon.Map inside Range caused an index-out-of-bounds panic. | ||
| // With uniform key sanitization, plain "a0" → "labels.a0" and "labels.a0" → | ||
| // "labels.labels_a0": no collision, both produce distinct output keys. | ||
| name: "no panic with 11 attrs including a labels-prefixed key", | ||
| setAttrs: func(attrs pcommon.Map) { | ||
| for _, k := range []string{"a0", "a1", "a2", "a3", "a4", "a5", "a6", "a7", "a8", "a9"} { | ||
| attrs.PutStr(k, "v") | ||
| } | ||
| attrs.PutStr("labels.a0", "existing") | ||
| }, | ||
| want: map[string]any{ | ||
| "labels.a0": "v", | ||
| "labels.labels_a0": "existing", | ||
| "labels.a1": "v", | ||
| "labels.a2": "v", | ||
| "labels.a3": "v", | ||
| "labels.a4": "v", | ||
| "labels.a5": "v", | ||
| "labels.a6": "v", | ||
| "labels.a7": "v", | ||
| "labels.a8": "v", | ||
| "labels.a9": "v", | ||
| }, | ||
| wantAbsent: []string{"a0", "a1", "a2", "a3", "a4", "a5", "a6", "a7", "a8", "a9"}, | ||
| }, |
There was a problem hiding this comment.
[For reviewers] These are the test cases that would fail/cause panic.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (27)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThis PR refactors ECS attribute translation in the APM processor by introducing a deferred-write pattern and adding a conditional label sanitization strategy. The main change adds a Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
vigneshshanmugam
left a comment
vigneshshanmugam
left a comment
There was a problem hiding this comment.
LGTM, thanks for the detailed test-cases, made it easier for the review.
| ) | ||
|
|
||
| // kv is a key-value pair for collecting deferred attribute insertions after a RemoveIf pass. | ||
| type kv struct { |
There was a problem hiding this comment.
nit: cant we use the otlpcommon.KeyValue ? I havent checked it though, just asking.
There was a problem hiding this comment.
That's from the proto package (and protocommon.KeyValue is also a proto struct), I don't think we need that heavy dependency just for this purpose. Happy to discuss further - can do a followup if we decide later.
| return kv{} | ||
| } | ||
| switch slice.At(0).Type() { | ||
| // TODO(lahsivjar): Can we assume all are same type and just use pcommon.Value#CopyTo? |
There was a problem hiding this comment.
++ was wondering the same as the logic seems to be the same for everything.
There was a problem hiding this comment.
Yeah, I am not sure too. I didn't want to do too drastic of a change to start with so will review this a bit later.
2 participants
Bugs fixed
1. Panic in
Remap*functions during map iterationRemapLogRecordAttributesToECSLabels(and the equivalent span/metric variants) usedRangeto iterate the attribute map while inserting new keys into it. This caused a panic for three cases: map/bytes/empty-typed attribute values (removed with no compensating insert), a plain attribute whose sanitized destination key already existed, and two attributes whose keys both sanitize to the same output key. Fixed by switching toRemoveIfwith deferred insertions collected into a slice and applied after iteration completes.2. APM intake path double-prefixing
labels.*resource attributesThe APM intake receiver places
labels.<key>andnumeric_labels.<key>directly on resource attributes (already sanitized).TranslateResourceMetadatawas treating these as raw unsupported keys and fully re-normalizing them, producinglabels.labels_my_valueinstead oflabels.my_value. Fixed by adding asanitizeExistingLabels boolparameter: the APM enricher path (telemetry.sdk.name == "ElasticAPM") passestrue, which sanitizes only the suffix of existinglabels.*/numeric_labels.*keys; the OTel path passesfalseand retains the existing full re-normalization behaviour.Validation
Both behaviours are validated against
apm-data(input/otlp/traces_test.go) — apm-data silently drops map/bytes/empty typed attributes and applies last-writer-wins on key collisions, which matches the processor's behaviour after this fix.