这是indexloc提供的服务,不要输入任何密码
Skip to content

Add usage event to track when a iOS network device is used #118915

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

Merged
merged 3 commits into from
Jan 25, 2023

Conversation

vashworth
Copy link
Contributor

@vashworth vashworth commented Jan 20, 2023

We recently added wireless debugging for iOS network devices #118895 and want to track it's usage.

Fixes #118479.

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.

@flutter-dashboard flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Jan 20, 2023
@vashworth
Copy link
Contributor Author

@jmagman You mentioned to me maybe using an existing tracking event such as when a user runs flutter run and adding data to it. I found this, which I believe is what you were referring to.

void _registerSignalHandlers(String commandPath, DateTime startTime) {
void handler(io.ProcessSignal s) {
globals.cache.releaseLock();
_sendPostUsage(
commandPath,
const FlutterCommandResult(ExitStatus.killed),
startTime,
globals.systemClock.now(),
);
}
globals.signals.addHandler(io.ProcessSignal.sigterm, handler);
globals.signals.addHandler(io.ProcessSignal.sigint, handler);
}
/// Logs data about this command.
///
/// For example, the command path (e.g. `build/apk`) and the result,
/// as well as the time spent running it.
void _sendPostUsage(
String commandPath,
FlutterCommandResult commandResult,
DateTime startTime,
DateTime endTime,
) {
if (commandPath == null) {
return;
}
assert(commandResult != null);
// Send command result.
CommandResultEvent(commandPath, commandResult.toString()).send();
// Send timing.
final List<String?> labels = <String?>[
if (commandResult.exitStatus != null)
getEnumName(commandResult.exitStatus),
if (commandResult.timingLabelParts?.isNotEmpty ?? false)
...?commandResult.timingLabelParts,
];
final String label = labels
.where((String? label) => label != null && !_isBlank(label))
.join('-');
globals.flutterUsage.sendTiming(
'flutter',
name,
// If the command provides its own end time, use it. Otherwise report
// the duration of the entire execution.
(commandResult.endTimeOverride ?? endTime).difference(startTime),
// Report in the form of `success-[parameter1-parameter2]`, all of which
// can be null if the command doesn't provide a FlutterCommandResult.
label: label == '' ? null : label,
);
}

Unfortunately the handler is registered before the device is selected, though, which is why I decided to go with a new event.

@vashworth vashworth requested a review from jmagman January 20, 2023 23:44
Comment on lines 1540 to 1545
final Iterable<Device> networkDevices = devices.where((Device device) {
return device is IOSDevice && device.interfaceType == IOSDeviceConnectionInterface.network;
});
if (networkDevices.isNotEmpty) {
UsageEvent('device', 'ios-network-device', label: name, flutterUsage: globals.flutterUsage).send();
}
Copy link
Member

Choose a reason for hiding this comment

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

This usage would tell us an absolute number of wireless iOS devices, however I think it would be more interesting to know what percentage of iOS devices are wireless. How about something like:

Suggested change
final Iterable<Device> networkDevices = devices.where((Device device) {
return device is IOSDevice && device.interfaceType == IOSDeviceConnectionInterface.network;
});
if (networkDevices.isNotEmpty) {
UsageEvent('device', 'ios-network-device', label: name, flutterUsage: globals.flutterUsage).send();
}
final Iterable<IOSDevice> iosDevices = devices.whereType<IOSDevice>();
if (iosDevices.isNotEmpty) {
final bool includesNetworkDevices =
iosDevices.any((IOSDevice device) => device.interfaceType == IOSDeviceConnectionInterface.network);
UsageEvent(
'device',
'ios-interface-type',
label: includesNetworkDevices ? 'wireless' : 'usb',
flutterUsage: globals.flutterUsage,
).send();
}

@jmagman
Copy link
Member

jmagman commented Jan 23, 2023

Spent some more time looking at this, maybe it's more appropriate to add this as a RunCommand.usageValues() CustomDimensions instead?

@christopherfujino can you weigh in on the best way to do this? Or maybe @eliasyishak working on analytics right now may have a suggestion?

@christopherfujino
Copy link
Contributor

christopherfujino commented Jan 23, 2023

Spent some more time looking at this, maybe it's more appropriate to add this as a RunCommand.usageValues() CustomDimensions instead?

@christopherfujino can you weigh in on the best way to do this? Or maybe @eliasyishak working on analytics right now may have a suggestion?

I'll defer to Elias on this one

@eliasyishak
Copy link
Contributor

@jmagman this is actually something I have been thinking about to standardize the usage events we send.

Does your RunCommand.usageValues() essentially resolve all the necessary data you want to send for collection?

@jmagman
Copy link
Member

jmagman commented Jan 23, 2023

Does your RunCommand.usageValues() essentially resolve all the necessary data you want to send for collection?

I guess that's my question, should @vashworth move her new usage dimension to that method? It seems like that's the right place for it.

@codecov-commenter
Copy link

Codecov Report

Merging #118915 (224e6aa) into master (bb73121) will decrease coverage by 0.06%.
The diff coverage is 96.27%.

❗ Current head 224e6aa differs from pull request most recent head 2ed679a. Consider uploading reports for the commit 2ed679a to get more accurate results

@@             Coverage Diff             @@
##           master   #118915      +/-   ##
===========================================
- Coverage   91.39%    91.34%   -0.06%     
===========================================
  Files         548       548              
  Lines       99585     98473    -1112     
===========================================
- Hits        91013     89946    -1067     
+ Misses       8572      8527      -45     
Impacted Files Coverage Δ
packages/flutter/lib/src/animation/animation.dart 91.66% <ø> (-0.34%) ⬇️
packages/flutter/lib/src/gestures/arena.dart 84.90% <ø> (-0.15%) ⬇️
packages/flutter/lib/src/gestures/binding.dart 86.52% <ø> (-0.38%) ⬇️
packages/flutter/lib/src/gestures/converter.dart 92.25% <ø> (-0.10%) ⬇️
...ackages/flutter/lib/src/gestures/drag_details.dart 100.00% <ø> (ø)
packages/flutter/lib/src/gestures/hit_test.dart 94.64% <ø> (-0.19%) ⬇️
packages/flutter/lib/src/gestures/long_press.dart 87.15% <ø> (-0.35%) ⬇️
packages/flutter/lib/src/gestures/monodrag.dart 96.37% <ø> (-0.06%) ⬇️
...tter/lib/src/gestures/pointer_signal_resolver.dart 70.00% <ø> (-2.73%) ⬇️
packages/flutter/lib/src/gestures/recognizer.dart 93.49% <ø> (-0.04%) ⬇️
... and 93 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@eliasyishak
Copy link
Contributor

eliasyishak commented Jan 23, 2023

@jmagman @vashworth I think that would make sense to add it to the CustomDimensions class that gets returned from the RunCommand.usageValues getter instead of sending it yourself through UsageEvents.send(...). That way you can just piggy back on an event that is already being sent.

But do you need to add a new custom dimension?

@vashworth
Copy link
Contributor Author

But do you need to add a new custom dimension?

Yes, I believe so. I see that it would need to be added in https://github.com/flutter/flutter/blob/master/packages/flutter_tools/lib/src/reporting/custom_dimensions.dart. Looks like I also need to add it to Google Analytics (https://support.google.com/analytics/answer/2709829#zippy=%2Cin-this-article)?

@jmagman
Copy link
Member

jmagman commented Jan 23, 2023

maybe it's more appropriate to add this as a RunCommand.usageValues() CustomDimensions instead?

Also DriveCommand, so the RunCommandBase commands. Although that doesn't seem to override any usageValues. They should probably be sending up the same data?

@vashworth you don't need to figure that out in this PR, run is good enough for now.

@jmagman jmagman requested review from eliasyishak and removed request for christopherfujino January 24, 2023 00:24
Copy link
Member

@jmagman jmagman 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 doing this twice after I lead you astray the first time 🙂
LGTM! @eliasyishak should also review.

Copy link
Contributor

@eliasyishak eliasyishak left a comment

Choose a reason for hiding this comment

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

LGTM! Let me know if you would like some help querying the database after this gets merged in as well

