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

Provide API for attaching custom measurements to transactions #2260

Merged
merged 25 commits into from
Oct 4, 2022

Conversation

adinauer
Copy link
Member

@adinauer adinauer commented Sep 27, 2022

📜 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
Copy link
Contributor

github-actions bot commented Sep 27, 2022

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

Generated by 🚫 dangerJS against f1747c8

@github-actions
Copy link
Contributor

github-actions bot commented Sep 27, 2022

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
Copy link

codecov-commenter commented Sep 28, 2022

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.

Copy link
Member Author

@adinauer adinauer left a comment

Choose a reason for hiding this comment

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

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

CHANGELOG.md Outdated Show resolved Hide resolved
sentry/src/main/java/io/sentry/ISpan.java Outdated Show resolved Hide resolved
sentry/src/main/java/io/sentry/SentryMeasurementUnit.java Outdated Show resolved Hide resolved
sentry/src/main/java/io/sentry/SentryTracer.java Outdated Show resolved Hide resolved
@romtsn romtsn changed the title Add custom measurements Provide API for attaching custom measurements to transactions Sep 29, 2022
@romtsn
Copy link
Member

romtsn commented Sep 29, 2022

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:
image

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:
image

@adinauer
Copy link
Member Author

adinauer commented Sep 29, 2022

change float to Number

Sounds good 👍

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

One important question about the API.

CHANGELOG.md Outdated Show resolved Hide resolved
sentry/src/main/java/io/sentry/SentryMeasurementUnit.java Outdated Show resolved Hide resolved
sentry/src/main/java/io/sentry/SentryMeasurementUnit.java Outdated Show resolved Hide resolved
@marandaneto
Copy link
Contributor

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: image

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: image

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

PERCENT;
}

final class Custom implements SentryMeasurementUnit {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

@romtsn
Copy link
Member

romtsn commented Sep 30, 2022

@adinauer @philipphofmann @marandaneto ready for review once again

@romtsn
Copy link
Member

romtsn commented Sep 30, 2022

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

@vaind
Copy link
Collaborator

vaind commented Oct 3, 2022

@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
Copy link
Contributor

@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.

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

Almost LGTM.

sentry/src/main/java/io/sentry/ISpan.java Outdated Show resolved Hide resolved
sentry/api/sentry.api Show resolved Hide resolved
PERCENT;
}

final class Custom implements SentryMeasurementUnit {
Copy link
Member

Choose a reason for hiding this comment

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

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

sentry/src/main/java/io/sentry/SentryMeasurementUnit.java Outdated Show resolved Hide resolved
sentry/src/main/java/io/sentry/ISpan.java Show resolved Hide resolved
Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @romtsn, and @adinauer.

Copy link
Member Author

@adinauer adinauer left a comment

Choose a reason for hiding this comment

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

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

@romtsn romtsn merged commit 54cebc8 into main Oct 4, 2022
@romtsn romtsn deleted the feat/custom-measurements branch October 4, 2022 19:34
@romtsn
Copy link
Member

romtsn commented Oct 4, 2022

Thanks everyone for the reviews and opinions 🎉

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.

Flaky SentryTracerTest
7 participants