Skip to content

Conversation

@eliasyishak
Copy link
Contributor

@eliasyishak eliasyishak commented Oct 16, 2023

Related to tracking issue:

This PR sends analytic events for each of the doctor validators.

This PR below will need to land first in dart-lang/tools before this merges.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@eliasyishak eliasyishak changed the title Add code to send events Unified analytics events for doctor validators Oct 16, 2023
@github-actions github-actions bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Oct 16, 2023
@github-actions github-actions bot added the a: text input Entering text in a text field or keyboard related problems label Oct 19, 2023
@github-actions github-actions bot added the a: text input Entering text in a text field or keyboard related problems label Oct 24, 2023
@eliasyishak eliasyishak requested a review from vashworth October 25, 2023 16:17
Comment on lines 419 to 441
if (validator is GroupedValidator) {
for (int i = 0; i < validator.subValidators.length; i++) {
final DoctorValidator subValidator = validator.subValidators[i];
final ValidationResult subResult = validator.subResults[i];

_analytics.send(Event.doctorValidatorResult(
validatorName: subValidator.title,
result: subResult.typeStr,
statusInfo: subResult.statusInfo,
partOfGroupedValidator: true,
doctorInvocationId: analyticsTimestamp,
));
}
} else {
_analytics.send(Event.doctorValidatorResult(
validatorName: validator.title,
result: result.typeStr,
statusInfo: result.statusInfo,
partOfGroupedValidator: false,
doctorInvocationId: analyticsTimestamp,
));
}
// TODO(eliasyishak): remove this after migrating from package:usage
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vashworth This is really the only updates made to send the events.. everything else is either making changes to direct inject the necessary instances or test updates

if (validator is GroupedValidator) {
for (int i = 0; i < validator.subValidators.length; i++) {
final DoctorValidator subValidator = validator.subValidators[i];
final ValidationResult subResult = validator.subResults[i];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there guaranteed to be a subResult for each subValidator? Seems like maybe a potential issue for out of index error

Copy link
Contributor Author

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, but yes, i believe that that the validator.subValidators and validators.subResults lists will be the same length because that's how we have been sending them to GA3 in the past

https://github.com/eliasyishak/flutter/blob/sending-doctor-events-ga4/packages/flutter_tools/lib/src/reporting/events.dart#L133-L137

Also this method seems to be what is responsible for merging the results for validators that are grouped

https://github.com/eliasyishak/flutter/blob/sending-doctor-events-ga4/packages/flutter_tools/lib/src/doctor_validator.dart#L109-L137

Copy link
Contributor

@vashworth vashworth Oct 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the first example you sent above, it does a check for if subResults is empty. Should we do the same here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another great catch, didn't realize the doctor validators could fail!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually think the grouped validators will always return those two lists to have the same length due to this code snippet below

https://github.com/eliasyishak/flutter/blob/sending-doctor-events-ga4/packages/flutter_tools/lib/src/doctor_validator.dart#L99-L102

But just incase, i have also added an additional conditional to ensure the lengths of the lists are the same before looping through the subvalidators

Copy link
Contributor

@vashworth vashworth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@vashworth vashworth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I just realized continuing will not print out details about the validator

Copy link
Contributor

@vashworth vashworth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making those changes! LGTM!

@eliasyishak eliasyishak added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 26, 2023
@auto-submit auto-submit bot merged commit 56ae555 into flutter:master Oct 26, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 27, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 27, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 27, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 27, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 27, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 27, 2023
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Oct 27, 2023
Roll Flutter from c555599 to 5907c97 (45 revisions)

flutter/flutter@c555599...5907c97

2023-10-27 [email protected] Add `isLogicalKeyPressed` to `KeyEvent` (flutter/flutter#136856)
2023-10-27 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Ensure `flutter build apk --release` optimizes+shrinks platform code" (flutter/flutter#137433)
2023-10-27 [email protected] Roll Flutter Engine from bedc49efc85c to a198ad4e740d (20 revisions) (flutter/flutter#137429)
2023-10-27 [email protected] Make `SemanticsNode.isMergedIntoParent` Readonly (flutter/flutter#137304)
2023-10-27 [email protected] Manual roll Flutter Engine from bedc49efc85c to 71e1a0430232 (16 revisions) (flutter/flutter#137422)
2023-10-27 [email protected] Roll Packages from fea24c5 to 2af6954 (5 revisions) (flutter/flutter#137421)
2023-10-27 [email protected] Run tests on either macOS 12 or 13 (flutter/flutter#137365)
2023-10-27 [email protected] Revert "Update `DataTable` test when data row is pressed for Material 3 (#137230)" (flutter/flutter#137407)
2023-10-27 [email protected] Revert "Reland - Update `OutlinedButton` tests for Material 3 (#136809) (#137247)" (flutter/flutter#137406)
2023-10-27 [email protected] Ensure `flutter build apk --release` optimizes+shrinks platform code (flutter/flutter#136880)
2023-10-27 [email protected] Update `DataTable` test when data row is pressed for Material 3 (flutter/flutter#137230)
2023-10-27 [email protected] Reland - Update `OutlinedButton` tests for Material 3 (#136809) (flutter/flutter#137247)
2023-10-27 [email protected] update asset manifest file name referenced in `WebServiceWorker` (flutter/flutter#135954)
2023-10-27 [email protected] give `throwsToolExit` a more useful description (flutter/flutter#136694)
2023-10-27 [email protected] Add ConstrainedLayoutBuilder.updateShouldRebuild() (flutter/flutter#136691)
2023-10-27 [email protected] Fix. typos (flutter/flutter#137325)
2023-10-27 [email protected] Roll Flutter Engine from 87f384c2d70b to bedc49efc85c (1 revision) (flutter/flutter#137382)
2023-10-26 [email protected] Fix Typos (flutter/flutter#137292)
2023-10-26 [email protected] AnimationController should dispatch creation in constructor. (flutter/flutter#134839)
2023-10-26 [email protected] Roll Flutter Engine from 9788bb9ff83e to 87f384c2d70b (4 revisions) (flutter/flutter#137380)
2023-10-26 [email protected] Roll Flutter Engine from ce1c1ee54107 to 9788bb9ff83e (3 revisions) (flutter/flutter#137373)
2023-10-26 [email protected] Marks Windows_arm64 windows_startup_test to be unflaky (flutter/flutter#137228)
2023-10-26 [email protected] Marks Windows_arm64 flutter_tool_startup__windows to be unflaky (flutter/flutter#137229)
2023-10-26 [email protected] Marks Windows_arm64 windows_home_scroll_perf__timeline_summary to be unflaky (flutter/flutter#137221)
2023-10-26 [email protected] Declare dependency on copyFlutterAssetsTask in bundleAarTask (flutter/flutter#137370)
2023-10-26 [email protected] Marks Windows_arm64 complex_layout_win_desktop__start_up to be unflaky (flutter/flutter#137225)
2023-10-26 [email protected] Roll Flutter Engine from 394744d2c4d0 to ce1c1ee54107 (2 revisions) (flutter/flutter#137367)
2023-10-26 [email protected] Marks Windows_arm64 hot_mode_dev_cycle_win_target__benchmark to be unflaky (flutter/flutter#137217)
2023-10-26 [email protected] Marks Windows_arm64 flutter_view_win_desktop__start_up to be unflaky (flutter/flutter#137226)
2023-10-26 [email protected] Marks Windows_arm64 platform_view_win_desktop__start_up to be unflaky (flutter/flutter#137227)
2023-10-26 [email protected] Marks Windows_arm64 flutter_gallery_win_desktop__start_up to be unflaky (flutter/flutter#137224)
2023-10-26 [email protected] Marks Windows_arm64 hello_world_win_desktop__compile to be unflaky (flutter/flutter#137222)
2023-10-26 [email protected] Marks Windows_arm64 flutter_gallery_win_desktop__compile to be unflaky (flutter/flutter#137223)
2023-10-26 [email protected] Marks Windows_arm64 run_release_test_windows to be unflaky (flutter/flutter#137220)
2023-10-26 [email protected] Fix dislocated doc and comment on ThemeData localize cache (flutter/flutter#137315)
2023-10-26 [email protected] Marks Windows_arm64 platform_channel_sample_test_windows to be unflaky (flutter/flutter#137218)
2023-10-26 [email protected] Roll Flutter Engine from 7c5c8f587992 to 394744d2c4d0 (1 revision) (flutter/flutter#137364)
2023-10-26 [email protected] Roll Flutter Engine from 9363fe6ba503 to 7c5c8f587992 (3 revisions) (flutter/flutter#137363)
2023-10-26 [email protected] Roll Flutter Engine from 542f8bc4c019 to 9363fe6ba503 (2 revisions) (flutter/flutter#137354)
2023-10-26 [email protected] Unified analytics events for doctor validators (flutter/flutter#136647)
2023-10-26 [email protected] Roll Flutter Engine from d8132d5070bf to 542f8bc4c019 (2 revisions) (flutter/flutter#137349)
2023-10-26 [email protected] Marks Windows_arm64 run_debug_test_windows to be unflaky (flutter/flutter#137219)
2023-10-26 [email protected] Roll Flutter Engine from 0a6253dbfafd to d8132d5070bf (1 revision) (flutter/flutter#137347)
2023-10-26 [email protected] Roll Flutter Engine from 5da115661f01 to 0a6253dbfafd (1 revision) (flutter/flutter#137341)
2023-10-26 [email protected] Roll Flutter Engine from 61ae5ef94e9c to 5da115661f01 (1 revision) (flutter/flutter#137340)

...
@eliasyishak eliasyishak deleted the sending-doctor-events-ga4 branch October 27, 2023 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: text input Entering text in a text field or keyboard related problems autosubmit Merge PR when tree becomes green via auto submit App tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants