Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: Custom measurements API #2268
feat: Custom measurements API #2268
Changes from 1 commit
febcbbb
4ae6b3f
9d1c9b2
e424daf
93cd097
9fc5b73
9a678ff
0f7a782
3b4ba4e
98964e0
98992d9
9175ae1
d857a63
b98b249
6eceb69
c478807
1ac57ea
7a4bc23
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will add comments for sure. Thanks for pointing that out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 aunit
of the typeString
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we said we wouldn't validate sdk side, so it's just a string in most sdks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea that's fine, i think i added type hints to python as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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