-
Notifications
You must be signed in to change notification settings - Fork 61
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
Create internal API for sending technical performance metrics #1083
Conversation
@@ -869,3 +905,11 @@ internal open class RumViewScope( | |||
} | |||
} | |||
} | |||
|
|||
private fun VitalInfo.toPerformanceMetric(): ViewEvent.FlutterBuildTime { |
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.
Having one specific type that is FlutterBuildTime
might be harder to change later. Maybe we can create a dedicated type for this to be more generic?
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.
This came from the generator, I think it just chose the first property that used the type? @0xnm do we maybe want to adjust the generator somehow?
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'm not quite sure what is the concern here. Can you please elaborate?
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.
Basically just that "FlutterBuildTime" is used as the type for things that are not FlutterBuildTime. But that type came from the schema generator so I just rolled with it.
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.
Well, this is how it is called in the schema. If it needs to have more generic name, the property should have generic name as well. But I don't see the issue here, because once we add a property for JS, it will get its own type. Types are basically not reused.
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.
In the generator we have a way to reuse types when they have the same schema. I'll create a task to try and find a better way to name types
This creates an internal API for adding cross platform technical performance metrics (currently only Flutter Build Time, Flutter Raster Time and JS Refresh Rate). Each update is a single sample, which is aggregated via the same method as the Vitals aggregation, and posted to View events. Fixes RUMM-2515
a057ee2
to
448d208
Compare
@@ -869,3 +905,11 @@ internal open class RumViewScope( | |||
} | |||
} | |||
} | |||
|
|||
private fun VitalInfo.toPerformanceMetric(): ViewEvent.FlutterBuildTime { |
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.
In the generator we have a way to reuse types when they have the same schema. I'll create a task to try and find a better way to name types
448d208
to
daf74d6
Compare
What does this PR do?
This creates an internal API for adding cross platform technical performance metrics (currently only Flutter Build Time, Flutter Raster Time and JS Refresh Rate). Each update is a single sample, which is aggregated via the same method as the Vitals aggregation, and posted to View events.
Motivation
The Flutter and React Native SDKs will use this method to send in their performance metrics. The API was left open to potentially support sending different metrics in the future just by adding to the enum and serializing to the proper property on the View event.
Additional Notes
Anything else we should know when reviewing?
Review checklist (to be filled by reviewers)