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 metrics API #3205

Merged
merged 32 commits into from
Feb 28, 2024
Merged

Add metrics API #3205

merged 32 commits into from
Feb 28, 2024

Conversation

markushi
Copy link
Member

@markushi markushi commented Feb 14, 2024

📜 Description

Add metrics API to support delightful developer metrics. This PR intends to provide the basic functionality:

  • Metrics API for counters, sets, gauges, distributions and timings
  • A MetricAggregator to locally aggregate into 10s buckets
  • Flush / Closing functionality
  • Hub integration
  • Sending over the aggregated metrics using the new statsd envelope type
image

❓ Open Topics / Missing items

✅ How should the API entrypoint look like?

The aggregator is part of the hub, so it's either IHub.getMetricsApi() and our typical convenience Sentry.getMetricsApi()shortcut. It should definitely be in a sub-space, as all the different metrics, like Sentry.increment() would just pollute our top level namespace.

After some discussion, let's go for Sentry.metrics().increment(...)
✅ Is it "Metric" or "Metrics"?
Let's go for metrics, as the aggregator actually deals with multiple of them.
✅ Rate Limiting / Caching
I need to check if we need special handling for metrics on this.
✅ Automatically add global tags (e.g. release, environment)
✅ Add more tests, especially for the aggregator
✅ Use CR32 for hashing Strings values for set metrics

Out of scope / Will be done with follow up PR

  1. Recording and sending code locations for metrics
  2. Any span handling / local span aggregation

💡 Motivation and Context

Implements first parts of #3190

💚 How did you test it?

Added unit tests, taking some inspiration from https://github.com/getsentry/sentry-python/blob/master/tests/test_metrics.py and .NET

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

@markushi markushi marked this pull request as draft February 14, 2024 09:51
Copy link
Contributor

github-actions bot commented Feb 14, 2024

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 9b1f4aa

Copy link
Contributor

github-actions bot commented Feb 14, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 300.54 ms 361.31 ms 60.76 ms
Size 1.70 MiB 2.28 MiB 590.47 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
0810952 364.51 ms 468.08 ms 103.57 ms
2fad834 390.07 ms 470.80 ms 80.73 ms
2465853 422.61 ms 491.20 ms 68.58 ms
8ff8fd0 432.77 ms 495.18 ms 62.41 ms
2465853 400.64 ms 465.47 ms 64.83 ms
5e04ee8 365.26 ms 448.49 ms 83.23 ms
c554ca2 368.52 ms 430.74 ms 62.22 ms
4e29063 384.14 ms 447.74 ms 63.60 ms
0404ea3 394.73 ms 461.79 ms 67.06 ms
0404ea3 332.47 ms 401.12 ms 68.66 ms

App size

Revision Plain With Sentry Diff
0810952 1.72 MiB 2.27 MiB 558.21 KiB
2fad834 1.72 MiB 2.29 MiB 577.53 KiB
2465853 1.70 MiB 2.27 MiB 583.82 KiB
8ff8fd0 1.72 MiB 2.27 MiB 558.15 KiB
2465853 1.70 MiB 2.27 MiB 583.82 KiB
5e04ee8 1.70 MiB 2.27 MiB 584.64 KiB
c554ca2 1.70 MiB 2.27 MiB 582.25 KiB
4e29063 1.72 MiB 2.29 MiB 578.38 KiB
0404ea3 1.72 MiB 2.29 MiB 577.52 KiB
0404ea3 1.72 MiB 2.29 MiB 577.52 KiB

Previous results on branch: feat/metrics

Startup times

Revision Plain With Sentry Diff
67ad747 424.90 ms 467.60 ms 42.70 ms
ac0ecb9 425.69 ms 506.96 ms 81.27 ms
f5d47c0 300.14 ms 342.90 ms 42.76 ms
5254f26 411.85 ms 442.08 ms 30.23 ms
0140fbc 399.74 ms 470.37 ms 70.63 ms
f5aa57f 413.70 ms 552.20 ms 138.50 ms
0e0f3c4 385.67 ms 458.14 ms 72.47 ms
4edc1f3 371.24 ms 446.73 ms 75.49 ms
6dd67a4 381.21 ms 445.40 ms 64.19 ms
038e0ae 364.92 ms 446.39 ms 81.47 ms

App size

Revision Plain With Sentry Diff
67ad747 1.70 MiB 2.28 MiB 592.10 KiB
ac0ecb9 1.70 MiB 2.28 MiB 590.22 KiB
f5d47c0 1.70 MiB 2.27 MiB 586.58 KiB
5254f26 1.70 MiB 2.28 MiB 592.06 KiB
0140fbc 1.70 MiB 2.27 MiB 587.40 KiB
f5aa57f 1.70 MiB 2.28 MiB 590.22 KiB
0e0f3c4 1.70 MiB 2.27 MiB 587.24 KiB
4edc1f3 1.70 MiB 2.27 MiB 586.13 KiB
6dd67a4 1.70 MiB 2.27 MiB 587.20 KiB
038e0ae 1.70 MiB 2.28 MiB 590.25 KiB

@markushi markushi changed the title Add basic metrics API Add metrics API Feb 15, 2024
@markushi markushi marked this pull request as ready for review February 15, 2024 14:59
// aggregates all of the metrics data for a particular time period. The Value is a dictionary for
// the metrics,
// each of which has a key that uniquely identifies it within the time period
private final NavigableMap<Long, Map<String, Metric>> buckets = new ConcurrentSkipListMap<>();
Copy link
Member

Choose a reason for hiding this comment

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

@NotNull 😛

Copy link
Member

@stefanosiano stefanosiano left a comment

Choose a reason for hiding this comment

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

Last minor things!

Copy link
Member

@stefanosiano stefanosiano left a comment

Choose a reason for hiding this comment

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

Just fix the last test (MetricsHelperTest.sanitizeValue) and we're good to go!

* Update Changelog

* Revert "Revert "Add support for force-flushing metrics when weight is too high""

This reverts commit 07fc4e5.

* Fix changelog

* Address PR feedback

* Fix tests

* Fix remove test code

* Address PR feedback

* Update Changelog
@markushi markushi merged commit 39e3ed7 into main Feb 28, 2024
24 checks passed
@markushi markushi deleted the feat/metrics branch February 28, 2024 11:45
@sysmat
Copy link

sysmat commented Mar 2, 2024

  • when using Sentry.metrics().notifyAll(); I get error
IllegalMonitorStateException: current thread is not owner
    at java.lang.Object.notifyAll(Object.java)
    at sentry.SentryMain.main(SentryMain.java:33)
  • java 21, sentry 7.5.0

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.

5 participants