@vashworth vashworth merged commit 81052a7 into flutter:master Jan 25, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 26, 2023
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Jan 27, 2023
* a0f7c8c 6f806491e [web] use a render target instead of a new surface for Picture.toImage (flutter/engine#38573) (flutter/flutter#119143)

* e85547b Roll Plugins from 11361d01099d to 8bab180a668a (28 revisions) (flutter/flutter#119115)

* 81052a7 Add usage event to track when a iOS network device is used (flutter/flutter#118915)

* cd34fa6 24aa324b8 Roll Skia from da5034f9d117 to c4b171fe5668 (1 revision) (flutter/engine#39127) (flutter/flutter#119159)

* 6cd4fa4 Add --serve-observatory flag to run, attach, and test (flutter/flutter#118402)

* 48cd95d 1e5efd144 [various] Enable use_build_context_synchronously (flutter/plugins#6585) (flutter/flutter#119162)

* b907acd Add the cupertino system colors mint, cyan, and brown (flutter/flutter#118971)

* c6fa5d9 c54580138 Only build analyze_snapshot on Linux host (flutter/engine#39129) (flutter/flutter#119164)

* f34ce86 7b72038ef Roll Fuchsia Linux SDK from E9m-Gk382PkB7_Nbp... to pGX7tanT1okL8XCg-... (flutter/engine#39130) (flutter/flutter#119169)

* 0dd63d3 Export View (flutter/flutter#117475)

* 1fd71de Remove superfluous words from comments (flutter/flutter#119055)

* cef9cc7 2e7d6fa7b Remove unnecessary null checks (flutter/engine#39113) (flutter/flutter#119174)

* 3be330a 30c02e4c8 [Impeller] Make text glyph offsets respect the current transform (flutter/engine#39119) (flutter/flutter#119179)

* a45727d Add MediaQuery to View (flutter/flutter#118004)

* 02a9c15 Fix lexer issue where select/plural/other/underscores cannot be in identifier names. (flutter/flutter#119190)

* 766e4d2 Remove single-view assumption from material library (flutter/flutter#117486)

* dcd3679 Roll Flutter Engine from 30c02e4c8b01 to 44362c90fcec (2 revisions) (flutter/flutter#119185)

* 9037e3f roll packages (flutter/flutter#119192)

* e0e88da Roll Flutter Engine from 44362c90fcec to 308ce918f67f (2 revisions) (flutter/flutter#119201)

* 202e902 Roll Flutter Engine from 308ce918f67f to 8f1e5dc1b124 (4 revisions) (flutter/flutter#119208)

* b319938 Add more flexible image API (flutter/flutter#118966)

* fc02701 Marks Mac run_debug_test_macos to be unflaky (flutter/flutter#117470)

* c9affdb Move windows-x64-flutter.zip to windows-x64-debug location. (flutter/flutter#119177)
auto-submit bot pushed a commit to flutter/plugins that referenced this pull request Jan 28, 2023
* a0f7c8c 6f806491e [web] use a render target instead of a new surface for Picture.toImage (flutter/engine#38573) (flutter/flutter#119143)

* e85547b Roll Plugins from 11361d0 to 8bab180 (28 revisions) (flutter/flutter#119115)

* 81052a7 Add usage event to track when a iOS network device is used (flutter/flutter#118915)

* cd34fa6 24aa324b8 Roll Skia from da5034f9d117 to c4b171fe5668 (1 revision) (flutter/engine#39127) (flutter/flutter#119159)

* 6cd4fa4 Add --serve-observatory flag to run, attach, and test (flutter/flutter#118402)

* 48cd95d 1e5efd1 [various] Enable use_build_context_synchronously (#6585) (flutter/flutter#119162)

* b907acd Add the cupertino system colors mint, cyan, and brown (flutter/flutter#118971)

* c6fa5d9 c54580138 Only build analyze_snapshot on Linux host (flutter/engine#39129) (flutter/flutter#119164)

* f34ce86 7b72038ef Roll Fuchsia Linux SDK from E9m-Gk382PkB7_Nbp... to pGX7tanT1okL8XCg-... (flutter/engine#39130) (flutter/flutter#119169)

* 0dd63d3 Export View (flutter/flutter#117475)

* 1fd71de Remove superfluous words from comments (flutter/flutter#119055)

* cef9cc7 2e7d6fa7b Remove unnecessary null checks (flutter/engine#39113) (flutter/flutter#119174)

* 3be330a 30c02e4c8 [Impeller] Make text glyph offsets respect the current transform (flutter/engine#39119) (flutter/flutter#119179)

* a45727d Add MediaQuery to View (flutter/flutter#118004)

* 02a9c15 Fix lexer issue where select/plural/other/underscores cannot be in identifier names. (flutter/flutter#119190)

* 766e4d2 Remove single-view assumption from material library (flutter/flutter#117486)

* dcd3679 Roll Flutter Engine from 30c02e4c8b01 to 44362c90fcec (2 revisions) (flutter/flutter#119185)

* 9037e3f roll packages (flutter/flutter#119192)

* e0e88da Roll Flutter Engine from 44362c90fcec to 308ce918f67f (2 revisions) (flutter/flutter#119201)

* 202e902 Roll Flutter Engine from 308ce918f67f to 8f1e5dc1b124 (4 revisions) (flutter/flutter#119208)

* b319938 Add more flexible image API (flutter/flutter#118966)

* fc02701 Marks Mac run_debug_test_macos to be unflaky (flutter/flutter#117470)

* c9affdb Move windows-x64-flutter.zip to windows-x64-debug location. (flutter/flutter#119177)

* 7d3b762 Fix: Added `margin` parameter for `MaterialBanner` class (flutter/flutter#119005)

* 40bd82e Roll Plugins from 1e5efd1 to e9406bc (4 revisions) (flutter/flutter#119249)

* 07522b7 Roll Flutter Engine from 8f1e5dc1b124 to 04f22beebb42 (5 revisions) (flutter/flutter#119218)

* 459c1b7 Marks Mac complex_layout_scroll_perf_macos__timeline_summary to be unflaky (flutter/flutter#119157)

* 2b8f2d0 Add API for discovering assets (flutter/flutter#118410)

* a04ab71 Revert "Add API for discovering assets (#118410)" (flutter/flutter#119273)

* 1da487d Roll Flutter Engine from 04f22beebb42 to 93901260098e (12 revisions) (flutter/flutter#119279)

* 1b779b6 Roll Flutter Engine from 93901260098e to be0125bd5716 (2 revisions) (flutter/flutter#119283)

* 42bd5f2 Download platform-agnostic Flutter Web SDK in the flutter_tool (flutter/flutter#118654)

* d52b6b9 Roll Flutter Engine from be0125bd5716 to d17004dd96d7 (2 revisions) (flutter/flutter#119287)

* 4aed487 Roll Flutter Engine from d17004dd96d7 to a63d98feb608 (3 revisions) (flutter/flutter#119299)

* 05fc29f Rename DeviceGestureSettings.fromWindow to DeviceGestureSettings.fromView (flutter/flutter#119291)

* 86ab01d Revert "Add --serve-observatory flag to run, attach, and test (#118402)" (flutter/flutter#119302)

* 8d03af3 Roll Flutter Engine from a63d98feb608 to 79c958fc7e9b (3 revisions) (flutter/flutter#119306)

* 27f8ebd ade610ec8 [fuchsia] Migrate to new RealmBuilder API (flutter/engine#39175) (flutter/flutter#119310)

* c31856b Roll Plugins from e9406bc to ff84c44 (2 revisions) (flutter/flutter#119335)

* d939863 Roll Flutter Engine from ade610ec88b5 to 621e13cc9be3 (3 revisions) (flutter/flutter#119344)

* 0b57596 Run "flutter update-packages --force-upgrade" (flutter/flutter#119340)

* 0417f66 Fix nullability of TableRow.children (flutter/flutter#119285)

* fc3e824 Roll Flutter Engine from 621e13cc9be3 to 189a69d9918d (3 revisions) (flutter/flutter#119347)

* ad1a44d Add `requestFocusOnTap` to `DropdownMenu` (flutter/flutter#117504)

* 4dbb573 [flutter_tools] remove usage of remap samplers arg (flutter/flutter#119346)

* b2f2bf3 Marks Linux run_release_test_linux to be unflaky (flutter/flutter#119156)

* 3f95bef Roll Flutter Engine from 189a69d9918d to b32fc7fef208 (3 revisions) (flutter/flutter#119358)

* 2e8bebd Remove single window assumption from macrobenchmark (flutter/flutter#119368)

* ab2232a Roll Flutter Engine from b32fc7fef208 to 8567d96993ed (5 revisions) (flutter/flutter#119369)

* e9ca9cc Remove references to dart:ui's window singelton (flutter/flutter#119296)

* da3d4bd Roll Flutter Engine from 8567d96993ed to 225ae87334a5 (2 revisions) (flutter/flutter#119376)

* 018c1f8 e2e089ebb Use arm64 engine variant on simulators in iOS unit tests (flutter/engine#39213) (flutter/flutter#119387)

* e349bdc 19651cb1d Roll Dart SDK from 2cd9b7ac95e8 to 135f4c51c9ff (3 revisions) (flutter/engine#39214) (flutter/flutter#119389)

* 95345b5 77bee011d Roll Dart SDK from 2cd9b7ac95e8 to 135f4c51c9ff (3 revisions) (flutter/engine#39217) (flutter/flutter#119394)

* de43ec9 Roll Flutter Engine from 77bee011dabf to 3394b84cc5d7 (3 revisions) (flutter/flutter#119405)

* f8d4de4 3dd0fc13f Roll Fuchsia Linux SDK from 6c2H32X3EXOGlWIgb... to TiK_fVODtUaKOgxRf... (flutter/engine#39224) (flutter/flutter#119408)

* 7856411 7c5c6c9c9 Roll Skia from 0b75650caf2a to 7df7a83f733d (13 revisions) (flutter/engine#39225) (flutter/flutter#119413)

* 75680ae 649362168 Roll Dart SDK from f9583e13e214 to 52dc94238144 (1 revision) (flutter/engine#39227) (flutter/flutter#119416)
@vashworth vashworth deleted the track_wireless_debugging branch September 27, 2023 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tool Affects the "flutter" command-line tool. See also t: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Track iOS wireless debugging
5 participants