Skip to content
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

batch updater for counter, timer, and dist summary #1003

Merged
merged 3 commits into from Nov 11, 2022

Conversation

brharrington
Copy link
Contributor

Adds a helper that can be used to batch updates to the delegate meter. The main use-case for this is timers or distributions summaries that need to be updated from a single thread with a high throughput.

For counters, this can easily be achieved by the user with a local variable to track the count and then calling increment with the amount when complete or at the desired batch interval. However, for timers and distribution summaries that doesn't work because the implementation needs the raw samples. In #941 a method was added so the samples could be accumulated in an array and posted in a batch. This helps a bit, but it is cumbersome to use, incurs memory overhead, and the full set of samples have to be traversed and processed when recorded.

This change introduces batch updaters that can be customized for a given implementation. So for registries like Atlas where it is possible to accumulate the stats without the memory overhead, it can provide an optimized implementation to do so.

@brharrington brharrington added this to the 1.3.11 milestone Nov 11, 2022
Adds a helper that can be used to batch updates to the
delegate meter. The main use-case for this is timers or
distributions summaries that need to be updated from a
single thread with a high throughput.

For counters, this can easily be achieved by the user
with a local variable to track the count and then calling
increment with the amount when complete or at the desired
batch interval. However, for timers and distribution
summaries that doesn't work because the implementation
needs the raw samples. In Netflix#941 a method was added so the
samples could be accumulated in an array and posted in
a batch. This helps a bit, but it is cumbersome to use,
incurs memory overhead, and the full set of samples have
to be traversed and processed when recorded.

This change introduces batch updaters that can be
customized for a given implementation. So for registries
like Atlas where it is possible to accumulate the stats
without the memory overhead, it can provide an optimized
implementation to do so.
Copy link
Contributor

@manolama manolama left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a bit worried about the overhead of the ref counter for instances with many, many meters but should be ok.

/** Create a new instance. */
AtlasMeter(Id id, Clock clock, long ttl) {
this.id = id;
this.clock = clock;
this.ttl = ttl;
lastUpdated = clock.wallTime();
batchUpdaterRefCount = new AtomicInteger(0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth looking at lazy initialization to avoid the overhead for every meter? Just worried about the high cardinality use case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I refactored it to avoid the ref count. It is a bit less self-contained as the swap types need to inspect updater and pass in a supplier so the meter can be resolved on updates in case the registry has expired them.

Swap implementations will check if updater type can
take a supplier to resolve the meter to avoid issues
with holding on to one that has expired.
@brharrington brharrington merged commit 4bef43c into Netflix:main Nov 11, 2022
@brharrington brharrington deleted the batch branch November 11, 2022 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants