Add instrumentation data to the analysis server diagnostics page.

Add instrumentation data to the analysis server diagnostics page.

BUG=
R=brianwilkerson@google.com

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

[devoncarew, 2017-02-27 04:55:18]

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

[devoncarew, 2017-02-27 04:55:19]

- add instrumentation data to the analysis server diagnostics page
- when we get a request for '/', redirect the browser to '/status'

[skybrian, 2017-02-27 05:31:46]

Seems fine if the analyzer team is okay with it.

https://codereview.chromium.org/2717253002/diff/1/pkg/analysis_server/lib/src...
File pkg/analysis_server/lib/src/status/get_handler2.dart (right):

https://codereview.chromium.org/2717253002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/status/get_handler2.dart:1117:
buffer.write(instrumentationServer.describe.replaceAll('\n', '<br>'));
If there is a link in the instrumentation server description, it might be nice
to put <a href> around it.

[Brian Wilkerson, 2017-02-27 14:41:31]

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

[Brian Wilkerson, 2017-02-27 14:41:32]

lgtm

https://codereview.chromium.org/2717253002/diff/1/pkg/analysis_server/lib/src...
File pkg/analysis_server/lib/src/status/get_handler2.dart (right):

https://codereview.chromium.org/2717253002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/status/get_handler2.dart:145:
_returnRedirect(request, STATUS_PATH);
Out of curiosity (because I don't know much about web programming) what is the
advantage of redirecting rather than just returning the same data?

https://codereview.chromium.org/2717253002/diff/1/pkg/analyzer/lib/instrument...
File pkg/analyzer/lib/instrumentation/file_instrumentation.dart (right):

https://codereview.chromium.org/2717253002/diff/1/pkg/analyzer/lib/instrument...
pkg/analyzer/lib/instrumentation/file_instrumentation.dart:28: String get
describe => "file: $filePath, session $sessionId";
Not sure it's useful to print the session id given that it's always an empty
string.

https://codereview.chromium.org/2717253002/diff/1/pkg/analyzer/lib/instrument...
File pkg/analyzer/lib/instrumentation/instrumentation.dart (right):

https://codereview.chromium.org/2717253002/diff/1/pkg/analyzer/lib/instrument...
pkg/analyzer/lib/instrumentation/instrumentation.dart:33: String get describe =>
'${toString()}, session $sessionId';
Personally, I would create a method other than `toString` (`destination`?) to
invoke here, then override that method in most subclasses (rather than
`describe`). (Doesn't apply to MulticastInstrumentationServer, which still needs
to override `describe`.)

https://codereview.chromium.org/2717253002/diff/1/pkg/analyzer/test/instrumen...
File pkg/analyzer/test/instrumentation/instrumentation_test.dart (right):

https://codereview.chromium.org/2717253002/diff/1/pkg/analyzer/test/instrumen...
pkg/analyzer/test/instrumentation/instrumentation_test.dart:169: String get
describe => 'test instrumentation, session $sessionId';
Always an empty string.

[devoncarew, 2017-03-01 01:51:33]

some tweaks - I think the comments are mostly addressed; ptal

https://codereview.chromium.org/2717253002/diff/1/pkg/analysis_server/lib/src...
File pkg/analysis_server/lib/src/status/get_handler2.dart (right):

https://codereview.chromium.org/2717253002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/status/get_handler2.dart:145:
_returnRedirect(request, STATUS_PATH);
On 2017/02/27 14:41:32, Brian Wilkerson wrote:
> Out of curiosity (because I don't know much about web programming) what is the
> advantage of redirecting rather than just returning the same data?

This will update the url in the browser (to say '/status'). No huge benefit, but
will give the user a bit more sense of where they are.

https://codereview.chromium.org/2717253002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/status/get_handler2.dart:1117:
buffer.write(instrumentationServer.describe.replaceAll('\n', '<br>'));
On 2017/02/27 05:31:46, skybrian wrote:
> If there is a link in the instrumentation server description, it might be nice
> to put <a href> around it.

I think I'm just going to let implementations pass raw html through - it can be
understood that the `describe` output may be used in a web page; not sure it's
worth more effort here.

https://codereview.chromium.org/2717253002/diff/1/pkg/analyzer/lib/instrument...
File pkg/analyzer/lib/instrumentation/file_instrumentation.dart (right):

https://codereview.chromium.org/2717253002/diff/1/pkg/analyzer/lib/instrument...
pkg/analyzer/lib/instrumentation/file_instrumentation.dart:28: String get
describe => "file: $filePath, session $sessionId";
On 2017/02/27 14:41:32, Brian Wilkerson wrote:
> Not sure it's useful to print the session id given that it's always an empty
> string.

updated to not use session

https://codereview.chromium.org/2717253002/diff/1/pkg/analyzer/lib/instrument...
File pkg/analyzer/lib/instrumentation/instrumentation.dart (right):

https://codereview.chromium.org/2717253002/diff/1/pkg/analyzer/lib/instrument...
pkg/analyzer/lib/instrumentation/instrumentation.dart:33: String get describe =>
'${toString()}, session $sessionId';
On 2017/02/27 14:41:32, Brian Wilkerson wrote:
> Personally, I would create a method other than `toString` (`destination`?) to
> invoke here, then override that method in most subclasses (rather than
> `describe`). (Doesn't apply to MulticastInstrumentationServer, which still
needs
> to override `describe`.)

I changed this to a purely abstract method (String get describe);
implementations will need to fill it in (it seems like the session id was not
used for most instrumentation server implementations).

https://codereview.chromium.org/2717253002/diff/1/pkg/analyzer/test/instrumen...
File pkg/analyzer/test/instrumentation/instrumentation_test.dart (right):

https://codereview.chromium.org/2717253002/diff/1/pkg/analyzer/test/instrumen...
pkg/analyzer/test/instrumentation/instrumentation_test.dart:169: String get
describe => 'test instrumentation, session $sessionId';
On 2017/02/27 14:41:32, Brian Wilkerson wrote:
> Always an empty string.

updated to not use session

[skybrian, 2017-03-01 02:06:01]

https://codereview.chromium.org/2717253002/diff/20001/pkg/analyzer/lib/instru...
File pkg/analyzer/lib/instrumentation/file_instrumentation.dart (right):

https://codereview.chromium.org/2717253002/diff/20001/pkg/analyzer/lib/instru...
pkg/analyzer/lib/instrumentation/file_instrumentation.dart:28: String get
describe => "file: $filePath";
HTML-escape the path (just in case).

https://codereview.chromium.org/2717253002/diff/20001/pkg/analyzer/lib/instru...
File pkg/analyzer/lib/instrumentation/instrumentation.dart (right):

https://codereview.chromium.org/2717253002/diff/20001/pkg/analyzer/lib/instru...
pkg/analyzer/lib/instrumentation/instrumentation.dart:32: */
Possible security issue here (or maybe just a broken web page).

Since the UI is going to display HTML unescaped, the dartdoc should say so here,
and to be really clear, I'd recommend renaming the method to something like
describeAsHtml as well.

https://codereview.chromium.org/2717253002/diff/20001/pkg/analyzer/lib/instru...
pkg/analyzer/lib/instrumentation/instrumentation.dart:383: .join("\n");
<br>

[skybrian, 2017-03-01 02:07:47]

https://codereview.chromium.org/2717253002/diff/20001/pkg/analyzer/lib/instru...
File pkg/analyzer/lib/instrumentation/instrumentation.dart (right):

https://codereview.chromium.org/2717253002/diff/20001/pkg/analyzer/lib/instru...
pkg/analyzer/lib/instrumentation/instrumentation.dart:383: .join("\n");
On 2017/03/01 02:06:01, skybrian wrote:
> <br>

(Or maybe surround each item with <div>?)

[devoncarew, 2017-03-01 02:30:27]

https://codereview.chromium.org/2717253002/diff/20001/pkg/analyzer/lib/instru...
File pkg/analyzer/lib/instrumentation/instrumentation.dart (right):

https://codereview.chromium.org/2717253002/diff/20001/pkg/analyzer/lib/instru...
pkg/analyzer/lib/instrumentation/instrumentation.dart:32: */
On 2017/03/01 02:06:01, skybrian wrote:
> Possible security issue here (or maybe just a broken web page).
> 
> Since the UI is going to display HTML unescaped, the dartdoc should say so
here,
> and to be really clear, I'd recommend renaming the method to something like
> describeAsHtml as well.

Hmm, I think I'll move to escaping the describe results here, and possibly
looking to auto-convert http: references as well...

[Brian Wilkerson, 2017-03-01 15:05:03]

still lgtm

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

https://codereview.chromium.org/2717253002/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/status/get_handler2.dart:1118:
buffer.write(description.replaceAll('\n', '<br>'));
nit: Could we combine this with the data on line 1101 (perhaps putting the
description after 'active')?

https://codereview.chromium.org/2717253002/diff/20001/pkg/analyzer/lib/instru...
File pkg/analyzer/lib/instrumentation/file_instrumentation.dart (right):

https://codereview.chromium.org/2717253002/diff/20001/pkg/analyzer/lib/instru...
pkg/analyzer/lib/instrumentation/file_instrumentation.dart:28: String get
describe => "file: $filePath";
> HTML-escape the path (just in case).

Safer to escape the result of describe in the GetHandler.

[devoncarew, 2017-03-10 22:04:31]

A bit of a delay, but landing this shortly. I've html escaped the descriptions
and did add code to convert http references to html anchors.

[devoncarew, 2017-03-10 22:08:19]

Description was changed from

==========
Add instrumentation data to the analysis server diagnostics page.

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

to

==========
Add instrumentation data to the analysis server diagnostics page.

BUG=
R=brianwilkerson@google.com

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

[devoncarew, 2017-03-10 22:08:21]

Committed patchset #3 (id:40001) manually as
8f3666993742ff4a1af5486ffc2d4213feed1760 (presubmit successful).