Make pprofile attribute table a real slice #11706
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #11706 +/- ##
==========================================
+ Coverage 91.54% 91.57% +0.02%
==========================================
Files 442 444 +2
Lines 23792 23873 +81
==========================================
+ Hits 21780 21861 +81
Misses 1641 1641
Partials 371 371 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
dmathieu
force-pushed
the
attributetable-slice
branch
from
cc7636f
to
f8fe840
Compare
| if lai := sample.Attributes().Len(); lai > 0 { | ||
| b.logEntry(" Attributes:") | ||
| for j := 0; j < lai; j++ { | ||
| if sample.Attributes().At(j) <= math.MaxInt32 { |
There was a problem hiding this comment.
The next version of the proto uses an int32 rather than an uint64 here. So after the next proto release/upgrade, we won't need this check anymore.
| // | ||
| // Must use NewAttributeTableSlice function to create new instances. | ||
| // Important: zero-initialized instance is not valid for use. | ||
| type AttributeTableSlice struct { |
There was a problem hiding this comment.
There was a problem hiding this comment.
I think this is a comment for the proto, not pprofile. Should we file an issue at the proto repo?
There was a problem hiding this comment.
Not sure I understand, what is this repeated key/value and what is the behavior if duplicates are present.
There was a problem hiding this comment.
AttributeTable is an array of key/values meaning attributes. They are an index though, not the actual list of attributes (though everything in that index should be used by samples in the profile).
Then, each sample has an array of integers, corresponding to the index of entries in AttributeTable.
If we make it a map, we can't have duplicate keys (with different values).
As for pcommon.Slice, I'm not seeing it being used by anything else. In every signal, we define slice structs specifically.
Description
AttributeTablewas setup as a map of attributes, same as they are used in traces.However, for profiles, they have a different meaning. They store a slice of attributes, and samples will point to the index of each attribute rather than duplicate the value.
So this can't be a map, it must be a slice of key/values, which can be duplicated and need a reliable index.
See https://github.com/open-telemetry/opentelemetry-proto/blob/793e43e25f2eb8063c2693bc2e66e16672cf7ed8/opentelemetry/proto/profiles/v1development/profiles.proto#L204-L205
cc @mx-psi