Skip to content

Review suggestions for PR 14546 #1

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.

Sign up for GitHub

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

Merged
florianl merged 8 commits into from
Feb 23, 2026
Merged

Review suggestions for PR 14546 #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5300a78
pdata/pprofile: fix Key not cleared after conversion to KeyRef
felixge Feb 21, 2026
bad7c68
pdata/pprofile: refactor to make extra predicate redundant
felixge Feb 21, 2026
2a5504b
pdata/pprofile: remove flawed optimizations
felixge Feb 21, 2026
4cb2c3d
pdata/pprofile: optimize MarshalProfiles
felixge Feb 21, 2026
3c02751
pdata/pprofile: refactor some code duplication
felixge Feb 21, 2026
a33a81e
pdata/pprofile: split BenchmarkMarshalProfiles into with_refs and wit…
felixge Feb 21, 2026
daf8673
pdata/pprofile: strengthen wire compatibility test with proto.Equal
felixge Feb 21, 2026
231f4ff
pdata/pprofile: convert references in JSON marshaler/unmarshaler
felixge Feb 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
203 changes: 33 additions & 170 deletions pdata/pprofile/dictionary_helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,104 +8,44 @@ import (
"go.opentelemetry.io/collector/pdata/pcommon"
)

// mapKeyValues returns the underlying KeyValue slice of a pcommon.Map.
func mapKeyValues(m pcommon.Map) []internal.KeyValue {
return *internal.GetMapOrig(internal.MapWrapper(m))
}

// resolveProfilesReferences walks through all profiles data after unmarshaling
// and resolves any string_value_ref and key_ref to their actual string values.
// This ensures the pdata API works transparently with referenced strings.
func resolveProfilesReferences(profiles Profiles) {
dict := profiles.Dictionary()

// Quick check: if there are no resource profiles, nothing to do
if profiles.ResourceProfiles().Len() == 0 {
return
}

// Check if resolution is needed by sampling first resource
rp := profiles.ResourceProfiles().At(0)
if !needsResolution(rp.Resource().Attributes()) {
// Check scope attributes too
if rp.ScopeProfiles().Len() == 0 {
return
}
sp := rp.ScopeProfiles().At(0)
if !needsResolution(sp.Scope().Attributes()) {
// Already resolved, skip
return
}
}

// Resolve references in resource attributes
for i := 0; i < profiles.ResourceProfiles().Len(); i++ {
rp := profiles.ResourceProfiles().At(i)
resolveMapReferences(dict, rp.Resource().Attributes())
resolveKeyValueReferences(dict, mapKeyValues(rp.Resource().Attributes()))

// Resolve references in scope attributes
for j := 0; j < rp.ScopeProfiles().Len(); j++ {
sp := rp.ScopeProfiles().At(j)
resolveMapReferences(dict, sp.Scope().Attributes())
resolveKeyValueReferences(dict, mapKeyValues(sp.Scope().Attributes()))
}
}
}

// needsResolution checks if a map has any refs that need resolution
func needsResolution(m pcommon.Map) bool {
if m.Len() == 0 {
return false
}
mapOrig := internal.GetMapOrig(internal.MapWrapper(m))
for i := 0; i < len(*mapOrig); i++ {
kv := &(*mapOrig)[i]
// If KeyRef is set, needs resolution
if kv.KeyRef != 0 {
return true
}
// Check if any values need resolution
if anyValueNeedsResolution(&kv.Value) {
return true
}
}
return false
}

// anyValueNeedsResolution checks if an AnyValue has refs that need resolution
func anyValueNeedsResolution(anyValue *internal.AnyValue) bool {
if ref, ok := anyValue.Value.(*internal.AnyValue_StringValueRef); ok && ref.StringValueRef != 0 {
return true
} else if kvList, ok := anyValue.Value.(*internal.AnyValue_KvlistValue); ok && kvList.KvlistValue != nil {
for i := 0; i < len(kvList.KvlistValue.Values); i++ {
kv := &kvList.KvlistValue.Values[i]
if kv.KeyRef != 0 {
return true
}
if anyValueNeedsResolution(&kv.Value) {
return true
}
}
} else if arrVal, ok := anyValue.Value.(*internal.AnyValue_ArrayValue); ok && arrVal.ArrayValue != nil {
for i := 0; i < len(arrVal.ArrayValue.Values); i++ {
if anyValueNeedsResolution(&arrVal.ArrayValue.Values[i]) {
return true
}
}
}
return false
}

