Configure GA for memory screen. #4705

polina-c · 2022-11-04T22:09:22Z

No description provided.

bkonyi

Everything LGTM from a code point of view, but I'd feel more comfortable letting @kenzieschmoll verify to make sure the analytics changes are good.

bkonyi · 2022-11-07T14:05:09Z

packages/devtools_app/lib/src/screens/memory/panes/control/settings_dialog.dart

@@ -35,7 +35,8 @@ class MemorySettingsDialog extends StatelessWidget {
          children: [
            CheckboxSetting(
              notifier: preferences.memory.androidCollectionEnabled,
-              title: 'Show Android memory chart',
+              title:
+                  'Show Android memory chart in addition to Dart memory chart',


Is this a tooltip?

Nope, it is label for the checkbox in the settings dialog.

packages/devtools_app/lib/src/analytics/constants.dart

kenzieschmoll · 2022-11-07T18:17:10Z

packages/devtools_app/lib/src/screens/memory/panes/diff/controller/diff_pane_controller.dart

-    // Make sure the method does not trigger itself recursively.
-    assert(!_updatingValues);
-    _updatingValues = true;
+    _startUpdatingValues();


instead of using timeStart and timeEnd, you could use timeSync, and place the whole body of this method in the syncOperation function parameter. This will be less added lines of code and will reduce some duplication

ga.timeSync( analytics_constants.memory, analytics_constants.MemoryTime.updateValues, syncOperation: () { ... the previous body of this entire method }, );

I thought about it. There are other non-functional operations to be done at start and at end. The current structure seems to be most readable for me, as non-functional elements do not crowd the method. Yes, at cost of code duplicates.

kenzieschmoll · 2022-11-07T18:19:14Z

packages/devtools_app/lib/src/screens/memory/panes/diff/controller/heap_diff.dart

+      objectsAfter: couple.younger.data.objects.length,
+    ),
+  );
+  return result;


if the catch block in timeSync is hit, this will throw a late initialization error. Should we let result be nullable to handle this? or rethrow?

ProcessCancelledException will never be thrown by DiffHeapClasses. If we ever start supporting it, we will test how it works and make needed adjustments.

But, what's the reason for timeSync to handle ProcessCancelledException? It seems it should be handled by the entity that started the calculation and wants to consume the result. Am I wrong?

I'm not referring to ProcessCancelledException. If any exception is thrown by the function passed to syncOperation, the catch block will be hit.

kenzieschmoll · 2022-11-07T18:22:04Z

packages/devtools_app/lib/src/screens/memory/panes/diff/controller/heap_diff.dart

+  return result;
+}
+
+class _DiffAnalyticsMetrics extends ScreenAnalyticsMetrics {


any custom metrics added in a ScreenAnalyticsMetrics subclass will need to be added to the _gtagEvent, _gtagException, and the GtagEventDevTools class in _analytics_web.dart. These will also need to be added in the Analytics360 console.

For all these reasons, we should make sure any custom metrics we want to add really will add meaning to our analysis of the timed operation.

It would also be preferable if we didn't have multiple ScreenAnalyticsMetrics subclasses for a single screen. For example, the only existing subclasses we currently have are PerformanceScreenMetrics and ProfilerScreenMetrics, so ideally we could also have MemoryScreenMetrics that hold all the custom metrics we need for the Memory screen.

It looks to be cleaner to put the subclasses to the folder 'analytics', the same way as constants, so that analytics_web does not have to depend on individual screen folders.

I will move other subclasses later, in a separate PR.

is there a guidance to add this to the Analytics360 console?

I think I may be the only one with Admin access to our console, so if you can make the changes to the analytics_web file, I will update the analytics 360 console when those land. The order matters here (you'll see other comments like "// metric5, // metric6", so once the changes are in the order should not be changed.

packages/devtools_app/lib/src/analytics/constants.dart

...tools_app/lib/src/screens/memory/panes/allocation_profile/allocation_profile_table_view.dart

kenzieschmoll · 2022-11-07T18:44:41Z

...ools_app/lib/src/screens/memory/panes/allocation_tracing/allocation_profile_class_table.dart

+            onChanged: (value) {
+              ga.select(
+                analytics_constants.memory,
+                analytics_constants.MemoryEvent.tracingClassFilter,


will this be triggered on every update to the text field? We probably don't want to send an analytics event for every character change to the text field. Can we send this in onSubmitted or onEditingComplete instead? You'll have to play with this a bit to make sure we send the event at the right time

Added throttling to handle this.

kenzieschmoll · 2022-11-07T18:46:36Z

packages/devtools_app/lib/src/screens/memory/panes/control/source_dropdown.dart

-          ga.select(
-            analytics_constants.memory,
-            analytics_constants.sourcesDropDown,
-          );


are we removing this because we are removing the SourcesDropdown feature?

Yes, as we agreed in #4645, we will redesign the feature. I do not see value in maintaining analytics for it.

packages/devtools_app/lib/src/screens/memory/panes/diff/widgets/snapshot_control_pane.dart

packages/devtools_app/lib/src/screens/memory/panes/diff/widgets/snapshot_list.dart

packages/devtools_app/lib/src/screens/memory/shared/heap/model.dart

kenzieschmoll · 2022-11-07T18:54:23Z

packages/devtools_app/lib/src/screens/memory/shared/heap/model.dart

+      numberOfObjects: graph.objects.length,
+    ),
+  );
+  return result;


also same comment about whether we should add handling for the case when the timeSync catch block is hit

added rethrow to tymeSync

packages/devtools_app/lib/src/analytics/_analytics_stub.dart

kenzieschmoll · 2022-11-07T22:50:38Z

...ools_app/lib/src/screens/memory/panes/allocation_tracing/allocation_profile_class_table.dart

-                analytics_constants.memory,
-                analytics_constants.MemoryEvent.tracingClassFilter,
-              );
+              _sendFilterEditGaEvent();


is there a reason we can't use onSubmitted or onEditingComplete? Seems a bit hack to throttle when we can use one of the text field lifecycle methods that indicates "the user is done editing"

both are designed to handle ui forms or dialogs that at some moment are 'done'. they need special signal that editing is done. it is not our case.

throttling seems to be simplest and most reliable way to handle ga for filter

https://stackoverflow.com/questions/63690311/flutter-textfield-difference-between-onedittingcomplete-and-onsubmitted

packages/devtools_app/lib/src/screens/profiler/cpu_profile_controller.dart

kenzieschmoll · 2022-11-07T23:01:37Z

packages/devtools_app/lib/src/screens/memory/panes/diff/controller/diff_pane_controller.dart

@@ -42,6 +45,17 @@ class DiffPaneController extends DisposableController {
  // is taken, and is used to assign a unique id to each [SnapshotListItem].
  int _snapshotId = 0;

+  VoidCallback? takeSnapshotHandler(String gaEvent) {


instead of adding an additional handler, can we just add an optional parameter gaEvent to takeSnapshot and send the analytics event from there?

Then from the widget code:
onPressed: () => controller.takeSnapshot(myGaEvent)

I'm not seeing extra value by adding an additional method, plus this adds risk that takeSnapshot can be called from somewhere else and analytics will not be recorded.

In both cases the handler should be null if snapshot taking is already in process. I do not want this logic to be duplicated.

polina-c added 10 commits November 3, 2022 17:14

-

e9672d4

Update constants.dart

45f6365

-

e2e8b78

-

ab3334d

-

f900941

Merge branch 'master' of github.com:flutter/devtools into ga

caf590c

-

6fd4e32

-

f539b77

-

6a035ec

-

043da1a

polina-c requested review from bkonyi and a team as code owners November 4, 2022 22:09

polina-c mentioned this pull request Nov 4, 2022

Memory screen: implement profile diff. #3949

Closed

17 tasks

polina-c added 3 commits November 4, 2022 15:19

Merge branch 'master' of github.com:flutter/devtools into ga

17a2a90

-

5ed1959

-

f1c8588

bkonyi requested a review from kenzieschmoll November 7, 2022 14:07

bkonyi approved these changes Nov 7, 2022

View reviewed changes

Merge branch 'master' of github.com:flutter/devtools into ga

1f15d12

kenzieschmoll reviewed Nov 7, 2022

View reviewed changes

packages/devtools_app/lib/src/analytics/constants.dart Outdated Show resolved Hide resolved

kenzieschmoll reviewed Nov 7, 2022

View reviewed changes

packages/devtools_app/lib/src/analytics/constants.dart Show resolved Hide resolved

kenzieschmoll reviewed Nov 7, 2022

View reviewed changes

packages/devtools_app/lib/src/analytics/constants.dart Outdated Show resolved Hide resolved

kenzieschmoll reviewed Nov 7, 2022

View reviewed changes

...tools_app/lib/src/screens/memory/panes/allocation_profile/allocation_profile_table_view.dart Outdated Show resolved Hide resolved

kenzieschmoll reviewed Nov 7, 2022

View reviewed changes

packages/devtools_app/lib/src/screens/memory/panes/diff/widgets/snapshot_control_pane.dart Outdated Show resolved Hide resolved

kenzieschmoll reviewed Nov 7, 2022

View reviewed changes

packages/devtools_app/lib/src/screens/memory/panes/diff/widgets/snapshot_list.dart Outdated Show resolved Hide resolved

kenzieschmoll reviewed Nov 7, 2022

View reviewed changes

packages/devtools_app/lib/src/screens/memory/shared/heap/model.dart Outdated Show resolved Hide resolved

kenzieschmoll reviewed Nov 7, 2022

View reviewed changes

polina-c added 7 commits November 7, 2022 11:30

-

609e493

-

d68fcb8

-

82ae794

-

ed5d1e4

Update diff_pane_controller.dart

07537f3

-

48b5845

-

cf7725f

kenzieschmoll reviewed Nov 7, 2022

View reviewed changes

packages/devtools_app/lib/src/analytics/_analytics_stub.dart Show resolved Hide resolved

kenzieschmoll reviewed Nov 7, 2022

View reviewed changes

packages/devtools_app/lib/src/screens/profiler/cpu_profile_controller.dart Outdated Show resolved Hide resolved

kenzieschmoll reviewed Nov 7, 2022

View reviewed changes

polina-c added 2 commits November 7, 2022 15:44

Update cpu_profile_controller.dart

b98778c

-

46d07d8

polina-c merged commit 2e84f08 into flutter:master Nov 8, 2022

polina-c deleted the ga branch November 8, 2022 00:52

Configure GA for memory screen. #4705

Configure GA for memory screen. #4705

Uh oh!

Conversation

polina-c commented Nov 4, 2022

Uh oh!

bkonyi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

polina-c Nov 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

polina-c Nov 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

polina-c Nov 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

polina-c Nov 7, 2022 •

edited

Loading

polina-c Nov 7, 2022 •

edited

Loading

polina-c Nov 7, 2022 •

edited

Loading