Introduce MergeFrom for profiles #13928
Conversation
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (65.86%) 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 #13928 +/- ##
==========================================
- Coverage 91.67% 91.45% -0.22%
==========================================
Files 652 653 +1
Lines 42516 42891 +375
==========================================
+ Hits 38977 39228 +251
- Misses 2731 2815 +84
- Partials 808 848 +40 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Israel Blancas <[email protected]>
| return ErrDictionarySizeExceeded | ||
| } | ||
| if int64(destDict.AttributeTable().Len())+int64(sourceDict.AttributeTable().Len()) > math.MaxInt32 { | ||
| return ErrDictionarySizeExceeded |
There was a problem hiding this comment.
This returns many times the same error, making it a bit hard to know which table exceeds the size. How about building a dynamic error with the name of the table?
| attributeTable map[int32]int32 | ||
| } | ||
|
|
||
| func (ms Profiles) mergeDictionaries(destDict, sourceDict ProfilesDictionary) (*indexMappings, error) { |
There was a problem hiding this comment.
I would move this (and the other relevant methods) into the ProfilesDictionary struct.
Not as a public method (we probably don't need it exposed), but as mergeFrom.
| return nil | ||
| } | ||
|
|
||
| func stacksEqual(a, b Stack) bool { |
There was a problem hiding this comment.
For these, we have been introducing Equal() public methods. How about doing the same here?
|
|
||
| func applyStringMappingToValueType(v ValueType, stringMapping map[int32]int32) { | ||
| if idx := v.TypeStrindex(); idx >= 0 { | ||
| if newIdx, exists := stringMapping[idx]; exists { |
There was a problem hiding this comment.
This assumes the string always exists in the destination string table, which may not be the case.
We should use SetString here, which is meant exactly for that.
| } | ||
| } | ||
|
|
||
| func copyValueType(src, dest ValueType, stringMapping map[int32]int32) { |
There was a problem hiding this comment.
How about a ValueType.mergeFrom method instead?
| destIndices.EnsureCapacity(sourceIndices.Len()) | ||
| for i := 0; i < sourceIndices.Len(); i++ { | ||
| idx := sourceIndices.At(i) | ||
| if newIdx, exists := mapping[idx]; exists { |
There was a problem hiding this comment.
This has the same issue as above with data missing from the mapping table. It should use PutAttribute/SetString.
This introduces `Stack.Equal()`, to help with profiles merging. See #13928
| for i := 0; i < sourceProfiles.Len(); i++ { | ||
| sourceProfile := sourceProfiles.At(i) | ||
| destProfile := destProfiles.AppendEmpty() | ||
|
|
There was a problem hiding this comment.
Profiles should not get merged without further checks.
E.g. merging different sample types will invalidate all data. Also, for investigations, the timestamps are crucial. Here the destination timestamp is overwritten with the source timestamps. And similar things happen to other fields of the message.
3 participants
Description
Link to tracking issue
This is part of #13106
It introduces a new MergeFrom method, which allows merging to profiles.
Continuation fort #13588