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

Create internal API for sending technical performance metrics #1083

Merged
merged 3 commits into from
Oct 14, 2022

Conversation

fuzzybinary
Copy link
Member

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)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • Make sure you discussed the feature or bugfix with the maintaining team in an Issue
  • Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)

@fuzzybinary fuzzybinary requested a review from a team as a code owner October 12, 2022 21:07
@xgouchet xgouchet added the size-medium This PR is medium sized label Oct 12, 2022
@@ -869,3 +905,11 @@ internal open class RumViewScope(
}
}
}

private fun VitalInfo.toPerformanceMetric(): ViewEvent.FlutterBuildTime {
Copy link
Contributor

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?

Copy link
Member Author

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?

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 quite sure what is the concern here. Can you please elaborate?

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member

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
@fuzzybinary fuzzybinary force-pushed the jward/RUMM-2515-cp-perf-metrics branch 3 times, most recently from a057ee2 to 448d208 Compare October 13, 2022 19:12
@@ -869,3 +905,11 @@ internal open class RumViewScope(
}
}
}

private fun VitalInfo.toPerformanceMetric(): ViewEvent.FlutterBuildTime {
Copy link
Member

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

@fuzzybinary fuzzybinary force-pushed the jward/RUMM-2515-cp-perf-metrics branch from 448d208 to daf74d6 Compare October 14, 2022 13:20
@fuzzybinary fuzzybinary merged commit 4cdbce3 into develop Oct 14, 2022
@fuzzybinary fuzzybinary deleted the jward/RUMM-2515-cp-perf-metrics branch October 14, 2022 17:50
@xgouchet xgouchet added this to the 1.15.0 milestone Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size-medium This PR is medium sized
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants