introduce pprofile.AddAttribute helper #12390
Conversation
Codecov ReportAttention: Patch coverage is
❌ Your patch status has failed because the patch coverage (66.66%) is below the target coverage (95.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #12390 +/- ##
==========================================
- Coverage 92.00% 91.98% -0.03%
==========================================
Files 469 469
Lines 25301 25328 +27
==========================================
+ Hits 23279 23297 +18
- Misses 1614 1620 +6
- Partials 408 411 +3 ☔ View full report in Codecov by Sentry. |
|
@bogdandrutu I'm happy to hear your input on improvements here, especially with the loop through all attributes issue, and the use of |
Co-authored-by: Tim Rühsen <[email protected]>
Co-authored-by: Tim Rühsen <[email protected]>
| // AddAttribute updates an AttributeTable and a record's AttributeIndices to | ||
| // add a new attribute. | ||
| // The record can by any struct that implements an `AttributeIndices` method. | ||
| func AddAttribute(table AttributeTableSlice, record attributable, key string, value any) error { |
There was a problem hiding this comment.
How come the attributable type is not an exported type (of some package)?
There was a problem hiding this comment.
attributable is there to allow the behavior on any of the generated types that have attribute indices.
Location, Mapping, Sample.
Folks will only ever need to manipulate the generated struct. They don't need the interface.
There was a problem hiding this comment.
Thanks. I have a feeling there is a more general solution out there, but this is pragmatic.
Part of #7455.
Co-authored-by: Joshua MacDonald <[email protected]>
| for i := range table.Len() { | ||
| a := table.At(i) | ||
|
|
||
| if a.Key() == key && value == a.Value().AsRaw() { |
There was a problem hiding this comment.
nit: I know that AsRaw() could make a copy of the data. This is a reminder that it would be nice if you could write a.Value().Equal(value) and avoid the allocation.
There was a problem hiding this comment.
@dmathieu Happy to merge after you reply to this :)
There was a problem hiding this comment.
I have opened #12561 to address this in another PR, since it requires changed in pcommon.
| // AddAttribute updates an AttributeTable and a record's AttributeIndices to | ||
| // add a new attribute. | ||
| // The record can by any struct that implements an `AttributeIndices` method. | ||
| func AddAttribute(table AttributeTableSlice, record attributable, key string, value any) error { |
There was a problem hiding this comment.
Thanks. I have a feeling there is a more general solution out there, but this is pragmatic.
Part of #7455.
72a8471
Description
This introduces the
pprofile.AddAttributehelper method so profile extensions can modify attributes.Closes #12206
Those benchmarks kind of have an N+1 issue, where the method will take longer the more attributes there are.
However, since the attribute table is a map, we can't go around with looping through it.
I also had to use
FromRawandAsRawin there, to properly handleanyvalues.This currently doesn't support removing entries. But doing so would also have a non-trivial performance impact, since we'd have to loop through the
AttributeIndicesslice.