Add an analytics domain to the analysis server.

Add an analytics domain to the analysis server.

BUG=
R=brianwilkerson@google.com

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

[devoncarew, 2017-06-01 23:43:53]

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

[devoncarew, 2017-06-01 23:43:54]

Add an analytics domain to the analysis server:
- includes tests and docs
- re-gen associated libraries
- the domain is marked experimental for now - it doesn't generate in the spec
docs
- I did not increment the analysis server protocol version; I assume we'll want
to do that when the API is no longer experimental

[Brian Wilkerson, 2017-06-02 15:57:31]

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

https://codereview.chromium.org/2922603002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/domain_analytics.dart:1: // Copyright (c) 2017, the
Dart project authors.  Please see the AUTHORS file
Looks like this file needs to be sorted.

https://codereview.chromium.org/2922603002/diff/1/pkg/analysis_server/tool/sp...
File pkg/analysis_server/tool/spec/spec_input.html (right):

https://codereview.chromium.org/2922603002/diff/1/pkg/analysis_server/tool/sp...
pkg/analysis_server/tool/spec/spec_input.html:2217: The analytics domain
contains APIs related to analytics and usage reporting.
"and"? Makes it sound like analytics reporting and usage reporting are two
separate things.

It would probably be worth writing some text explaining what information clients
should send to server. Not all client authors will understand what analytics can
be used for. Or perhaps adding examples to each request would be sufficient.

It would also be worth indicating that analysis server will send it's own data
(this is in addition to the data it's sending, not everything it's sending) and
explain where the data is going (and whether / how authors of clients can get
information based on the analytics they send us).

https://codereview.chromium.org/2922603002/diff/1/pkg/analysis_server/tool/sp...
pkg/analysis_server/tool/spec/spec_input.html:2224: <p>Whether sending analytics
is enabled or not.</p>
tl;dr spec docs should be complete, not terse

I think this could be confusing because "send" could have two different
interpretations. It could be taken to mean that this indicates whether the
`sendEvent` and `sendTiming` requests can be used (whether analytics should be
sent to server). What it really indicates is whether any analytics information
sent to server will be sent to the cloud. This needs to be clarified.

It should be explained in the docs that the value of the flag can change at any
time (not just via the `enable` request), so clients cannot depend on the value
returned by this request to not change.

We should make it clear that the purpose of this request is to provide an
indication of the value of the flag at a given moment, but that clients are not
required to (and should not) use the value returned to decide whether or not to
send the `sendEvent` and `sendTiming` requests. Those requests should be used
unconditionally and server will determine whether or not it is appropriate to
forward the information to the cloud at the time each request is received.

https://codereview.chromium.org/2922603002/diff/1/pkg/analysis_server/tool/sp...
pkg/analysis_server/tool/spec/spec_input.html:2233: <p>Enable or disable
analytics.</p>
Perhaps something like

-----
Enable or disable the sending of analytics data from the analysis server to
<however we describe where the data is going>. Note that there are other ways
for users to change this setting, so clients cannot assume that they have
complete control over this setting. In particular, there is no guarantee that
the result returned by the <tt>isEnabled</tt> request will match the last value
set via this request.
-----

https://codereview.chromium.org/2922603002/diff/1/pkg/analysis_server/tool/sp...
pkg/analysis_server/tool/spec/spec_input.html:2238: <p>Send information about
client events.</p>
Perhaps we can be more specific.

-----
Ask the analysis server to include the fact that an action was performed in the
client as part of the analytics data being sent. The data will only be included
if the sending of analytics data is enabled at the time the request is
processed. The action that was performed is indicated by the value of the
<tt>action</tt> field.

The value of the action field should not include the identity of the client. The
analytics data sent by server will include the client id passed in using the
<tt>--client-id</tt> command-line argument. The request will be ignored if the
client id was not provided when server was started.
-----

Also, some examples would be useful.

https://codereview.chromium.org/2922603002/diff/1/pkg/analysis_server/tool/sp...
pkg/analysis_server/tool/spec/spec_input.html:2242: <p>The client action
name.</p>
For consistency with the changes above, perhaps

-----
The value used to indicate which action was performed.
-----

https://codereview.chromium.org/2922603002/diff/1/pkg/analysis_server/tool/sp...
pkg/analysis_server/tool/spec/spec_input.html:2247: <p>Send timing information
for client events (e.g. code completions).</p>
Needs similar work.

https://codereview.chromium.org/2922603002/diff/1/pkg/analysis_server/tool/sp...
pkg/analysis_server/tool/spec/spec_input.html:2249: <field name="variable">
Should we keep the name from the underlying API, or would it make more sense to
name this "event"? The latter seems more consistent with "action" and with the
descriptions we're providing.

[devoncarew, 2017-06-02 16:40:53]

Thanks for the review! Agree with all the above :) Will iterate and update the
CL.

[devoncarew, 2017-06-02 20:17:03]

Updated docs, and one question about a review comment.

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

https://codereview.chromium.org/2922603002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/domain_analytics.dart:1: // Copyright (c) 2017, the
Dart project authors.  Please see the AUTHORS file
On 2017/06/02 15:57:30, Brian Wilkerson wrote:
> Looks like this file needs to be sorted.

How so? Do we sort members in this project?

https://codereview.chromium.org/2922603002/diff/1/pkg/analysis_server/tool/sp...
File pkg/analysis_server/tool/spec/spec_input.html (right):

https://codereview.chromium.org/2922603002/diff/1/pkg/analysis_server/tool/sp...
pkg/analysis_server/tool/spec/spec_input.html:2217: The analytics domain
contains APIs related to analytics and usage reporting.
On 2017/06/02 15:57:31, Brian Wilkerson wrote:
> "and"? Makes it sound like analytics reporting and usage reporting are two
> separate things.
> 
> It would probably be worth writing some text explaining what information
clients
> should send to server. Not all client authors will understand what analytics
can
> be used for. Or perhaps adding examples to each request would be sufficient.
> 
> It would also be worth indicating that analysis server will send it's own data
> (this is in addition to the data it's sending, not everything it's sending)
and
> explain where the data is going (and whether / how authors of clients can get
> information based on the analytics they send us).

Updated with some verbiage.

https://codereview.chromium.org/2922603002/diff/1/pkg/analysis_server/tool/sp...
pkg/analysis_server/tool/spec/spec_input.html:2249: <field name="variable">
On 2017/06/02 15:57:31, Brian Wilkerson wrote:
> Should we keep the name from the underlying API, or would it make more sense
to
> name this "event"? The latter seems more consistent with "action" and with the
> descriptions we're providing.

'variable' is what it's called in the google analytics API, but I think in
context 'event' would make more sense.

[Brian Wilkerson, 2017-06-02 20:38:57]

lgtm!

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

https://codereview.chromium.org/2922603002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/domain_analytics.dart:1: // Copyright (c) 2017, the
Dart project authors.  Please see the AUTHORS file
> How so? Do we sort members in this project?

Yes, we use "Code > Sort Members in Dart File" for all of the analyzer packages
(analysis_server, analyzer, analyzer_plugin, and analyzer_cli) as well as a few
others.

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

https://codereview.chromium.org/2922603002/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/domain_analytics.dart:39: // TODO(devoncarew:
Implement.
You're also missing some closing paren's, if you care.

https://codereview.chromium.org/2922603002/diff/20001/pkg/analysis_server/too...
File pkg/analysis_server/tool/spec/spec_input.html (right):

https://codereview.chromium.org/2922603002/diff/20001/pkg/analysis_server/too...
pkg/analysis_server/tool/spec/spec_input.html:2242: the cloud. If disabled, the
analysis server doesn not send any analytics
"doesn" --> "does"

[devoncarew, 2017-06-02 20:45:36]

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

https://codereview.chromium.org/2922603002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/domain_analytics.dart:1: // Copyright (c) 2017, the
Dart project authors.  Please see the AUTHORS file
On 2017/06/02 20:38:56, Brian Wilkerson wrote:
> > How so? Do we sort members in this project?
> 
> Yes, we use "Code > Sort Members in Dart File" for all of the analyzer
packages
> (analysis_server, analyzer, analyzer_plugin, and analyzer_cli) as well as a
few
> others.

Done.

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

https://codereview.chromium.org/2922603002/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/domain_analytics.dart:39: // TODO(devoncarew:
Implement.
On 2017/06/02 20:38:57, Brian Wilkerson wrote:
> You're also missing some closing paren's, if you care.

Done.

https://codereview.chromium.org/2922603002/diff/20001/pkg/analysis_server/too...
File pkg/analysis_server/tool/spec/spec_input.html (right):

https://codereview.chromium.org/2922603002/diff/20001/pkg/analysis_server/too...
pkg/analysis_server/tool/spec/spec_input.html:2242: the cloud. If disabled, the
analysis server doesn not send any analytics
On 2017/06/02 20:38:57, Brian Wilkerson wrote:
> "doesn" --> "does"

Done.

[devoncarew, 2017-06-02 21:38:56]

Description was changed from

==========
Add an analytics domain to the analysis server.

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

to

==========
Add an analytics domain to the analysis server.

BUG=
R=brianwilkerson@google.com

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

[devoncarew, 2017-06-02 21:38:58]

Committed patchset #3 (id:40001) manually as
511963d71509de50ac577ac22f3bac8d33c17e34 (presubmit successful).