-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Add commandHasTerminal parameter + apple usage event + sendException events for package:unified_analytics
#138806
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
Add commandHasTerminal parameter + apple usage event + sendException events for package:unified_analytics
#138806
Conversation
This update included updating the `Environment` class so it touches several files
|
@christopherfujino, this migration touched a lot more files than the previous PRs so to help make it easier i tried to migrate one file at a time in the commits if you wanted to review it step-by-step I'm also pretty sure i'll need to get a g3fix out to update the package, working on that atm EDIT: g3fix at cl/584355872 |
| label: buildSuccess ? 'success' : 'fail', | ||
| flutterUsage: environment.usage, | ||
| ).send(); | ||
| environment.analytics.send(Event.appleUsageEvent( |
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.
why is this called appleUsageEvent? or, why have a single enum for both ios and macos workflows?
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 made a call to combine the leftover UsageEvent calls, they were all ungrouped so it may have been the wrong call to combine them all under appleUsageEvent instead of separate iosUsageEvent and macosUsageEvent.
We spoke offline about this but if you think this won't scale well if we have additional apple related events, i can definitely revert the original PR in dart-lang/tools
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.
it's not a big deal, it just struck my eye as unusual. there has also been discussion about what to name the union of macOS and iOS in the codebase (many places in the tool we call it "Darwin", but others "Apple" ). let's leave this.
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.
gotcha sounds good! i'll keep this in mind for future work
| commandPath: commandPath, | ||
| result: commandResult.toString(), | ||
| maxRss: maxRss, | ||
| commandHasTerminal: globals.stdio.hasTerminal, |
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.
with package:unified_analytics, are we only tracking this on command results, or will we have that data with every event? I'm still unclear on dart-lang/tools#197 (comment)
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.
That's a good question, i was actually doing some digging and it doesn't seem like that boolean is getting sent with each event, only for the events that indicate which event was run.
It's not set at the session level
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.
Ahh, but was this the only context we were providing the custom dimension with the legacy analytics? https://github.com/flutter/flutter/blob/master/packages/flutter_tools/lib/src/runner/flutter_command.dart#L1740
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 believe so, I went through the code base to find any instances of the commandHasTerminal custom dimension. I also checked in the database for GA3 and didn't see it being sent with every event.
I believe the only two dimensions we send with every event is the channel and host os. Below is a snippet of the first record in the table
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.
SGTM
christopherfujino
left a comment
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.
LGTM
…dException` events for `package:unified_analytics` (flutter/flutter#138806)
…dException` events for `package:unified_analytics` (flutter/flutter#138806)
…dException` events for `package:unified_analytics` (flutter/flutter#138806)
…dException` events for `package:unified_analytics` (flutter/flutter#138806)
…dException` events for `package:unified_analytics` (flutter/flutter#138806)
…dException` events for `package:unified_analytics` (flutter/flutter#138806)
Manual roll requested by [email protected] flutter/flutter@ab721f9...14549b3 2023-11-22 [email protected] make FakeView not send Scene and semantics to the engine (flutter/flutter#138849) 2023-11-22 [email protected] Update the default outline color for `OutlinedButton` (flutter/flutter#138768) 2023-11-22 [email protected] Roll Flutter Engine from c954a64c509b to 342fb003b11f (1 revision) (flutter/flutter#138903) 2023-11-22 [email protected] Roll Packages from c9933fc to 2102327 (3 revisions) (flutter/flutter#138895) 2023-11-22 [email protected] Marks Mac_build_test flutter_gallery__transition_perf_e2e_ios to be unflaky (flutter/flutter#138886) 2023-11-22 [email protected] Roll Flutter Engine from d55b673f895c to c954a64c509b (1 revision) (flutter/flutter#138894) 2023-11-22 [email protected] Add `commandHasTerminal` parameter + apple usage event + `sendException` events for `package:unified_analytics` (flutter/flutter#138806) 2023-11-22 [email protected] Roll Flutter Engine from 6c8f7b566a22 to d55b673f895c (1 revision) (flutter/flutter#138890) 2023-11-22 [email protected] Roll Flutter Engine from dda2499df48a to 6c8f7b566a22 (1 revision) (flutter/flutter#138875) 2023-11-22 [email protected] Roll Flutter Engine from 1ae1d5318257 to dda2499df48a (4 revisions) (flutter/flutter#138872) 2023-11-22 [email protected] Roll Flutter Engine from 7cf9d90d59e1 to 1ae1d5318257 (8 revisions) (flutter/flutter#138861) 2023-11-22 [email protected] Revert "Reland VelocityTracker update (again)" (flutter/flutter#138863) 2023-11-21 [email protected] In `flutter doctor -v`, when JRE is too out-of-date to run `sdkmanager`, print a helpful error message (flutter/flutter#138762) 2023-11-21 [email protected] Roll Flutter Engine from a8e9b8dd562b to 7cf9d90d59e1 (1 revision) (flutter/flutter#138847) 2023-11-21 [email protected] Roll Flutter Engine from 90d1ff04ae02 to a8e9b8dd562b (1 revision) (flutter/flutter#138846) 2023-11-21 [email protected] Reland VelocityTracker update (again) (flutter/flutter#138843) 2023-11-21 [email protected] Roll Flutter Engine from 31799868224d to 90d1ff04ae02 (2 revisions) (flutter/flutter#138844) 2023-11-21 [email protected] Fix M3 Tabs Specs links (flutter/flutter#138808) 2023-11-21 [email protected] Roll Flutter Engine from 0c2de1e8849c to 31799868224d (1 revision) (flutter/flutter#138840) 2023-11-21 [email protected] Roll Flutter Engine from 746697c27569 to 0c2de1e8849c (1 revision) (flutter/flutter#138834) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…on` events for `package:unified_analytics` (flutter#138806) Relates to tracker issue: - flutter#128251 This PR includes 3 major updates: - Adding the `commandHasTerminal` parameter for `Event.flutterCommandResult` - In `packages/flutter_tools/lib/src/runner/flutter_command.dart` - Adding the new event for `sendException` from package:usage to be `Event.exception` (this event can be used by all dash tools) - In `packages/flutter_tools/lib/runner.dart` - Migrating the generic `UsageEvent` which was only used for Apple related workflows for iOS and macOS. I did an initial analysis in this [sheet](https://docs.google.com/spreadsheets/d/11KJLkHXFpECMX7tw-trNkYSr5MHDG15XNGv6TgLjfQs/edit?resourcekey=0-j4qdvsOEEg3wQW79YlY1-g#gid=0) to identify all the call sites - Found in several files, highlighted in the sheet above
…dException` events for `package:unified_analytics` (flutter/flutter#138806)
Relates to tracker issue:
package:unified_analyticsimplementation in flutter tool #128251This PR includes 3 major updates:
commandHasTerminalparameter forEvent.flutterCommandResultpackages/flutter_tools/lib/src/runner/flutter_command.dartsendExceptionfrom package:usage to beEvent.exception(this event can be used by all dash tools)packages/flutter_tools/lib/runner.dartUsageEventwhich was only used for Apple related workflows for iOS and macOS. I did an initial analysis in this sheet to identify all the call sitesPre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.