Skip to content

Ensure all events sent via package:unified_analytics are received #136926

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

Closed
christopherfujino opened this issue Oct 19, 2023 · 7 comments
Closed
Labels
P1 High-priority issues at the top of the work list team-tool Owned by Flutter Tool team triaged-tool Triaged by Flutter Tool team

Comments

@christopherfujino
Copy link
Contributor

From the discussion in #136647 (comment), since the public API to send analytics from package:unified_analytics returns a Future<http.Response> (https://github.com/dart-lang/tools/blob/92c5c15e3eb713b39779f4545bfa207ccdfeb1af/pkgs/unified_analytics/lib/src/analytics.dart#L562), callers need to decide to either:

  1. await this future, or
  2. ignore the future, with the unawaited() function

With option 1, however, this would mean that future tool work may not happen until AFTER the HTTP request has finished. And with option 2, if the dart isolate finishes its other work, it appears to exit, cancelling the HTTP request to the analytics server.

With the existing GA3 implementation, before exiting the tool we call globals.flutterUsage.ensureAnalyticsSent(); https://github.com/flutter/flutter/blob/main/packages/flutter_tools/lib/src/base/process.dart#L617 which ends up calling the waitForLastPing() method in package:usage: https://github.com/dart-lang/usage/blob/master/lib/src/usage_impl.dart#L224. package:usage maintains its own list of futures (https://github.com/dart-lang/usage/blob/master/lib/src/usage_impl.dart#L77) that waitForLastPing() will ensure are completed (although we provide that method with a 250ms timeout to avoid extended delays of the tool exiting).

I believe the simplest solution for the flutter tool would be to create a global class that wraps all calls to package:unified_analytics' .send() method, and collects the futures. Then we can await all these futures at the same place we do for GA3 events (https://github.com/flutter/flutter/blob/main/packages/flutter_tools/lib/src/base/process.dart#L617), with a 250ms timeout.

@christopherfujino christopherfujino added P1 High-priority issues at the top of the work list team-tool Owned by Flutter Tool team triaged-tool Triaged by Flutter Tool team labels Oct 19, 2023
@christopherfujino
Copy link
Contributor Author

FYI @eliasyishak & @andrewkolos

@christopherfujino
Copy link
Contributor Author

Actually, what would be easier than wrapping, is to just have class Analytics from package:unified_analytics do what package:usage did, and keep track of all these futures, and expose a public .waitForLastPing({Duration? timeout}) method that takes an optional timeout.

@eliasyishak
Copy link
Contributor

Related to what I said in this comment, whatever we decide on for sending events, I don't think we can reliably check BigQuery exports to confirm our approach is working.

I am exploring now how it would look if we kept the futures for each send internal to package:unified_analytics now

@eliasyishak
Copy link
Contributor

Captured the issue in the dart-lang/tools repo as well, I will create a separate PR

@eliasyishak
Copy link
Contributor

Looping back on this issue, we made progress on this by implementing an internal futures list within the Analytics instance so that we don't have to await send calls anymore.

There was also additional functionality added to the existing close method to flush out any remaining send events in the futures list (if there are any), and similar to package:usage, we also provide a default value of 250 ms before the close method timeouts and exits.

PR below is where these updates were made:

@eliasyishak
Copy link
Contributor

eliasyishak commented Nov 15, 2023

Closing since I believe we addressed this in dart-lang/tools#184, feel free to reopen if needed

Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
P1 High-priority issues at the top of the work list team-tool Owned by Flutter Tool team triaged-tool Triaged by Flutter Tool team
Projects
None yet
Development

No branches or pull requests

2 participants