-
Notifications
You must be signed in to change notification settings - Fork 8
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.
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
Optimize proto translation for merger #40
Optimize proto translation for merger #40
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
draft lgtm
len(pb.ServiceTransactionMetrics)) | ||
for _, kstm := range pb.ServiceTransactionMetrics { | ||
m.TransactionGroups[k] = ktm | ||
// TODO: Either clone proto or add a comment that we change the input |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even better would be to get rid of the FromProto methods. I think we should be able to do that if we:
- create an empty CombinedMetrics when constructing a new merger, and merge the initial metrics into that (rather than using FromProto for the first metrics)
- change the Processor function type to accept an
*aggregationpb.CombinedMetrics
I think ideally we would make CombinedMetrics and friends unexported types, and only use them for merging. Perhaps we should move all merging logic to an internal package. WDYT?
The second point might be a heavy lift. If it is, then I think as an interim step we could still remove FromProto by merging onto an empty CombinedMetrics with no limits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW that doesn't need to happen in this PR. I can create a followup if you think it sounds reasonable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds reasonable, I will give it a go for point 1. Point 2 also I think is simple, will create a followup for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, love the idea of hinding the full model (minus the *Key
structs). Will give that a go to.
BTW, *Key
need to stay since the proto models are not comparable. This is because the proto models have some private fields (ref).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed with @axw offline, decided to create follow up PRs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change the Processor function type to accept an *aggregationpb.CombinedMetrics
I had the same idea. Agg key map access aesbodyhash is taking some time now, but can't be sure if performance is better without maps (sorted array merge) or with a temporary map (most likely worse).
BTW, *Key need to stay since the proto models are not comparable. This is because the proto models have some private fields
Could EqualVT
be useful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agg key map access aesbodyhash is taking some time now
Didn't quite get this but I am working on updating the Processor to use proto type and avoid the final translation after harvest.
Could EqualVT be useful?
We use *Key
as keys for our maps and map keys must be strictly comparable so probably not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! We may need to do something about directly merging a binary representation into an existing HLL++ sketch at some point. I think we should be able to do that non-breaking though.
Optimizes merging of metrics by optimizing translation to proto models.
Benchmark with main: