Add analytics to analyzer-cli and analysis server.

Add analytics to analyzer-cli and analysis server.

BUG=
R=brianwilkerson@google.com, scheglov@google.com, zra@google.com

Committed: https://github.com/dart-lang/sdk/commit/d4abd49df7466b795988b28b9abfd5ebee35ee49

[devoncarew, 2017-06-30 18:00:09]

devoncarew@google.com changed reviewers:
+ brianwilkerson@google.com, scheglov@google.com

[devoncarew, 2017-06-30 18:00:09]

Add analytics to analyzer-cli and analysis server.

(I may still have some open work left to do to disable for analysis server
integration tests)

[Brian Wilkerson, 2017-06-30 18:51:22]

lgtm

https://codereview.chromium.org/2963323002/diff/1/pkg/analysis_server/lib/src...
File pkg/analysis_server/lib/src/domain_analysis.dart (right):

https://codereview.chromium.org/2963323002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/domain_analysis.dart:251:
server.options.analytics.sendEvent('analysis', 'reanalyze');
For consistency it might be useful to make all of the request implementation
methods follow a similar pattern, such as: extract parameters from the request,
send analytics, respond to the request.

https://codereview.chromium.org/2963323002/diff/1/pkg/analyzer_cli/analysis_o...
File pkg/analyzer_cli/analysis_options.yaml (right):

https://codereview.chromium.org/2963323002/diff/1/pkg/analyzer_cli/analysis_o...
pkg/analyzer_cli/analysis_options.yaml:7: - directives_ordering
I'm not sure how useful this is, given that we always use the sorter to sort
everything in all our packages (which also sorts the directives).

https://codereview.chromium.org/2963323002/diff/1/pkg/analyzer_cli/lib/src/dr...
File pkg/analyzer_cli/lib/src/driver.dart (right):

https://codereview.chromium.org/2963323002/diff/1/pkg/analyzer_cli/lib/src/dr...
pkg/analyzer_cli/lib/src/driver.dart:88: /// Public for testing.
Please use the annotation `visibleForTesting` (from the 'meta' package).

https://codereview.chromium.org/2963323002/diff/1/pkg/analyzer_cli/lib/src/dr...
pkg/analyzer_cli/lib/src/driver.dart:144: /// [isTesting] is true is we're
running in a test environment.
"true is" --> "true if"

[devoncarew, 2017-06-30 21:58:14]

Updated for review comments. Also, I added some code to allow us to suppress
analytics for the current session, and hooked that up to the analysis server
integration tests.

Also, some additional work around the analyzer cli's Driver class and the
analysis server shutdown request to better handle termination (the analytics
requests will often leave requests lingering, and we don't want to delay
shutdown of the analyzer CLI just for those requests).

https://codereview.chromium.org/2963323002/diff/1/pkg/analysis_server/lib/src...
File pkg/analysis_server/lib/src/domain_analysis.dart (right):

https://codereview.chromium.org/2963323002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/domain_analysis.dart:251:
server.options.analytics.sendEvent('analysis', 'reanalyze');
On 2017/06/30 18:51:22, Brian Wilkerson wrote:
> For consistency it might be useful to make all of the request implementation
> methods follow a similar pattern, such as: extract parameters from the
request,
> send analytics, respond to the request.

Sounds good! I may implement in a follow-up CL. It would be nice to refactor a
bit so the domain is always available (as a field on AbstractRequestHandler, or
a getter on Request).

https://codereview.chromium.org/2963323002/diff/1/pkg/analyzer_cli/analysis_o...
File pkg/analyzer_cli/analysis_options.yaml (right):

https://codereview.chromium.org/2963323002/diff/1/pkg/analyzer_cli/analysis_o...
pkg/analyzer_cli/analysis_options.yaml:7: - directives_ordering
On 2017/06/30 18:51:22, Brian Wilkerson wrote:
> I'm not sure how useful this is, given that we always use the sorter to sort
> everything in all our packages (which also sorts the directives).

