feat: Custom measurements API #2268

philipphofmann · 2022-10-07T09:09:10Z

📜 Description

Draft PR for getting feedback on the API. In ObjC, there is no inheritance for enums, nor can they implement protocols. Therefore I created multiple enums for duration, information, and fraction. For custom measurements, I added an overload to simply pass in a string, and for none, you just don't pass in any unit.

transaction.setMeasurement(name: "custom", value: 44, customUnit: "custom-unit")
transaction.setMeasurement(name: "none", value: 55)
transaction.setMeasurement(name: "duration", value: 1.23, durationUnit: .milliSecond)
transaction.setMeasurement(name: "information", value: 134.3, informationUnit: .gigabyte)
transaction.setMeasurement(name: "fraction", value: 9.3, fractionUnit: .percent)

Update 1

Made API similar to the NSMeasurement API.

transaction.setMeasurement(name: "duration", value: 44, unit: MeasurementUnitDuration.nanoseconds)
transaction.setMeasurement(name: "information", value: 44, unit: MeasurementUnitInformation.bit)
transaction.setMeasurement(name: "duration-custom", value: 22, unit: MeasurementUnit(symbol: "custom"))
transaction.setMeasurement(name: "none", value: 11)

Docs PR getsentry/sentry-docs#5629

💡 Motivation and Context

See getsentry/team-mobile#51

💚 How did you test it?

Unit tests, and with the iOS-Swift sample project.

📝 Checklist

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

🔮 Next steps

github-actions · 2022-10-07T09:10:25Z

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

Generated by 🚫 dangerJS against 7a4bc23

adinauer

LGTM only one question regarding protocol comment.

adinauer · 2022-10-07T09:18:43Z

Sources/Sentry/Public/SentrySpanProtocol.h

@@ -87,6 +87,28 @@ NS_SWIFT_NAME(Span)
 */
 - (void)removeTagForKey:(NSString *)key NS_SWIFT_NAME(removeTag(key:));

+- (void)setMeasurement:(NSString *)name


Should we add some comments briefly explaining which units can be used with which overload or do we expect people to simply drill down on the enum they can pass?

I will add comments for sure. Thanks for pointing that out.

github-actions · 2022-10-07T09:23:43Z

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	1263.17 ms	1275.82 ms	12.65 ms
Size	20.50 KiB	335.95 KiB	315.44 KiB

Baseline results on branch: master

Startup times

Revision	Plain	With Sentry	Diff
`5025d2e`	1245.14 ms	1268.58 ms	23.44 ms
`e958899`	1230.40 ms	1248.31 ms	17.91 ms
`e43ce74`	1235.77 ms	1252.06 ms	16.29 ms
`9231f60`	1256.78 ms	1267.74 ms	10.96 ms
`b869536`	1250.37 ms	1274.84 ms	24.47 ms
`fbeb49a`	1218.13 ms	1243.70 ms	25.57 ms
`8b040e4`	1234.76 ms	1244.71 ms	9.95 ms
`0e2e593`	1234.78 ms	1266.10 ms	31.32 ms
`7977992`	1219.80 ms	1241.92 ms	22.12 ms
`c61d869`	1255.92 ms	1267.47 ms	11.55 ms

App size

Revision	Plain	With Sentry	Diff
`5025d2e`	20.51 KiB	331.79 KiB	311.28 KiB
`e958899`	20.51 KiB	331.92 KiB	311.41 KiB
`e43ce74`	20.51 KiB	335.49 KiB	314.99 KiB
`9231f60`	20.51 KiB	333.58 KiB	313.07 KiB
`b869536`	20.51 KiB	331.79 KiB	311.28 KiB
`fbeb49a`	20.51 KiB	333.51 KiB	313.00 KiB
`8b040e4`	20.50 KiB	333.54 KiB	313.04 KiB
`0e2e593`	20.50 KiB	333.88 KiB	313.38 KiB
`7977992`	20.50 KiB	333.58 KiB	313.07 KiB
`c61d869`	20.51 KiB	333.10 KiB	312.59 KiB

Previous results on branch: feat/custom-measurements

Startup times

Revision	Plain	With Sentry	Diff
`a2653d7`	1198.12 ms	1233.30 ms	35.18 ms
`7d333b1`	1245.63 ms	1247.41 ms	1.78 ms
`a33cff7`	1251.44 ms	1265.33 ms	13.89 ms
`d4d9d7c`	1231.90 ms	1242.84 ms	10.94 ms
`fdbae9f`	1211.30 ms	1250.53 ms	39.23 ms
`3274774`	1257.63 ms	1268.62 ms	10.99 ms
`7418308`	1257.86 ms	1271.52 ms	13.66 ms
`807f9a2`	1261.98 ms	1268.55 ms	6.57 ms
`36778e0`	1217.04 ms	1243.45 ms	26.41 ms
`1514f97`	1240.66 ms	1256.78 ms	16.12 ms

App size

Revision	Plain	With Sentry	Diff
`a2653d7`	20.50 KiB	335.88 KiB	315.38 KiB
`7d333b1`	20.50 KiB	335.88 KiB	315.38 KiB
`a33cff7`	20.50 KiB	334.71 KiB	314.21 KiB
`d4d9d7c`	20.50 KiB	335.89 KiB	315.38 KiB
`fdbae9f`	20.50 KiB	335.93 KiB	315.43 KiB
`3274774`	20.50 KiB	335.89 KiB	315.38 KiB
`7418308`	20.50 KiB	335.21 KiB	314.70 KiB
`807f9a2`	20.50 KiB	335.88 KiB	315.38 KiB
`36778e0`	20.50 KiB	335.93 KiB	315.43 KiB
`1514f97`	20.50 KiB	335.85 KiB	315.35 KiB

marandaneto · 2022-10-07T09:46:04Z

Sources/Sentry/Public/SentrySpanProtocol.h

@@ -87,6 +87,28 @@ NS_SWIFT_NAME(Span)
 */
 - (void)removeTagForKey:(NSString *)key NS_SWIFT_NAME(removeTag(key:));

+- (void)setMeasurement:(NSString *)name


So apparently ObjC has the same issues as Dart, on Dart, we've used a single enum for now (with all the units), and will refactor later once we are able to upgrade to the latest version.
Here we have an overload per unit type, not sure if it's good or bad, maybe just a method that takes a String would be enough.
Dart does not have overloads, so I could not do this either.
Maybe drop a message to folks that were part of the setMeasurement API and see if they are fine with that due to the restrictions of the language?

The trade-off here is:
Exposing many public APIs, maybe more in the future, so it's overwhelming for the user.

Or a single setMeasurement method that takes a unit of the type String which is open and users would need to learn all the unit types thru the docs.
@sl0thentr0py and @jan-auer specifically were driver and approver.

we said we wouldn't validate sdk side, so it's just a string in most sdks.

Yep, but the implementation should be UX friendly, at least for strongly typed languages, an enum would be ideal, but one can set a string as well.
On java one can do:

transaction.setMeasurement("metric1", 2, MeasurementUnit.Duration.DAY) transaction.setMeasurement("metric1", 2, MeasurementUnit.Custom("myOwnUnit"))

yea that's fine, i think i added type hints to python as well

sry forgot to follow up earlier, was on phone.
I have these type hints in python
https://github.com/getsentry/sentry-python/blob/f71a8f45e780525e52fa5868f45bb876dcf0994b/sentry_sdk/_types.py#L53-L82

Sources/Sentry/SentryTracer.m

marandaneto · 2022-10-07T09:58:39Z

Consider using this new API when adding app start and slow/frozen frame measurements instead of addMeasurements in the SentryTracer directly.

brustolin

I know that I said Im ok with the overloading approach, and it is indeed ok-ish, however, ideas mature like good wine, with time.

This adds too much responsibility to SentrySpan.
What do you think about having a Measurement class? Then Sentry span only need ‘addMeasurement:’. or 'AddMeasurement:withName:' in case we dont think the name belongs to the entity.

This Measurement class may have as many constructors as types of enums, and be responsible for its own serialization. If we add overloads to SentrySpan protocol we need to update 3 different classes (SentrySpan, SentryTracer, SentryNoOpSpan) for every new type of unit.

Sources/Sentry/Public/SentryDefines.h

brustolin

This is top engineering. 😉
IMO having both SentryMeasurement and SentryMeasurementUnit is an overkill, but that is ok.

Sources/Sentry/Public/SentryMeasurementUnit.h

Sources/Sentry/Public/SentrySpanProtocol.h

Sources/Sentry/SentryMeasurementUnit.m

fix this in another PR

Only merge after releasing Cocoa 7.28.0. Related Cocoa PR getsentry/sentry-cocoa#2268.

brustolin

Very solid PR!! 🚀
Congrats!!!

kevinrenskers · 2022-10-11T18:24:21Z

Can we not use the same measurement units for app/device context? We send free/total memory and storage for example, and the question has come up multiple time "but what is the unit". This could be a nice way to do that?

philipphofmann · 2022-10-12T06:03:20Z

Can we not use the same measurement units for app/device context? We send free/total memory and storage for example, and the question has come up multiple time "but what is the unit". This could be a nice way to do that?

That's a good point, @kevinrenskers. We would need to extend the protocol for that. I'm going to start a discussion internally and keep you posted.

armcknight

Is one goal here to be able to interop with Apple's NSUnit/NSMeasurement APIs? If that's the case, we could consider using those definitions directly and writing a polyfill for iOS <13, and provide categories to help with serialization, instead of recreating it all.

If it's simply to model our API off of Apple's... I'm not a huge fan of their way of describing units and values 🙃 I don't think the class properties are very intuitive. It's basically an ObjC approximation of uninhabited enums, but this isn't really the use case for those, since we have basis for comparing/converting between different cases like bytes and kilobytes. Uninhabited enums are more for grouping related constants that aren't convertible. You don't get the benefit of switch exhaustibility checks with them.

It's also not performant to allocate so many objects to represent the difference between a megabyte vs a kilobyte. I would just use an integer-backed enum and a function to convert to a string for serialization into envelopes. More performant, and you can use switch again.

armcknight · 2022-10-12T04:39:57Z

Sources/Sentry/Public/SentryMeasurementUnit.h

+NS_ASSUME_NONNULL_BEGIN
+
+NS_SWIFT_NAME(MeasurementUnit)
+@interface SentryMeasurementUnit : NSObject <NSCopying>


Some explanatory headerdoc would be helpful for these class definitions.

Sources/Sentry/Public/SentryMeasurementUnit.h

armcknight · 2022-10-12T04:52:37Z

Sources/Sentry/include/SentryMeasurementValue.h

+- (instancetype)initWithValue:(NSNumber *)value unit:(SentryMeasurementUnit *)unit;
+
+@property (nonatomic, copy, readonly) NSNumber *value;
+@property (nullable, readonly, copy) SentryMeasurementUnit *unit;


What would it mean for this to be nil? For custom units? I saw we have some initializer that accepts an arbitrary string, so I figured that would be used for this, so I just wasn't sure what nil represents here.

It means this will be handled server side.

Sources/Sentry/Public/SentryOptions.h

Sources/Sentry/Public/SentryMeasurementUnit.h

philipphofmann · 2022-10-12T07:01:05Z

Is one goal here to be able to interop with Apple's NSUnit/NSMeasurement APIs? If that's the case, we could consider using those definitions directly and writing a polyfill for iOS <13, and provide categories to help with serialization, instead of recreating it all.

No the goal is not to interop with them.

If it's simply to model our API off of Apple's... I'm not a huge fan of their way of describing units and values 🙃 I don't think the class properties are very intuitive. It's basically an ObjC approximation of uninhabited enums, but this isn't really the use case for those, since we have basis for comparing/converting between different cases like bytes and kilobytes. Uninhabited enums are more for grouping related constants that aren't convertible. You don't get the benefit of switch exhaustibility checks with them.

I'm also not a huge fan the Apple APIs. Initially, I had different enums with method overloads, see my first commit febcbbb, but @marandaneto had concerns about it. Ideally, it would be nice to do something like MeasurementUnit.Duration.DAY, but I didn't find a proper way to achieve this. We could also do overloads with NS_TYPED_ENUM.

It's also not performant to allocate so many objects to represent the difference between a megabyte vs a kilobyte. I would just use an integer-backed enum and a function to convert to a string for serialization into envelopes. More performant, and you can use switch again.

I don't expect people to use this API heavily, so it'd be OK with a couple of extra objects.

philipphofmann · 2022-10-12T08:12:26Z

Can we not use the same measurement units for app/device context? We send free/total memory and storage for example, and the question has come up multiple time "but what is the unit". This could be a nice way to do that?

That's a good point, @kevinrenskers. We would need to extend the protocol for that. I'm going to start a discussion internally and keep you posted.

@kevinrenskers, FYI: The context is usually set by us, and expects certain units explicitly (as is in the develop spec).
It would be hard to make this backwards compat unless it means we explicitly add new unit fields for every context item appropriately, which could balloon the payload a lot.

kevinrenskers · 2022-10-12T08:13:28Z

Ok, that makes sense.

