Implement Aggregation of Increment and Time measurements #55
Conversation
| // Bus Portal (busliniensuche.de) | ||
| // ========================================================================== | ||
| // All rights reserved. | ||
| // ========================================================================== |
There was a problem hiding this comment.
Contributions need to be made under the same license as the rest of the codebase
| .WriteTo.Emitter(pts => list.AddRange(pts)) | ||
| .CreateCollector(); | ||
|
|
||
| collector.Emit(new[] |
There was a problem hiding this comment.
Does this design require memory space for all of the points in an interval until they're aggregated at emit time? Just clarifying my quick initial reading of the code ... If so it might need a re-think; one of the goals of aggregation, at the client side, would be to maintain a low bound on memory usage in the presence of vast numbers of points. Apologies if I'm misunderstanding, here!
There was a problem hiding this comment.
You're right. Currently, the aggregation happens at emit time, i.e. if you're using the batcher, it happens when the batcher emits. My goal with this implementation was to reduce the amount of sent data. But your point is totally valid. Reducing the memory footprint would be a nice goal, too.
What is the scenario you're thinking about, i.e. what would be the batch and aggregation timespans?
There was a problem hiding this comment.
One aspect we should keep in mind is not to accidentally delay the sending of the measurements twice be buffering the aggregation and then buffering the sending.
There was a problem hiding this comment.
Thanks for the follow-up!
Just seems like a high-allocation metrics implementation is not ideal - generally apps that collect a lot of metrics need to be wary of this kind of caveat. It's not unusual to collect metrics in the multiple-thousands-of-hits/second, so if we're going to try optimizing for this case, aggregating at the point of collection seems pretty appealing (c.f. other metrics implementations).
|
@cypressious thanks again for the time spent on this. It's a nicely-implemented PR, but I'm not keen to move forward with it because I don't think we'll ultimately be able to build on a buffer-then-aggregate design without an eventual reset to avoid the allocations. Let me know if you're keen to dig into it further 👍 Thanks! |
|
@nblumhardt I will rework the PR. I'll implement a similar mechanism to the current batching approach where data will be periodically aggregated and then flushed down the pipeline. |
cypressious
force-pushed
the
feature-aggregate
branch
2 times, most recently
from
199aaab
to
ab0b8f1
Compare
|
@nblumhardt I've finished the rework. I've reimplemented the aggregation to run periodically, just like the batching. They also both derrive from the same base class now. Because the aggregation now runs periodically, it needs to run before the batching in the pipeline. I have one concern with the current implementation. Because points are allowed to have arbitrary timestamps, points that are buffered during each tick aren't guaranteed to really be from the same interval. That's why I still need to group them into time buckets. However, time buckets aren't necessarily aligned at the times when the aggregation happens. So points from one timer interval will most likely fall into two time buckets, even though the interval and bucket "length" is the same. In practice, this means that we will (almost) always have twice the number of transmitted points. I'll see if I can come up with a solution.
|
cypressious
force-pushed
the
feature-aggregate
branch
from
ab0b8f1
to
dca3b4a
Compare
cypressious
force-pushed
the
feature-aggregate
branch
from
dca3b4a
to
b47e85f
Compare
cypressious
force-pushed
the
feature-aggregate
branch
from
1d44eff
to
f708be2
Compare
|
@nblumhardt I think I've found a solution for the problem described above. Points within the timer interval, i.e. inside I think this PR is ready for review. |
|
@nblumhardt did you have time to look at the PR? |
|
Hi @cypressious - thanks for the follow-up and sorry about the slow response; my time for this project is currently low. Just to clarify my last comment:
By buffer-then-aggregate, I mean, collecting up the By an eventual reset, I mean, if we don't accommodate this goal into the design from the start, we probably won't be able to do it in a future version without starting afresh. I think to dig in further requires some discussion of the goals/design space etc., and probably needs another contributor or two to join the conversation to provide some perspective. |
|
@nblumhardt Thanks for the reply. I misunderstood your last comment but I think I get your point now. For sums, the bucket approach seems ideal. Other kinds of aggregations can be done too. For averages, we could store the sum and the count to compute I'm still interested in implementing this feature. Do you have suggestions on which contributors to get involved? |
|
👍 I'll put the call out over on the original (#9) ticket. Cheers! |
Fixes #9