Add more analytics #5985

stevemessick · 2022-02-18T23:35:30Z

Needs testing, and tests. Some basic work is still not done, too.

flutter-idea/src/io/flutter/analytics/FlutterAnalysisServerListener.java

stevemessick · 2022-03-14T23:41:42Z

This is ready for review.

Note that due to changes in Testing there will be compilation errors when using any IntelliJ older than EAP. The Dart plugin API that is being used is apparently only available in the EAP builds.

jacob314 · 2022-03-25T18:38:33Z

flutter-idea/src/io/flutter/analytics/FlutterAnalysisServerListener.java

+  }
+
+  @Override
+  public void computedErrors(String path, List<AnalysisError> list) {


is this getting called for every file in a project?
If it is called for more than each root path, I'm concerned we are logging too much.

Not every file, but all Dart files plus some unexpected files. All three AndroidManifest.xml files were checked plus pubspec.yaml. Also analysis_options.yaml.

jacob314 · 2022-03-25T18:48:53Z

flutter-idea/src/io/flutter/analytics/FlutterAnalysisServerListener.java

+    FlutterInitializer.getAnalytics().sendTiming(E2E_IJ_COMPLETION_TIME, FAILURE, e2eCompletionMS); // test: logE2ECompletionErrorMS()
+  }
+
+  private void logAnalysisError(@Nullable AnalysisError error) {


I'm concerned this is logging too much. If you have 10,000 errors, we are still trying to log 100 events.
What are we trying to achieve by logging the error codes?

Actually, we log 100 events plus the analysis time of every error in an open editor. So, that's potentially worse. We'd have to check with @jwren for design rationale.

We need to log far fewer events. Logging analytics needs to not be the reason why users have poor performance or complain our tools use too much of their bandwidth. I would suggest changing all these analytics so a single analytic event or a couple of events are logged summarizing the state of all errors reported rather than ever emitting events proportional to the number of errors or files.

I think this will be straight-forward to change. I'll drop the call to logAnalysisError for each error and clean up unused elements. We already log the summary info in serverStatus.

jacob314 · 2022-03-25T18:52:31Z

flutter-idea/src/io/flutter/analytics/FlutterAnalysisServerListener.java

+  public void computedErrors(String path, List<AnalysisError> list) {
+    assert list != null;
+    list.forEach(this::logAnalysisError);
+    pathToErrors.put(path, list);


what happens if you restart the analysis server? Do we get a new event with new errors.
Wish we had a merged Dart and Flutter plugin so we didn't have to duplicate this logic that the analysis errors window already handles.

Yes. It would make sense to check if the file had already been analyzed and, if the errors are the same, ignore it, I think.

I added that test and asked for an opinion from jwren.

No right choice. We can discuss, there are pros and cons for both (as with all things). If you don't send the information you won't have the signal that users are re-analyzing which is a signal in-itself, but if you do send the information there will be fuller logs with the same information.

jacob314

A few minor comments then LGTM.
Main thing I'm worried about is ensuring we don't start logging too much and cause performance problems particularly for c cases where an IDE is already struggling with 1000s of errors in the analysis errors view.

stevemessick · 2022-03-25T22:31:23Z

@jacob314 My main concern is that we are doing a lot of work that 90% of our users won't see for a year. The new Dart API is (or was -- I have not checked this week) only present in the EAP version of the Dart plugin.

BTW you have to click the Approve button now. LGTM in comments is no longer sufficient to merge. No hurry, I have not yet looked into your questions.

stevemessick · 2022-03-25T23:31:07Z

flutter-idea/src/io/flutter/analytics/FlutterAnalysisServerListener.java

+  public void computedErrors(String path, List<AnalysisError> list) {
+    assert list != null;
+    list.forEach(this::logAnalysisError);
+    pathToErrors.put(path, list);


Yes. It would make sense to check if the file had already been analyzed and, if the errors are the same, ignore it, I think.

stevemessick · 2022-03-25T23:33:53Z

flutter-idea/src/io/flutter/analytics/FlutterAnalysisServerListener.java

+    FlutterInitializer.getAnalytics().sendTiming(E2E_IJ_COMPLETION_TIME, FAILURE, e2eCompletionMS); // test: logE2ECompletionErrorMS()
+  }
+
+  private void logAnalysisError(@Nullable AnalysisError error) {


Actually, we log 100 events plus the analysis time of every error in an open editor. So, that's potentially worse. We'd have to check with @jwren for design rationale.

stevemessick · 2022-03-28T21:13:08Z

flutter-idea/src/io/flutter/analytics/FlutterAnalysisServerListener.java

+  public void computedErrors(String path, List<AnalysisError> list) {
+    assert list != null;
+    List<AnalysisError> existing = pathToErrors.get(path);
+    if (existing != null && existing.equals(list)) {


@jwren Do you think this test makes sense? And will it be expensive?

jacob314

stevemessick · 2022-04-11T21:31:08Z

@jacob314 @jwren After looking at the log file and seeing just how much stuff was getting sent I decided to throttle everything to one transmission per minute. The cumulative error counts are sent when the project is closed, and every two hours, for back-end percentile analysis. Both of those intervals can be adjusted.

.idea/runConfigurations/flutter_idea__runIde_.xml

flutter-idea/src/io/flutter/analytics/FlutterAnalysisServerListener.java

jacob314 · 2022-04-11T23:11:23Z

flutter-idea/src/io/flutter/analytics/FlutterAnalysisServerListener.java

+      if (lintCount > 0) {
+        analytics.sendEventMetric(DAS_STATUS_EVENT_TYPE, LINTS, lintCount); // test: serverStatus()
+      }
+      errorCount = warningCount = hintCount = lintCount = 0;


Why do we zero out the counts? This seems wrong. I would expect these #s should match the # of errors reported in the analysis server window. As is, I'm not clear how I would interpret these #s.

They are accumulated while analysis is active, then sent when analysis is complete. They need to be zeroed so the accumulated values are accurate.

jacob314 · 2022-04-11T23:16:28Z

flutter-idea/src/io/flutter/analytics/FlutterAnalysisServerListener.java


  void logE2ECompletionSuccessMS(long e2eCompletionMS) {
-    FlutterInitializer.getAnalytics().sendTiming(E2E_IJ_COMPLETION_TIME, SUCCESS, e2eCompletionMS); // test: logE2ECompletionSuccessMS()
+    maybeReport(true, (analytics) -> {


For the completion time, rather than throttling, what you could alternately report a single metric on intellij close that reports the P50, P90, and P95 times for the entire session.
That would make the completion time numbers less noisy than filtering to only report 1 completion per minute.

See the comment below for why we cannot do it when exiting.

flutter-idea/src/io/flutter/analytics/FlutterAnalysisServerListener.java

jacob314 · 2022-04-11T23:21:07Z

flutter-idea/src/io/flutter/analytics/FlutterAnalysisServerListener.java

+      if (IS_TESTING) {
+        errorCount = warningCount = hintCount = lintCount = 1;
+      }
+      maybeReportErrorCounts();


have you manually verified that these #s match what the analyzer window shows?

It's been a while, but yes.

jacob314 · 2022-04-11T23:27:56Z

Rather than rate limiting, I think we should emit summary statistics when the IntelliJ session is closed. The reason is that just rate limiting like this can cause the data to be a bit noisy and skewed.
For example: right now we'd over sample the very first autocompletion returned right when a user starts typing which might not be representative of autocompletions when users are in the middle of typing. Imagine a user that types for a bit then pauses for a minute to run a build or read code.

Example summary statistics format that I think would work better:

{
  event: 'autocompleteE2E',
  P50Time: 73,
  P90Time: 350,
  P95Time: 700 ,
  count: 2000,
}

This indicates that the user had 2000 autocomplete events with a P50 time of 73, P90 time of 350, and P95 time of 700.
That way few events are sent to analytics but there is enough data to compute P50, P90, or P95 times across users.
Fyi @jwren who might have some ideas based on how similar problems have been solved in g3.

stevemessick · 2022-04-12T16:07:48Z

We can't do all the computation and reporting at exit. IntelliJ enforces a strict, limited time for exit processing (i.e. project close). We can't know what else is going on, so relying on it would potentially limit the data we would collect.

stevemessick · 2022-04-12T21:12:48Z

how similar problems have been solved in g3

Percentiles are computed on the server. The same analytic events are used here as are used there.

My statistics is rusty, and my stats book doesn't even mention this, but I'm not sure you'd get the same percentiles if you tried combining percentiles from samples rather than computing the population percentiles on the server.

jwren

LGTM

jacob314 · 2022-04-14T16:10:47Z

We can't do all the computation and reporting at exit. IntelliJ enforces a strict, limited time for exit processing (i.e. project close). We can't know what else is going on, so relying on it would potentially limit the data we would collect.

Ok I'm ok with sampling for most things instead. Perhaps I'm being too conservative on how many analytics events we should send.

jacob314 · 2022-04-14T16:13:41Z

I agree the median of the median is not the same as the median of all the data but in some ways it is the metric we really want.
For metrics like this what I'm looking for is something I can conceptualize.
For example, the statement I want to make is that for 90% of our users, 90% of their completions are less than 200ms. Framed like that, there is no harm in aggregating locally as it is actually exactly what we want.

jacob314 · 2022-04-14T16:14:24Z

Lets land this and perhaps iterate on summarizing some of the metrics.

stevemessick requested review from jacob314 and jwren February 21, 2022 18:20

jacob314 reviewed Mar 1, 2022

View reviewed changes

flutter-idea/src/io/flutter/analytics/FlutterAnalysisServerListener.java Outdated Show resolved Hide resolved

stevemessick force-pushed the more-analytics branch from 26dd4f0 to 155616f Compare March 2, 2022 18:26

stevemessick force-pushed the more-analytics branch from 155616f to fb8ae55 Compare March 10, 2022 19:44

stevemessick changed the title ~~[WIP] Add more analytics~~ Add more analytics Mar 14, 2022

stevemessick mentioned this pull request Mar 24, 2022

Plugin tool updates #6054

Closed

8 tasks

jacob314 reviewed Mar 25, 2022

View reviewed changes

stevemessick force-pushed the more-analytics branch 2 times, most recently from 132f677 to fdacbf5 Compare March 28, 2022 21:09

stevemessick commented Mar 28, 2022

View reviewed changes

stevemessick force-pushed the more-analytics branch from 709f05c to e699f85 Compare April 6, 2022 23:55

jacob314 approved these changes Apr 8, 2022

View reviewed changes

stevemessick added 3 commits April 8, 2022 12:01

Recreate branch due to git pilot error

672cc2c

Fix syntax error

09a2c27

Add throttling to high frequency reports

8f372a6

stevemessick force-pushed the more-analytics branch from a38996f to 8f372a6 Compare April 11, 2022 21:16

stevemessick added 2 commits April 11, 2022 14:25

Fix syntax for older IntelliJ

4e255d3

ditto

6b1b264

Fix typo

8736a49

jacob314 reviewed Apr 11, 2022

View reviewed changes

.idea/runConfigurations/flutter_idea__runIde_.xml Outdated Show resolved Hide resolved

jacob314 reviewed Apr 11, 2022

View reviewed changes

flutter-idea/src/io/flutter/analytics/FlutterAnalysisServerListener.java Outdated Show resolved Hide resolved

jacob314 reviewed Apr 11, 2022

View reviewed changes

flutter-idea/src/io/flutter/analytics/FlutterAnalysisServerListener.java Show resolved Hide resolved

jacob314 reviewed Apr 11, 2022

View reviewed changes

flutter-idea/src/io/flutter/analytics/FlutterAnalysisServerListener.java Show resolved Hide resolved

jacob314 reviewed Apr 11, 2022

View reviewed changes

Typo

68d072c

jwren approved these changes Apr 12, 2022

View reviewed changes

Merge branch 'master' into more-analytics

3d6e2bb

stevemessick merged commit d4072cf into master Apr 14, 2022

stevemessick deleted the more-analytics branch April 14, 2022 16:34

Add more analytics #5985

Add more analytics #5985

Uh oh!

Conversation

stevemessick commented Feb 18, 2022

Uh oh!

Uh oh!

stevemessick commented Mar 14, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevemessick Mar 25, 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jacob314 left a comment

Choose a reason for hiding this comment

Uh oh!

stevemessick commented Mar 25, 2022

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!

jacob314 left a comment

Choose a reason for hiding this comment

Uh oh!

stevemessick commented Apr 11, 2022

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jacob314 commented Apr 11, 2022

Uh oh!

stevemessick commented Apr 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stevemessick commented Apr 12, 2022

Uh oh!

jwren left a comment

Choose a reason for hiding this comment

Uh oh!

jacob314 commented Apr 14, 2022

Uh oh!

jacob314 commented Apr 14, 2022

Uh oh!

jacob314 commented Apr 14, 2022

stevemessick Mar 25, 2022 •

edited

Loading

stevemessick commented Apr 12, 2022 •

edited

Loading