Report commands that resulted in success or failure #34288
Conversation
1b627fd to
cacfd98
Compare
cacfd98 to
281bc6d
Compare
| @@ -316,10 +315,7 @@ class AttachCommand extends FlutterCommand { | |||
| result = await runner.attach(); | |||
| assert(result != null); | |||
| } | |||
| if (result == 0) { | |||
There was a problem hiding this comment.
As a meta-point (this might have been there from before), have we been splitting our flutter attach analytics volume between flutterUsage.sendEvent/events and flutterUsage.sendCommand/screen?
If so, we might want to consolidate them in one place and make sure the go/flutter-dash graph reflects it.
There was a problem hiding this comment.
I saw the comment. I agree that given the flutterUsage.sendEvent/events mechanism, it makes sense to add global annotations to all of them. I was asking about the semantics of FlutterCommands such as flutter attach that both send flutterUsage.sendCommand/screen in FlutterCommand.verifyThenRunCommand and a flutterUsage.sendEvent/events in FlutterCommand.run.
There was a bit of an existing pattern already going on where events are being used as smaller inner acts such as hot restarting and screens being used for bigger outer flutter CLI acts such as run, attach etc.
1- should we keep that semantic separated, and
2- should we also log success/failure in flutterUsage.sendCommand/screens as well?
There was a problem hiding this comment.
Sorry, in my comment in the other PR, I missed the sendCommand call in verifyThenRunCommand. If it makes more sense to add a success/failure parameter to the sendCommand there, then you could put the runCommand() in a try {} except {} finally {} with the sendCommand in the finally. Similar pattern as above in run().
52b495e to
bee864b
Compare
bee864b to
93e8464
Compare
This is added as a dimension cd26
Description
Success and failure events at the command level.
Related Issues
Fixes #33779
Tests
I added the following tests:
flutter_command_test.dartChecklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.///).flutter analyze --flutter-repo) does not report any problems on my PR.Breaking Change
Does your PR require Flutter developers to manually update their apps to accommodate your change?