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

feat(metrics): Add BeforeEmitMetrics callback #3799

Merged
merged 33 commits into from Mar 28, 2024

Conversation

philipphofmann
Copy link
Member

📜 Description

Add the BeforeEmitMetrics to the options, as described in develop docs https://develop.sentry.dev/sdk/metrics/#hooks.

💡 Motivation and Context

So users can discard metrics if they want to.

💚 How did you test it?

Unit tests.

📝 Checklist

You have to check all boxes before merging:

  • 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

Add classes for Set, Gauge, and Distribution.
Add LocalMetricsAggregator, which is the foundation for correlating
metrics to spans.
# Conflicts:
#	Sources/Swift/Metrics/LocalMetricsAggregator.swift
# Conflicts:
#	Sources/Swift/Metrics/LocalMetricsAggregator.swift
#	Tests/SentryTests/Swift/Metrics/LocalMetricsAggregatorTests.swift
The MetricsAPI for adding a set should accept a string, which gets
converted to a CRC32 hash instead of an integer.
For the set metric, the SDK casts the value back and forth from UInt to
Double, which doesn't work correctly with large UInt numbers as Double
can't represent them accurately. This is fixed now by only using UInt
for set metrics.
The MetricsAPI for adding a set should accept a string, which gets
converted to a CRC32 hash instead of an integer.
# Conflicts:
#	Sources/Swift/Metrics/BucketsMetricsAggregator.swift
#	Sources/Swift/Metrics/SentryMetricsAPI.swift
#	Sources/Swift/Metrics/SetMetric.swift
#	Tests/SentryTests/Swift/Metrics/SetMetricTests.swift
# Conflicts:
#	Sources/Swift/Metrics/BucketsMetricsAggregator.swift
#	Sources/Swift/Metrics/SentryMetricsAPI.swift
#	Sources/Swift/Metrics/SetMetric.swift
#	Tests/SentryTests/Swift/Metrics/BucketMetricsAggregatorTests.swift
#	Tests/SentryTests/Swift/Metrics/SetMetricTests.swift
Add the BeforeEmitMetrics to the options, as described in develop docs
https://develop.sentry.dev/sdk/metrics/#hooks.
Copy link

github-actions bot commented Mar 27, 2024

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

Generated by 🚫 dangerJS against b316400

Copy link

codecov bot commented Mar 27, 2024

Codecov Report

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

Project coverage is 89.568%. Comparing base (96eb740) to head (b316400).

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3799       +/-   ##
=============================================
+ Coverage   89.507%   89.568%   +0.061%     
=============================================
  Files          560       560               
  Lines        60739     60807       +68     
  Branches     21837     21866       +29     
=============================================
+ Hits         54366     54464       +98     
+ Misses        5446      5417       -29     
+ Partials       927       926        -1     
Files Coverage Δ
Sources/Sentry/SentryHub.m 98.613% <100.000%> (+0.002%) ⬆️
Sources/Sentry/SentryOptions.m 97.717% <100.000%> (+0.014%) ⬆️
...urces/Swift/Metrics/BucketsMetricsAggregator.swift 100.000% <100.000%> (ø)
Sources/Swift/Metrics/SentryMetricsAPI.swift 100.000% <100.000%> (ø)
Tests/SentryTests/SentrySDKTests.swift 93.687% <100.000%> (+0.138%) ⬆️
...s/Swift/Metrics/BucketMetricsAggregatorTests.swift 100.000% <100.000%> (ø)
...tryTests/Swift/Metrics/SentryMetricsAPITests.swift 99.056% <80.000%> (-0.944%) ⬇️
Tests/SentryTests/SentryOptionsTest.m 97.394% <71.428%> (-0.401%) ⬇️

... and 15 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 96eb740...b316400. Read the comment docs.

Copy link

github-actions bot commented Mar 27, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1244.20 ms 1256.31 ms 12.10 ms
Size 21.58 KiB 573.17 KiB 551.59 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
2a894d5 1211.02 ms 1236.72 ms 25.70 ms
c471221 1244.57 ms 1257.94 ms 13.37 ms
6e1452d 1241.69 ms 1253.47 ms 11.78 ms
16450b8 1196.33 ms 1217.26 ms 20.93 ms
c0b4b71 1218.16 ms 1251.28 ms 33.12 ms
7fb7afb 1235.00 ms 1256.81 ms 21.81 ms
e681215 6240.94 ms 6254.36 ms 13.42 ms
60d6cec 1257.14 ms 1273.92 ms 16.78 ms
c00eafe 1253.04 ms 1256.41 ms 3.37 ms
c6773e5 1222.48 ms 1240.02 ms 17.54 ms

App size

Revision Plain With Sentry Diff
2a894d5 21.58 KiB 414.57 KiB 392.99 KiB
c471221 22.85 KiB 413.89 KiB 391.04 KiB
6e1452d 21.58 KiB 419.68 KiB 398.10 KiB
16450b8 22.85 KiB 410.98 KiB 388.13 KiB
c0b4b71 20.76 KiB 430.98 KiB 410.22 KiB
7fb7afb 20.76 KiB 419.69 KiB 398.94 KiB
e681215 20.76 KiB 431.91 KiB 411.15 KiB
60d6cec 22.84 KiB 403.51 KiB 380.67 KiB
c00eafe 20.76 KiB 432.88 KiB 412.12 KiB
c6773e5 20.76 KiB 435.25 KiB 414.49 KiB

Previous results on branch: feat/metrics-before-emit-metrics

Startup times

Revision Plain With Sentry Diff
734807e 1201.16 ms 1215.06 ms 13.90 ms
27ff265 1236.67 ms 1250.98 ms 14.31 ms

App size

Revision Plain With Sentry Diff
734807e 21.58 KiB 573.19 KiB 551.61 KiB
27ff265 21.58 KiB 573.17 KiB 551.59 KiB

Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

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

LGTM!!

Only one suggestion.

Sources/Swift/Metrics/BucketsMetricsAggregator.swift Outdated Show resolved Hide resolved
Base automatically changed from ref/avoid-casting-for-set-metric to main March 28, 2024 08:47
# Conflicts:
#	Sources/Swift/Metrics/BucketsMetricsAggregator.swift
@philipphofmann philipphofmann merged commit b15521e into main Mar 28, 2024
68 of 70 checks passed
@philipphofmann philipphofmann deleted the feat/metrics-before-emit-metrics branch March 28, 2024 09:22
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