* master: feat: Custom measurements API (#2268) ref: Don't only update App State in the OOM tracker (#2276) test: generate profiles with different levels of efficiency (#2274) test: disable testStartUpCrash_CallsFlush as it flakes (#2275) build(deps): bump github/codeql-action from 2.1.26 to 2.1.27 (#2272) ref: Use the new loadPreviousAppState in SentryAppStartTracker (#2261) test: use Test/TestCI configs for fastfile test runs (#2257)

feat: Custom measurements API

febcbbb

philipphofmann requested review from marandaneto and adinauer October 7, 2022 09:13

adinauer approved these changes Oct 7, 2022

View reviewed changes

marandaneto reviewed Oct 7, 2022

View reviewed changes

This was referenced Oct 7, 2022

Custom measurements API getsentry/team-mobile#51

Closed

Support measurement.unit getsentry/team-mobile#31

Closed

marandaneto reviewed Oct 7, 2022

View reviewed changes

Sources/Sentry/SentryTracer.m Outdated Show resolved Hide resolved

brustolin reviewed Oct 7, 2022

View reviewed changes

Sources/Sentry/SentryTracer.m Outdated Show resolved Hide resolved

brustolin reviewed Oct 7, 2022

View reviewed changes

Make API similar to NSMEasurement

4ae6b3f

philipphofmann requested a review from brustolin October 10, 2022 09:48

philipphofmann commented Oct 10, 2022

View reviewed changes

Sources/Sentry/Public/SentryDefines.h Outdated Show resolved Hide resolved

philipphofmann added 4 commits October 10, 2022 11:54

Rename symbol to unit

9d1c9b2

Simplify API

e424daf

Add missing units

93cd097

Rename to SentryMeasurmentUnit

9fc5b73

brustolin reviewed Oct 11, 2022

View reviewed changes

Sources/Sentry/Public/SentryMeasurementUnit.h Show resolved Hide resolved

Sources/Sentry/Public/SentryMeasurementUnit.h Show resolved Hide resolved

Sources/Sentry/Public/SentrySpanProtocol.h Show resolved Hide resolved

Sources/Sentry/SentryMeasurementUnit.m Outdated Show resolved Hide resolved

philipphofmann added 8 commits October 11, 2022 09:54

Refactoring

9a678ff

Rename SentryMeasurement to SentryMeasurementValue

0f7a782

Add TracerTests

3b4ba4e

Changelog

98964e0

Undo SENTRY_NO_INIT for SentryTracer

98992d9

fix this in another PR

Add code docs

9175ae1

Remove not needed casting

d857a63

One more

b98b249

philipphofmann self-assigned this Oct 11, 2022

philipphofmann marked this pull request as ready for review October 11, 2022 09:36

philipphofmann requested a review from armcknight as a code owner October 11, 2022 09:36

philipphofmann added a commit to getsentry/sentry-docs that referenced this pull request Oct 11, 2022

feat(apple): Add custom performance metrics

f668ad5

Only merge after releasing Cocoa 7.28.0. Related Cocoa PR getsentry/sentry-cocoa#2268.

philipphofmann mentioned this pull request Oct 11, 2022

feat(apple): Add custom performance metrics getsentry/sentry-docs#5629

Merged

philipphofmann requested a review from brustolin October 11, 2022 12:12

brustolin approved these changes Oct 11, 2022

View reviewed changes

philipphofmann requested a review from marandaneto October 11, 2022 14:20

armcknight reviewed Oct 12, 2022

View reviewed changes

Remove setMeasurements from SentryOptions

6eceb69

philipphofmann added 3 commits October 13, 2022 08:58

Add tests and docs for measurement unit

c478807

Add link to docs in changelog

1ac57ea

Merge branch 'master' into feat/custom-measurements

7a4bc23

philipphofmann merged commit 372ffbe into master Oct 13, 2022

philipphofmann deleted the feat/custom-measurements branch October 13, 2022 12:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Custom measurements API #2268

feat: Custom measurements API #2268

philipphofmann commented Oct 7, 2022 •

edited

Loading

github-actions bot commented Oct 7, 2022 •

edited

Loading

adinauer left a comment

adinauer Oct 7, 2022

philipphofmann Oct 7, 2022

github-actions bot commented Oct 7, 2022 •

edited

Loading

Baseline results on branch: master

Startup times

App size

Previous results on branch: feat/custom-measurements

Startup times

App size

marandaneto Oct 7, 2022 •

edited

Loading

sl0thentr0py Oct 7, 2022

marandaneto Oct 7, 2022

sl0thentr0py Oct 7, 2022

sl0thentr0py Oct 7, 2022

marandaneto commented Oct 7, 2022 •

edited

Loading

brustolin left a comment

brustolin left a comment

brustolin left a comment

kevinrenskers commented Oct 11, 2022

philipphofmann commented Oct 12, 2022

armcknight left a comment

armcknight Oct 12, 2022

armcknight Oct 12, 2022

brustolin Oct 12, 2022

philipphofmann commented Oct 12, 2022

philipphofmann commented Oct 12, 2022

kevinrenskers commented Oct 12, 2022

feat: Custom measurements API #2268

feat: Custom measurements API #2268

Conversation

philipphofmann commented Oct 7, 2022 • edited Loading

📜 Description

Update 1

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

github-actions bot commented Oct 7, 2022 • edited Loading

adinauer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 7, 2022 • edited Loading

Performance metrics 🚀

Baseline results on branch: master

Startup times

App size

Previous results on branch: feat/custom-measurements

Startup times

App size

marandaneto Oct 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marandaneto commented Oct 7, 2022 • edited Loading

brustolin left a comment

Choose a reason for hiding this comment

brustolin left a comment

Choose a reason for hiding this comment

brustolin left a comment

Choose a reason for hiding this comment

kevinrenskers commented Oct 11, 2022

philipphofmann commented Oct 12, 2022

armcknight left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philipphofmann commented Oct 12, 2022

philipphofmann commented Oct 12, 2022

kevinrenskers commented Oct 12, 2022

philipphofmann commented Oct 7, 2022 •

edited

Loading

github-actions bot commented Oct 7, 2022 •

edited

Loading

github-actions bot commented Oct 7, 2022 •

edited

Loading

marandaneto Oct 7, 2022 •

edited

Loading

marandaneto commented Oct 7, 2022 •

edited

Loading