[GitHub] [beam] alexeyinkin commented on a diff in pull request #24957: [Playground] [Frontend] Migrate Playground to Google Analytics 4

["alexeyinkin (via GitHub)" <gi...@apache.org>]

alexeyinkin commented on code in PR #24957:
URL: https://github.com/apache/beam/pull/24957#discussion_r1130975127


##########
playground/frontend/lib/components/playground_run_or_cancel_button.dart:
##########
@@ -20,38 +20,38 @@ import 'package:flutter/widgets.dart';
 import 'package:playground_components/playground_components.dart';
 import 'package:provider/provider.dart';
 
-import '../modules/analytics/analytics_service.dart';
-import '../utils/analytics_utils.dart';
-
 class PlaygroundRunOrCancelButton extends StatelessWidget {
   const PlaygroundRunOrCancelButton();
 
   @override
   Widget build(BuildContext context) {
     return Consumer<PlaygroundController>(
-      builder: (context, playgroundController, child) {
-        final analyticsService = AnalyticsService.get(context);
-        final stopwatch = Stopwatch();
-        final exampleName = getAnalyticsExampleName(playgroundController);
-
-        return RunOrCancelButton(
-          playgroundController: playgroundController,
-          beforeCancel: () {
-            final exampleName = getAnalyticsExampleName(playgroundController);
-            analyticsService.trackClickCancelRunEvent(exampleName);
-          },
-          beforeRun: () {
-            stopwatch.start();
-            analyticsService.trackClickRunEvent(exampleName);
-          },
-          onComplete: () {
-            analyticsService.trackRunTimeEvent(
-              exampleName,
-              stopwatch.elapsedMilliseconds,
-            );
-          },
-        );
-      }
+      builder: (context, playgroundController, child) => RunOrCancelButton(
+        playgroundController: playgroundController,
+        beforeCancel: (runner) {
+          PlaygroundComponents.analyticsService.sendUnawaited(

Review Comment:
   `sendUnawaited` is to wrap the sending function with an `unawaited` call so we do not get the lint of a lost Future (just added to the doc comment). This is better than wrapping all analytics calls with explicit `unawaited`. So the first thing to get it is to think of it as just `send` which is simpler to understand.
   
   `AnalyticsService.sendRunOrCancel` is not an option because the service must then be aware of the running state, but we aim to keep it thin, thus the separate methods.
   
   I do not see a problem here because `RunOrCancelButton` has the separate callbacks so the three specific events can be fired there.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org