Removed (it didn't catch anything in this package).

https://codereview.chromium.org/2963323002/diff/1/pkg/analyzer_cli/lib/src/dr...
File pkg/analyzer_cli/lib/src/driver.dart (right):

https://codereview.chromium.org/2963323002/diff/1/pkg/analyzer_cli/lib/src/dr...
pkg/analyzer_cli/lib/src/driver.dart:88: /// Public for testing.
On 2017/06/30 18:51:22, Brian Wilkerson wrote:
> Please use the annotation `visibleForTesting` (from the 'meta' package).

Updated!

https://codereview.chromium.org/2963323002/diff/1/pkg/analyzer_cli/lib/src/dr...
pkg/analyzer_cli/lib/src/driver.dart:144: /// [isTesting] is true is we're
running in a test environment.
On 2017/06/30 18:51:22, Brian Wilkerson wrote:
> "true is" --> "true if"

Done.

[devoncarew, 2017-06-30 21:59:21]

devoncarew@google.com changed reviewers:
+ zra@google.com

[devoncarew, 2017-06-30 21:59:22]

Zach, I added a gn file for one new package, and updated two others. Can you
review? I have not modified the gn files in the sdk repo before.

[zra, 2017-06-30 22:06:53]

GN lgtm

[Brian Wilkerson, 2017-06-30 22:52:17]

lgtm

https://codereview.chromium.org/2963323002/diff/20001/pkg/analysis_server/lib...
File pkg/analysis_server/lib/src/analysis_server.dart (right):

https://codereview.chromium.org/2963323002/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/analysis_server.dart:917: await new Future(() {
We don't currently wait. Are you sure we should?

[scheglov, 2017-06-30 23:36:30]

https://codereview.chromium.org/2963323002/diff/20001/pkg/analysis_server/lib...
File pkg/analysis_server/lib/src/analysis_server.dart (right):

https://codereview.chromium.org/2963323002/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/analysis_server.dart:913: Future shutdown() async {
Future<Null>

https://codereview.chromium.org/2963323002/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/analysis_server.dart:1070: void addMocks() {
Do we create and assign these objects just to avoid checks for null?
Why not create them in constructor to avoid requirement for every client to call
addMocks()?

https://codereview.chromium.org/2963323002/diff/20001/pkg/analysis_server/tes...
File pkg/analysis_server/test/domain_server_test.dart (right):

https://codereview.chromium.org/2963323002/diff/20001/pkg/analysis_server/tes...
pkg/analysis_server/test/domain_server_test.dart:85: await new
Future.delayed(new Duration(seconds: 1));
Please don't add unconditional delays.
We run these these tests many times each days, and waiting the whole second is
not fun :-(

It is also not obvious why we should wait at all.
The comment does not explain this :-(

[devoncarew, 2017-07-01 22:18:05]

Reworked slightly:

- we now have a way to close any in-progress analytics requests, so no longer
need to do explicit calls to exit() on termination
- the analytics and crashReporting objects are null for most tests, and places
which reference those object use null-aware operators
- the server.shutdown call is now async; it returns after any analytics calls
have finished (or after we've closed in-progress calls)

https://codereview.chromium.org/2963323002/diff/20001/pkg/analysis_server/lib...
File pkg/analysis_server/lib/src/analysis_server.dart (right):

https://codereview.chromium.org/2963323002/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/analysis_server.dart:913: Future shutdown() async {
On 2017/06/30 23:36:29, scheglov wrote:
> Future<Null>

Done.

https://codereview.chromium.org/2963323002/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/analysis_server.dart:917: await new Future(() {
On 2017/06/30 22:52:17, Brian Wilkerson wrote:
> We don't currently wait. Are you sure we should?

I've removed the await (and added the ability to close any active analytics
requests).

https://codereview.chromium.org/2963323002/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/analysis_server.dart:1070: void addMocks() {
On 2017/06/30 23:36:29, scheglov wrote:
> Do we create and assign these objects just to avoid checks for null?
> Why not create them in constructor to avoid requirement for every client to
call
> addMocks()?

I removed this method, and instead had all the places that dereference the
analytics or crashReportSender objects use null aware operators (and added some
dartdoc for those fields).

https://codereview.chromium.org/2963323002/diff/20001/pkg/analysis_server/tes...
File pkg/analysis_server/test/domain_server_test.dart (right):

https://codereview.chromium.org/2963323002/diff/20001/pkg/analysis_server/tes...
pkg/analysis_server/test/domain_server_test.dart:85: await new
Future.delayed(new Duration(seconds: 1));
On 2017/06/30 23:36:30, scheglov wrote:
> Please don't add unconditional delays.
> We run these these tests many times each days, and waiting the whole second is
> not fun :-(
> 
> It is also not obvious why we should wait at all.
> The comment does not explain this :-(

This has been re-worked to not use a delay.

[scheglov, 2017-07-02 00:59:22]

lgtm

[Brian Wilkerson, 2017-07-02 15:54:26]

lgtm

https://codereview.chromium.org/2963323002/diff/60001/pkg/analysis_server/lib...
File pkg/analysis_server/lib/src/analysis_server.dart (right):

https://codereview.chromium.org/2963323002/diff/60001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/analysis_server.dart:1064: /// The analytics
instance; note, this object can be `null`, and should be
Perhaps use a doc comment style consistent with existing code?

https://codereview.chromium.org/2963323002/diff/60001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/analysis_server.dart:1072: // IDE options
This should have been a doc comment (one disadvantage of using '///'; it's too
easy to miss a slash). But it clearly isn't in the right place anymore. It
probably belongs on the enclosing class. Would you mind moving and fixing it (or
deleting it given that it doesn't really add any value)?

https://codereview.chromium.org/2963323002/diff/60001/pkg/analyzer_cli/lib/sr...
File pkg/analyzer_cli/lib/src/driver.dart (right):

https://codereview.chromium.org/2963323002/diff/60001/pkg/analyzer_cli/lib/sr...
pkg/analyzer_cli/lib/src/driver.dart:87: void
setAnalyticsMock(telemetry.Analytics replacementAnalytics) {
Remove "Mock"? I don't think it's required that the argument be a mock object.

[devoncarew, 2017-07-04 02:35:48]

https://codereview.chromium.org/2963323002/diff/60001/pkg/analysis_server/lib...
File pkg/analysis_server/lib/src/analysis_server.dart (right):

https://codereview.chromium.org/2963323002/diff/60001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/analysis_server.dart:1064: /// The analytics
instance; note, this object can be `null`, and should be
On 2017/07/02 15:54:26, Brian Wilkerson wrote:
> Perhaps use a doc comment style consistent with existing code?

Done.

https://codereview.chromium.org/2963323002/diff/60001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/analysis_server.dart:1072: // IDE options
On 2017/07/02 15:54:26, Brian Wilkerson wrote:
> This should have been a doc comment (one disadvantage of using '///'; it's too
> easy to miss a slash). But it clearly isn't in the right place anymore. It
> probably belongs on the enclosing class. Would you mind moving and fixing it
(or
> deleting it given that it doesn't really add any value)?

Sure - moved to the AnalysisServerOptions class.

https://codereview.chromium.org/2963323002/diff/60001/pkg/analyzer_cli/lib/sr...
File pkg/analyzer_cli/lib/src/driver.dart (right):

https://codereview.chromium.org/2963323002/diff/60001/pkg/analyzer_cli/lib/sr...
pkg/analyzer_cli/lib/src/driver.dart:87: void
setAnalyticsMock(telemetry.Analytics replacementAnalytics) {
On 2017/07/02 15:54:26, Brian Wilkerson wrote:
> Remove "Mock"? I don't think it's required that the argument be a mock object.

Makes sense - done.

[devoncarew, 2017-07-06 06:29:06]

Description was changed from

==========
Add analytics to analyzer-cli and analysis server.

BUG=
==========

to

==========
Add analytics to analyzer-cli and analysis server.

BUG=
R=brianwilkerson@google.com, scheglov@google.com, zra@google.com

Committed:
https://github.com/dart-lang/sdk/commit/d4abd49df7466b795988b28b9abfd5ebee35ee49
==========

[devoncarew, 2017-07-06 06:29:08]

Committed patchset #5 (id:80001) manually as
d4abd49df7466b795988b28b9abfd5ebee35ee49 (presubmit successful).