Provide API for attaching custom measurements to transactions by adinauer · Pull Request #2260 · getsentry/sentry-java

adinauer · 2022-09-27T14:19:52Z

📜 Description

Add API for setting custom measurements.

💡 Motivation and Context

getsentry/team-mobile#51
Closes #2143

💚 How did you test it?

📝 Checklist

I reviewed the submitted code
I added tests to verify the changes
I updated the docs if needed
No breaking changes

🔮 Next steps

github-actions · 2022-09-27T14:20:58Z

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

Generated by 🚫 dangerJS against f1747c8

github-actions · 2022-09-27T14:36:25Z

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	329.92 ms	363.72 ms	33.80 ms
Size	1.73 MiB	2.29 MiB	579.43 KiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
`1e4690d`	354.69 ms	387.88 ms	33.19 ms
`3d89dea`	345.59 ms	364.06 ms	18.47 ms
`7300956`	324.20 ms	353.79 ms	29.58 ms
`c5ccd8a`	329.98 ms	365.52 ms	35.54 ms
`3d89dea`	322.38 ms	350.82 ms	28.45 ms
`7300956`	337.57 ms	384.21 ms	46.64 ms
`2f079a1`	296.91 ms	337.43 ms	40.51 ms

App size

Revision	Plain	With Sentry	Diff
`1e4690d`	1.74 MiB	2.33 MiB	604.92 KiB
`3d89dea`	1.74 MiB	2.33 MiB	604.92 KiB
`7300956`	1.73 MiB	2.29 MiB	578.69 KiB
`c5ccd8a`	1.74 MiB	2.33 MiB	607.44 KiB
`3d89dea`	1.74 MiB	2.33 MiB	604.92 KiB
`7300956`	1.73 MiB	2.29 MiB	578.69 KiB
`2f079a1`	1.74 MiB	2.33 MiB	605.56 KiB

Previous results on branch: feat/custom-measurements

Startup times

Revision	Plain	With Sentry	Diff
`2a895ac`	314.41 ms	356.27 ms	41.86 ms
`6f273ba`	317.75 ms	323.18 ms	5.43 ms
`1554df9`	319.72 ms	410.52 ms	90.80 ms
`c55413e`	296.53 ms	329.29 ms	32.76 ms
`aa54cc6`	353.30 ms	366.80 ms	13.50 ms
`7b304b0`	261.60 ms	314.70 ms	53.10 ms
`cb34c81`	329.60 ms	335.20 ms	5.60 ms
`246d07b`	297.00 ms	333.06 ms	36.06 ms
`2fda34d`	290.53 ms	341.90 ms	51.37 ms
`1a7023b`	378.96 ms	417.00 ms	38.04 ms

App size

Revision	Plain	With Sentry	Diff
`2a895ac`	1.74 MiB	2.33 MiB	608.07 KiB
`6f273ba`	1.73 MiB	2.29 MiB	579.42 KiB
`1554df9`	1.73 MiB	2.29 MiB	579.18 KiB
`c55413e`	1.74 MiB	2.33 MiB	608.07 KiB
`aa54cc6`	1.73 MiB	2.29 MiB	579.42 KiB
`7b304b0`	1.73 MiB	2.29 MiB	579.43 KiB
`cb34c81`	1.74 MiB	2.33 MiB	608.07 KiB
`246d07b`	1.74 MiB	2.33 MiB	608.06 KiB
`2fda34d`	1.74 MiB	2.33 MiB	608.07 KiB
`1a7023b`	1.74 MiB	2.33 MiB	608.06 KiB

codecov-commenter · 2022-09-28T10:14:05Z

Codecov Report

Base: 80.07% // Head: 80.10% // Increases project coverage by +0.03% 🎉

Coverage data is based on head (16b9489) compared to base (7300956).
Patch coverage: 83.33% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #2260      +/-   ##
============================================
+ Coverage     80.07%   80.10%   +0.03%     
- Complexity     3415     3423       +8     
============================================
  Files           241      242       +1     
  Lines         12636    12692      +56     
  Branches       1697     1698       +1     
============================================
+ Hits          10118    10167      +49     
- Misses         1876     1882       +6     
- Partials        642      643       +1

Impacted Files	Coverage Δ
sentry/src/main/java/io/sentry/NoOpSpan.java	`23.07% <0.00%> (-1.93%)`	⬇️
...entry/src/main/java/io/sentry/NoOpTransaction.java	`24.32% <0.00%> (-1.39%)`	⬇️
sentry/src/main/java/io/sentry/Span.java	`96.36% <0.00%> (-3.64%)`	⬇️
sentry/src/main/java/io/sentry/SentryTracer.java	`90.41% <81.81%> (-0.42%)`	⬇️
...src/main/java/io/sentry/SentryMeasurementUnit.java	`100.00% <100.00%> (ø)`
...main/java/io/sentry/protocol/MeasurementValue.java	`94.23% <100.00%> (+4.86%)`	⬆️
...ain/java/io/sentry/protocol/SentryTransaction.java	`90.29% <0.00%> (+0.74%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

adinauer

Mostly LGTM. Need to decide whether to set NONE as default in the SDK or not. If not there's some changes required.

romtsn · 2022-09-29T10:53:27Z

btw, I feel like we should change float to Number here, thoughts @adinauer @marandaneto ? Because if we keep float, then we'd send something like

"screen_load_count": {
	"value": 1.0,
	"unit": "day"
}

which in turn, would be displayed like this in the Sentry UI:

and I think it doesn't look nice.

Worth noting, that if we don't send any units at all, then the frontend does not format it, and it just looks fine:

adinauer · 2022-09-29T11:26:52Z

change float to Number

Sounds good 👍

philipphofmann

One important question about the API.

marandaneto · 2022-09-30T09:10:43Z

btw, I feel like we should change float to Number here, thoughts @adinauer @marandaneto ? Because if we keep float, then we'd send something like
"screen_load_count": {
	"value": 1.0,
	"unit": "day"
}
which in turn, would be displayed like this in the Sentry UI:

and I think it doesn't look nice.

Worth noting, that if we don't send any units at all, then the frontend does not format it, and it just looks fine:

it's a num in Dart, so it can be an int or double.

romtsn · 2022-09-30T15:43:42Z

+    PERCENT;
+  }
+
+  final class Custom implements SentryMeasurementUnit {


I'm not entirely sure about this, would require creating a new class instance for every custom unit. If it was Kotlin, I'd have used inline classes, problem solved.

In theory we could provide one more setMeasurement overload which accepts a plain String, but also not sure if it's better.

I'd keep it as it is, Relay does the same.
Some languages don't have overloads, so at least we keep it unified.

I'm also fine with this API. I think we should add some code comments, please.

what kind of comments would you like to add? There's a link to the develop docs explaining every unit though

I've added some comments, mainly deriving from the Relay docs, lemme know if that works.

romtsn · 2022-09-30T15:52:15Z

@adinauer @philipphofmann @marandaneto ready for review once again

romtsn · 2022-09-30T19:34:35Z

@vaind looks like I made app with sentry start up faster than without sentry 🤣

vaind · 2022-10-03T08:25:21Z

@vaind looks like I made app with sentry start up faster than without sentry 🤣

🥇

I should probably make it re-run the whole suite in this case.

marandaneto · 2022-10-03T11:12:29Z

@adinauer @philipphofmann @marandaneto ready for review once again

Feature-wise all good.
Good catch with the missing units.
I'd leave your peers to do the final review.

philipphofmann

Almost LGTM.

philipphofmann · 2022-10-03T14:29:45Z

+    PERCENT;
+  }
+
+  final class Custom implements SentryMeasurementUnit {


I'm also fine with this API. I think we should add some code comments, please.

philipphofmann

LGTM, thanks @romtsn, and @adinauer.

adinauer

LGTM, just two more questions around NONE and marking as internal.

romtsn · 2022-10-04T19:34:26Z

Thanks everyone for the reviews and opinions 🎉

Add custom measurements

5a44c48

adinauer requested review from romtsn and stefanosiano as code owners September 27, 2022 14:19

marandaneto reviewed Sep 28, 2022

View reviewed changes

Comment thread sentry/src/main/java/io/sentry/SentryMeasurementUnit.java Outdated

Changelog

dd3a183

Add tests

0d10f60

romtsn mentioned this pull request Sep 28, 2022

Custom measurements API getsentry/team-mobile#51

Closed

5 tasks

romtsn reviewed Sep 28, 2022

View reviewed changes

Comment thread sentry/src/test/java/io/sentry/SentryTracerTest.kt

romtsn added 2 commits September 28, 2022 20:27

Update sentry/src/test/java/io/sentry/SentryTracerTest.kt

018e0db

Add example custom metrics to samples

f765bcf

marandaneto reviewed Sep 29, 2022

View reviewed changes

Comment thread sentry/src/main/java/io/sentry/SentryTracer.java Outdated

adinauer commented Sep 29, 2022

View reviewed changes

romtsn changed the title ~~Add custom measurements~~ Provide API for attaching custom measurements to transactions Sep 29, 2022

Address PR reviews

2d60a97

philipphofmann reviewed Sep 30, 2022

View reviewed changes

Comment thread CHANGELOG.md Outdated

Comment thread sentry/src/main/java/io/sentry/SentryMeasurementUnit.java Outdated

Comment thread sentry/src/main/java/io/sentry/SentryMeasurementUnit.java Outdated

Add missing units

4c66b5b

marandaneto mentioned this pull request Sep 30, 2022

Add missing measurements units getsentry/sentry-dart#1033

Merged

5 tasks

philipphofmann reviewed Sep 30, 2022

View reviewed changes

Comment thread sentry/src/main/java/io/sentry/SentryMeasurementUnit.java Outdated

romtsn added 3 commits September 30, 2022 15:57

Add missing units

c944154

Convert float to Number for MeasurementValue

bc3c3b3

Merge branch 'main' into feat/custom-measurements

52fe355

romtsn reviewed Sep 30, 2022

View reviewed changes

Format code

63b3d70

romtsn added 2 commits September 30, 2022 17:57

api dump

330c28a

Fix test

16b9489

marandaneto reviewed Oct 3, 2022

View reviewed changes

Comment thread sentry-android-core/src/main/java/io/sentry/android/core/ActivityFramesTracker.java Outdated

philipphofmann reviewed Oct 3, 2022

View reviewed changes

romtsn and others added 6 commits October 3, 2022 20:10

SentryMeasurementUnit -> MeasurementUnit

04f3811

Add missing test

45265a5

Add comments

03f4f92

Add NONE unit back

ea1ef57

api dump

62809fb

Format code

290a86f

marandaneto reviewed Oct 4, 2022

View reviewed changes

Comment thread sentry/src/main/java/io/sentry/protocol/MeasurementValue.java

philipphofmann approved these changes Oct 4, 2022

View reviewed changes

Comment thread sentry/src/test/java/io/sentry/SentryTracerTest.kt

adinauer commented Oct 4, 2022

View reviewed changes

Comment thread sentry/src/main/java/io/sentry/MeasurementUnit.java Outdated

Comment thread sentry/src/main/java/io/sentry/protocol/MeasurementValue.java

romtsn and others added 6 commits October 4, 2022 16:15

Merge branch 'main' into feat/custom-measurements

0a0929a

Add @internal annotation to NONE unit

0d2e6b1

Format code

4e4598f

Changelog

68519fd

Changelog

457b04a

Fix test

f1747c8

romtsn merged commit 54cebc8 into main Oct 4, 2022

romtsn deleted the feat/custom-measurements branch October 4, 2022 19:34

markushi mentioned this pull request Oct 5, 2022

feat(java): Add automatic and custom performance metrics doc getsentry/sentry-docs#5604

Merged

Uh oh!

Uh oh!

Conversation

adinauer commented Sep 27, 2022 • edited by romtsn Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

Uh oh!

github-actions Bot commented Sep 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Sep 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Performance metrics 🚀

Baseline results on branch: main

Startup times

App size

Previous results on branch: feat/custom-measurements

Startup times

App size

Uh oh!

Uh oh!

codecov-commenter commented Sep 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

adinauer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

romtsn commented Sep 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adinauer commented Sep 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

philipphofmann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marandaneto commented Sep 30, 2022

Uh oh!

Uh oh!

romtsn Sep 30, 2022

Choose a reason for hiding this comment

Uh oh!

marandaneto Oct 3, 2022

Choose a reason for hiding this comment

Uh oh!

philipphofmann Oct 3, 2022

Choose a reason for hiding this comment

Uh oh!

romtsn Oct 3, 2022

Choose a reason for hiding this comment

Uh oh!

romtsn Oct 3, 2022

Choose a reason for hiding this comment

Uh oh!

romtsn commented Sep 30, 2022

Uh oh!

romtsn commented Sep 30, 2022

Uh oh!

vaind commented Oct 3, 2022

Uh oh!

Uh oh!

marandaneto commented Oct 3, 2022

Uh oh!

philipphofmann left a comment

Choose a reason for hiding this comment

adinauer commented Sep 27, 2022 •

edited by romtsn

Loading

github-actions Bot commented Sep 27, 2022 •

edited

Loading

github-actions Bot commented Sep 27, 2022 •

edited

Loading

codecov-commenter commented Sep 28, 2022 •

edited

Loading

romtsn commented Sep 29, 2022 •

edited

Loading

adinauer commented Sep 29, 2022 •

edited

Loading

philipphofmann left a comment •

edited

Loading