[pdata/pprofile] MergeTo: reserve index 0 of empty destination dictionary tables - #15662
[pdata/pprofile] MergeTo: reserve index 0 of empty destination dictionary tables#15662pujitha24 wants to merge 2 commits into
Conversation
…nary tables Motivation: Profiles.MergeTo remaps entries from the source dictionary into the destination dictionary but never reserves index 0 of each destination table for that table's zero value. profiles.proto requires that "the element at index 0 MUST be the zero value for the dictionary's element type" (e.g. "" for string_table), since every unset reference (e.g. an unset Strindex) resolves to it. When the destination dictionary starts out empty, the first entry switched over lands at index 0 instead of the zero value, producing a non-conformant dictionary in which unset references silently resolve to real data. This is a data-correctness bug, not a crash: MergeTo itself does not panic or error. Concretely, if the first string merged into an empty destination is some real value (e.g. a semconv attribute key), any function/location that had an unset filename/name (Strindex == 0) will report that unrelated string as its filename/name after the merge, as described in the issue's pprofreceiver-based repro. Destinations that are already pre-populated with conformant zero values before calling MergeTo (the existing convention used throughout this package's own tests) are unaffected, since the fix only seeds tables that are still empty. Approach: Add reserveDictionaryZeroValues, called at the top of MergeTo (after the self-merge no-op check, before switchDictionary runs). It seeds index 0 of each of the destination dictionary's 7 tables (string, attribute, function, link, location, mapping, stack) with that table's zero value, but only for tables that are still empty. A table that already holds entries is left untouched, since inserting at index 0 would shift every existing index and invalidate references already pointing into it. This matches the fix suggested in the issue. Validation: Added TestProfilesMergeTo_ReservesDictionaryZeroValues, adapted from the issue's reproduction: it merges a conformant source (whose index 0 in every table is already the zero value, plus one real string at index 1) into a freshly empty destination, and asserts every destination table's index-0 entry is the zero value while index 1 still holds the real value. Confirmed the test fails (wrong value, then an index-out-of-range panic) when profiles_merge.go's fix is reverted, and passes with it applied. Ran: cd pdata/pprofile && go build ./... && go vet ./... && go test ./... All packages pass, including the existing MergeTo test suite in profiles_merge_test.go, which already covers destinations that are pre-populated with zero values by the caller and confirms no regression there. Fixes open-telemetry#15661 Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a data-correctness bug in pdata/pprofile where Profiles.MergeTo could produce a non-conformant destination dictionary when merging into an empty destination, causing unset references (index 0) to resolve to real (incorrect) data instead of the required zero value.
Changes:
- Seed index
0of each destination dictionary table with that table’s required zero value only when the destination table is empty, before dictionary remapping occurs. - Add a regression test that merges into a freshly empty destination and asserts every destination table preserves the zero-value at index
0. - Add a changelog entry documenting the bug fix for release notes.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
pdata/pprofile/profiles_merge.go |
Seeds zero-value entries in empty destination dictionary tables before remapping to preserve spec-required index-0 semantics. |
pdata/pprofile/profiles_merge_test.go |
Adds a regression test covering merge-into-empty-destination dictionary conformance (index 0 reserved for zero values). |
.chloggen/fix-pprofile-mergeto-dictionary-zero-value.yaml |
Release note entry describing the bug fix and its impact. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // unset Strindex) resolve to it. Tables that already hold entries are left | ||
| // untouched, since inserting at index 0 would invalidate every existing | ||
| // reference into them. | ||
| func reserveDictionaryZeroValues(dst ProfilesDictionary) { |
There was a problem hiding this comment.
Can we move this functionality into dictionary_helpers.go and maybe rename it to something like ensureDictionarySentinels()?
…ers.go Addresses review feedback from florianl: move the helper that seeds index 0 of destination dictionary tables into dictionary_helpers.go and rename it to ensureDictionarySentinels for clarity. Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
|
Good call, done — moved the helper into |
Motivation:
Profiles.MergeTo remaps entries from the source dictionary into the
destination dictionary but never reserves index 0 of each destination
table for that table's zero value. profiles.proto requires that "the
element at index 0 MUST be the zero value for the dictionary's element
type" (e.g. "" for string_table), since every unset reference (e.g. an
unset Strindex) resolves to it. When the destination dictionary starts
out empty, the first entry switched over lands at index 0 instead of
the zero value, producing a non-conformant dictionary in which unset
references silently resolve to real data. This is a data-correctness
bug, not a crash: MergeTo itself does not panic or error. Concretely,
if the first string merged into an empty destination is some real
value (e.g. a semconv attribute key), any function/location that had
an unset filename/name (Strindex == 0) will report that unrelated
string as its filename/name after the merge, as described in the
issue's pprofreceiver-based repro. Destinations that are already
pre-populated with conformant zero values before calling MergeTo (the
existing convention used throughout this package's own tests) are
unaffected, since the fix only seeds tables that are still empty.
Approach:
Add reserveDictionaryZeroValues, called at the top of MergeTo (after
the self-merge no-op check, before switchDictionary runs). It seeds
index 0 of each of the destination dictionary's 7 tables (string,
attribute, function, link, location, mapping, stack) with that table's
zero value, but only for tables that are still empty. A table that
already holds entries is left untouched, since inserting at index 0
would shift every existing index and invalidate references already
pointing into it. This matches the fix suggested in the issue.
Validation:
Added TestProfilesMergeTo_ReservesDictionaryZeroValues, adapted from
the issue's reproduction: it merges a conformant source (whose index 0
in every table is already the zero value, plus one real string at
index 1) into a freshly empty destination, and asserts every
destination table's index-0 entry is the zero value while index 1
still holds the real value. Confirmed the test fails (wrong value,
then an index-out-of-range panic) when profiles_merge.go's fix is
reverted, and passes with it applied.
Ran:
cd pdata/pprofile && go build ./... && go vet ./... && go test ./...
All packages pass, including the existing MergeTo test suite in
profiles_merge_test.go, which already covers destinations that are
pre-populated with zero values by the caller and confirms no
regression there.
Fixes #15661
Signed-off-by: Pujitha Paladugu 10557236+pujitha24@users.noreply.github.com
AI assistance: this change was drafted with Claude Code.