Add analytics to performance and cpu profiler screens #3281
Conversation
| void timing( | ||
| String screenName, | ||
| String timedOperation, { | ||
| @required Duration duration, |
There was a problem hiding this comment.
Comment to clarify that duration should always refers to time DevTools took not time an app took
I would suggest creating a DevToolsScreen metrics objects that groups
cpuSampleCount, cpuStackDepth, and traceEventCount. Those are very performance page specific so it is a bit surprising to see them in this method. Keeping them separate will make the important parts of this API clearer and make it easier if you chose to refactor how those metrics are exposed. Internally it could be just a key value pair map or something.
I would also suggest a timing api that is more similar to what dart:developer does. So for example:
AnalyticsTimeline.timeSync("screenName", "someTimedOperation", () {
doSomething();
}, ScreenMetrics(cpuSampleCount: 1000, inspectorTreeSize: 100000)
);https://api.dart.cn/stable/2.13.4/dart-developer/Timeline-class.html
I'm less sure about the async version of the API as the one in dart:developer seems like it was written before Dart supported async. Something like this might make sense:
AnalyticsTimeline.timeAsync("screenName", "someTimedOperation", someFutureThatCompleteAtTheEndTime, ScreenMetrics(cpuSampleCount: 1000, inspectorTreeSize: 100000)
);or
AnalyticsTimeline.timeAsync("screenName", "someTimedOperation",
() async { someAsyncCodeThatTakesTime },
ScreenMetrics(cpuSampleCount: 1000, inspectorTreeSize: 100000)
);which would be cleaner for one of the examples further down in this CL.
There was a problem hiding this comment.
did some significant refactoring - PTAL
| int uiDuration, // Custom metric | ||
| ]) { | ||
| String screenName, { | ||
| @required Duration rasterDuration, // Custom metric |
There was a problem hiding this comment.
I would omit these custom metric comments. not clear what they are communicating.
| devtools_version: devtoolsVersion, | ||
| ide_launched: ideLaunched, | ||
| flutter_client_id: flutterClientId, | ||
| ui_duration_micros: screenMetrics is PerformanceScreenMetrics |
There was a problem hiding this comment.
having is checks for subtypes isn't very robust. Can you instead switch to adding a toJson() or similar method in the ScreenAnalyticsMetrics subclasses that surfaces the custom metrics each screen cares about?
Will requiring tweaking GtagEventDevTools to take a JSON map of extra parameters rather than just a fixed list of fields but probably worth it to keep adding extra metrics simple.
There was a problem hiding this comment.
To land this now add a TODO here and a comment in the ScreenAnalyticsMetrics base class clarifying that extending this class is fragile.
There was a problem hiding this comment.
I thought about this. Since the GtagEventDevTools is an external factory constructor, I'm not sure how passing json in this would affect the custom dimension setup in JS.
I will go ahead and add a TODO to consider trying to make this take a json map in the future, but for now I will add better documentation.
| // Code in this file should be able to be imported by both dart:html and | ||
| // dart:io dependent libraries. | ||
|
|
||
| /// Base class for all screen metrics classes. |
There was a problem hiding this comment.
Describe what is involved in creating a subclass or switch to a
toJson() based scheme so that it isn't magical.
This PR also
Work towards #3270