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

Ingest metrics with Sentry as well #1351

Closed
wants to merge 1 commit into from
Closed

Conversation

Swatinem
Copy link
Member

@Swatinem Swatinem commented Jan 29, 2024

The way we often &X.to_string() for cadence sake because it takes a &'a str, and now we have to .to_string() a shitton more because Sentry wants a Cow<'static, str> makes me die inside :-(

The overhead of using Sentry metrics looks like a ~2x-10x reduction in throughput:

# before
Workload 0 (concurrency: 10): 3140 operations, 209.33 ops/s
Workload 1 (concurrency: 10): 2496 operations, 166.40 ops/s
Workload 2 (concurrency: 10): 4581 operations, 305.40 ops/s
Workload 3 (concurrency: 100): 12127 operations, 808.47 ops/s
Workload 4 (concurrency: 100): 16652 operations, 1110.13 ops/s
Workload 5 (concurrency: 50): 17048 operations, 1136.53 ops/s

# after
Workload 0 (concurrency: 10): 1754 operations, 116.93 ops/s
Workload 1 (concurrency: 10): 1352 operations, 90.13 ops/s
Workload 2 (concurrency: 10): 1959 operations, 130.60 ops/s
Workload 3 (concurrency: 100): 999 operations, 66.60 ops/s
Workload 4 (concurrency: 100): 4112 operations, 274.13 ops/s
Workload 5 (concurrency: 50): 10243 operations, 682.87 ops/s

#skip-changelog

@Swatinem Swatinem self-assigned this Jan 29, 2024
@Swatinem Swatinem marked this pull request as ready for review January 29, 2024 14:19
@Swatinem Swatinem force-pushed the swatinem/sentry-metrics branch 2 times, most recently from 805b729 to 4d321c1 Compare February 5, 2024 15:20
Copy link

codecov bot commented Feb 5, 2024

Codecov Report

Attention: Patch coverage is 4.00000% with 24 lines in your changes are missing coverage. Please review.

Project coverage is 76.06%. Comparing base (5a85fd7) to head (4d321c1).
Report is 38 commits behind head on master.

❗ Current head 4d321c1 differs from pull request most recent head 0a9804c. Consider uploading reports for the commit 0a9804c to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1351      +/-   ##
==========================================
- Coverage   76.09%   76.06%   -0.03%     
==========================================
  Files         101      100       -1     
  Lines       15262    14851     -411     
==========================================
- Hits        11613    11297     -316     
+ Misses       3649     3554      -95     

@Swatinem Swatinem requested a review from a team March 13, 2024 09:29
Comment on lines 23 to 25
let sink = UdpMetricSink::from(&addrs[..], socket).unwrap();
let mut builder = StatsdClient::builder(prefix, sink);
let udp_sink = BufferedUdpMetricSink::from(&addrs[..], socket).unwrap();
let queuing_sink = QueuingMetricSink::from(udp_sink);
let mut builder = StatsdClient::builder(prefix, queuing_sink);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the same change as in #1360, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think you are looking at on old diff. I pretty much just rebased this far enough so that I can capture a profile for demonstration purposes :-D

@Swatinem
Copy link
Member Author

#1408 is now hooked up to the aggregator and sends Sentry metrics from the final aggregator thread.

@Swatinem Swatinem closed this Mar 14, 2024
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.

2 participants