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

Add a ThreadLocal metrics aggregator #1379

Merged
merged 6 commits into from
Feb 29, 2024
Merged

Conversation

Swatinem
Copy link
Member

Thus far, this only works for count metrics, but we can extend it in the future as well.

I would still call this pretty WIP for now. Ideally, I might extract all this metrics business into its own crate, or move it into the SDK crate.

@Swatinem Swatinem requested a review from a team February 19, 2024 14:09
@Swatinem Swatinem self-assigned this Feb 19, 2024
crates/symbolicator-js/src/metrics.rs Outdated Show resolved Hide resolved
crates/symbolicator-service/src/metrics.rs Outdated Show resolved Hide resolved
@Swatinem Swatinem force-pushed the swatinem/aggregating-metrics branch 3 times, most recently from dce41b7 to be4c89b Compare February 21, 2024 10:57
@Swatinem
Copy link
Member Author

I finished implementing this for all the metrics types we are using, meaning counter, gauge and distribution-like.

The results running symbolicator-stress look like this:

# no metrics

Workload 0 (concurrency: 50): 1378551 operations, 91903.40 ops/s

# unmodified `cadence` metrics

Workload 0 (concurrency: 50): 801336 operations, 53422.40 ops/s

# this PR with the thread-local aggregator

Workload 0 (concurrency: 50): 1202184 operations, 80145.60 ops/s

So this is a 50% improvement over using cadence on its own. And this reduces the overhead of collecting metrics to ~13% if my math is right.

For all the "distribution-like" metrics, we currently keep all the individual values, but we send them as batches. We could instead calculate the distribution and percentiles locally and avoid that, whereby potentially improving things further.

@Swatinem Swatinem requested a review from a team February 21, 2024 13:31
crates/symbolicator-service/src/metrics.rs Outdated Show resolved Hide resolved
crates/symbolicator-service/src/metrics.rs Outdated Show resolved Hide resolved
crates/symbolicator-service/src/metrics.rs Outdated Show resolved Hide resolved
crates/symbolicator-service/src/metrics.rs Outdated Show resolved Hide resolved
crates/symbolicator-service/src/metrics.rs Outdated Show resolved Hide resolved
@Swatinem Swatinem force-pushed the swatinem/aggregating-metrics branch from bcb4aea to 1f279bb Compare February 28, 2024 15:15
@Swatinem Swatinem merged commit 6a4a76b into master Feb 29, 2024
10 checks passed
@Swatinem Swatinem deleted the swatinem/aggregating-metrics branch February 29, 2024 09:00
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.

3 participants