// resolveMapReferences resolves all string_value_ref and key_ref in a map
func resolveMapReferences(dict ProfilesDictionary, m pcommon.Map) {
mapOrig := internal.GetMapOrig(internal.MapWrapper(m))

for i := 0; i < len(*mapOrig); i++ {
kv := &(*mapOrig)[i]

// resolveKeyValueReferences resolves key_ref and string_value_ref in a KeyValue slice
func resolveKeyValueReferences(dict ProfilesDictionary, kvs []internal.KeyValue) {
for i := range kvs {
kv := &kvs[i]
// Resolve key_ref if set
if kv.KeyRef != 0 {
if kv.KeyRef >= 0 {
idx := int(kv.KeyRef)
if idx >= 0 && idx < dict.StringTable().Len() {
if idx < dict.StringTable().Len() {
kv.Key = dict.StringTable().At(idx)
// Keep ref set for potential re-marshaling
// N.b. keep KeyRef set to optimize re-marshaling. This is
// technically a violation of the proto spec, but acceptable
// for the in-memory pdata API since keys are immutable.
}
}

// Resolve string_value_ref if set
resolveAnyValueReference(dict, &kv.Value)
}
Expand All @@ -127,16 +67,7 @@ func resolveAnyValueReference(dict ProfilesDictionary, anyValue *internal.AnyVal
anyValue.Value = ov
}
} else if kvList, ok := anyValue.Value.(*internal.AnyValue_KvlistValue); ok && kvList.KvlistValue != nil {
for i := 0; i < len(kvList.KvlistValue.Values); i++ {
kv := &kvList.KvlistValue.Values[i]
if kv.KeyRef != 0 {
idx := int(kv.KeyRef)
if idx >= 0 && idx < dict.StringTable().Len() {
kv.Key = dict.StringTable().At(idx)
}
}
resolveAnyValueReference(dict, &kv.Value)
}
resolveKeyValueReferences(dict, kvList.KvlistValue.Values)
} else if arrVal, ok := anyValue.Value.(*internal.AnyValue_ArrayValue); ok && arrVal.ArrayValue != nil {
for i := 0; i < len(arrVal.ArrayValue.Values); i++ {
resolveAnyValueReference(dict, &arrVal.ArrayValue.Values[i])
Expand All @@ -151,32 +82,16 @@ func convertProfilesToReferences(profiles Profiles) {
dict := profiles.Dictionary()
stringTable := dict.StringTable()

// Quick check: if there are no resource profiles, nothing to do
if profiles.ResourceProfiles().Len() == 0 {
return
}

// Check if conversion is needed by sampling first resource
rp := profiles.ResourceProfiles().At(0)
if !needsConversion(rp.Resource().Attributes()) {
// Check scope attributes too
if rp.ScopeProfiles().Len() == 0 {
return
}
sp := rp.ScopeProfiles().At(0)
if !needsConversion(sp.Scope().Attributes()) {
// Already converted, skip
return
}
}

// Map for quick string lookups - only allocate if needed
stringIndex := make(map[string]int32, stringTable.Len())
for i := 0; i < stringTable.Len(); i++ {
stringIndex[stringTable.At(i)] = int32(i)
}

var stringIndex map[string]int32
getStringIndex := func(s string) int32 {
if stringIndex == nil {
stringIndex = make(map[string]int32, stringTable.Len())
for i := 0; i < stringTable.Len(); i++ {
stringIndex[stringTable.At(i)] = int32(i)
}
}

if idx, ok := stringIndex[s]; ok {
return idx
}
Expand All @@ -189,70 +104,25 @@ func convertProfilesToReferences(profiles Profiles) {
// Convert strings in resource attributes
for i := 0; i < profiles.ResourceProfiles().Len(); i++ {
rp := profiles.ResourceProfiles().At(i)
convertMapToReferences(getStringIndex, rp.Resource().Attributes())
convertKeyValueToReferences(getStringIndex, mapKeyValues(rp.Resource().Attributes()))

// Convert strings in scope attributes
for j := 0; j < rp.ScopeProfiles().Len(); j++ {
sp := rp.ScopeProfiles().At(j)
convertMapToReferences(getStringIndex, sp.Scope().Attributes())
convertKeyValueToReferences(getStringIndex, mapKeyValues(sp.Scope().Attributes()))
}
}
}

// needsConversion checks if a map has any string values that need conversion to refs
func needsConversion(m pcommon.Map) bool {
if m.Len() == 0 {
return false
}
mapOrig := internal.GetMapOrig(internal.MapWrapper(m))
for i := 0; i < len(*mapOrig); i++ {
kv := &(*mapOrig)[i]
// If KeyRef is not set but Key is, needs conversion
if kv.Key != "" && kv.KeyRef == 0 {
return true
}
// Check if any string values need conversion
if anyValueNeedsConversion(&kv.Value) {
return true
}
}
return false
}

// anyValueNeedsConversion checks if an AnyValue has string values that need conversion
func anyValueNeedsConversion(anyValue *internal.AnyValue) bool {
if strVal, ok := anyValue.Value.(*internal.AnyValue_StringValue); ok && strVal.StringValue != "" {
return true
} else if kvList, ok := anyValue.Value.(*internal.AnyValue_KvlistValue); ok && kvList.KvlistValue != nil {
for i := 0; i < len(kvList.KvlistValue.Values); i++ {
kv := &kvList.KvlistValue.Values[i]
if kv.Key != "" && kv.KeyRef == 0 {
return true
}
if anyValueNeedsConversion(&kv.Value) {
return true
}
}
} else if arrVal, ok := anyValue.Value.(*internal.AnyValue_ArrayValue); ok && arrVal.ArrayValue != nil {
for i := 0; i < len(arrVal.ArrayValue.Values); i++ {
if anyValueNeedsConversion(&arrVal.ArrayValue.Values[i]) {
return true
}
}
}
return false
}

// convertMapToReferences converts string keys and values to references
func convertMapToReferences(getStringIndex func(string) int32, m pcommon.Map) {
mapOrig := internal.GetMapOrig(internal.MapWrapper(m))

for i := 0; i < len(*mapOrig); i++ {
kv := &(*mapOrig)[i]
// convertKeyValueToReferences converts string keys and values to references in a KeyValue slice
func convertKeyValueToReferences(getStringIndex func(string) int32, kvs []internal.KeyValue) {
for i := range kvs {
kv := &kvs[i]

// Convert key to reference
if kv.Key != "" && kv.KeyRef == 0 {
kv.KeyRef = getStringIndex(kv.Key)
kv.Key = ""
}

// Convert string values to references
Expand All @@ -279,14 +149,7 @@ func convertAnyValueToReference(getStringIndex func(string) int32, anyValue *int
ov.StringValueRef = idx
anyValue.Value = ov
} else if kvList, ok := anyValue.Value.(*internal.AnyValue_KvlistValue); ok && kvList.KvlistValue != nil {
// Recursively convert nested key-value lists
for i := 0; i < len(kvList.KvlistValue.Values); i++ {
kv := &kvList.KvlistValue.Values[i]
if kv.Key != "" && kv.KeyRef == 0 {
kv.KeyRef = getStringIndex(kv.Key)
}
convertAnyValueToReference(getStringIndex, &kv.Value)
}
convertKeyValueToReferences(getStringIndex, kvList.KvlistValue.Values)
} else if arrVal, ok := anyValue.Value.(*internal.AnyValue_ArrayValue); ok && arrVal.ArrayValue != nil {
// Recursively convert arrays
for i := 0; i < len(arrVal.ArrayValue.Values); i++ {
Expand Down
Loading