Prototype of I/O statistics for Observatory

Socket I/O statistics for Observatory

This is the first part of adding I/O statistics to the observatory.

R=ajohnsen@google.com, johnmccutchan@google.com

Committed: https://code.google.com/p/dart/source/detail?r=33366

[Cutch, 2014-02-13 22:42:09]

https://codereview.chromium.org/163903002/diff/1/runtime/bin/socket_patch.dart
File runtime/bin/socket_patch.dart (right):

https://codereview.chromium.org/163903002/diff/1/runtime/bin/socket_patch.dar...
runtime/bin/socket_patch.dart:226: sockets.forEach((s) {
You probably should use the dart:convert library instead of serializing the JSON
yourself.

https://codereview.chromium.org/163903002/diff/1/runtime/bin/socket_patch.dar...
runtime/bin/socket_patch.dart:258:
sb.write('{"bytesWritten","${s.bytesWritten}"},');
What about rates over the last second, 5 seconds, minute, ?

https://codereview.chromium.org/163903002/diff/1/runtime/vm/service.cc
File runtime/vm/service.cc (right):

https://codereview.chromium.org/163903002/diff/1/runtime/vm/service.cc#newcod...
runtime/vm/service.cc:642: if (Dart_IsError(result))
Dart_PropagateError(result);
This code is executing inside the users isolate, in the case that an error
occurs when processing a service request we need to make sure that it is dropped
and does not affect user code. Errors should be reported on the JSONStream
object (see other handlers).

[Søren Gjesse, 2014-02-14 12:10:13]

https://codereview.chromium.org/163903002/diff/1/runtime/bin/socket_patch.dart
File runtime/bin/socket_patch.dart (right):

https://codereview.chromium.org/163903002/diff/1/runtime/bin/socket_patch.dar...
runtime/bin/socket_patch.dart:226: sockets.forEach((s) {
On 2014/02/13 22:42:09, Cutch wrote:
> You probably should use the dart:convert library instead of serializing the
JSON
> yourself.

Absolutely - this was just a first hack.

https://codereview.chromium.org/163903002/diff/1/runtime/bin/socket_patch.dar...
runtime/bin/socket_patch.dart:258:
sb.write('{"bytesWritten","${s.bytesWritten}"},');
On 2014/02/13 22:42:09, Cutch wrote:
> What about rates over the last second, 5 seconds, minute, ?

Added rate calculation using a timer. We might have to find a better way, and we
should only enable it when the VM service is used.

https://codereview.chromium.org/163903002/diff/1/runtime/vm/service.cc
File runtime/vm/service.cc (right):

https://codereview.chromium.org/163903002/diff/1/runtime/vm/service.cc#newcod...
runtime/vm/service.cc:642: if (Dart_IsError(result))
Dart_PropagateError(result);
On 2014/02/13 22:42:09, Cutch wrote:
> This code is executing inside the users isolate, in the case that an error
> occurs when processing a service request we need to make sure that it is
dropped
> and does not affect user code. Errors should be reported on the JSONStream
> object (see other handlers).

Done.

[Cutch, 2014-02-18 22:43:10]

Soren,

Please see https://code.google.com/p/dart/source/detail?r=32769 for the API to
register your handlers with (i.e. Dart_RegisterIsolateServiceRequestCallback and
Dart_RegisterRootServiceRequestCallback).

John

[Søren Gjesse, 2014-02-19 13:17:57]

On 2014/02/18 22:43:10, Cutch wrote:
> Soren,
> 
> Please see https://code.google.com/p/dart/source/detail?r=32769 for the API to
> register your handlers with (i.e. Dart_RegisterIsolateServiceRequestCallback
and
> Dart_RegisterRootServiceRequestCallback).
> 
> John

John,

I have changed to use the new embedders API. However this only gives JSON data
(using the path /isolates/7114/io/sockets. How will adding rendering templates
for the path /#/isolates/7114/io/sockets work?

If I return an error (e.g. going to /#/isolates/7114/io/xxx) the error template
is used.

[Cutch, 2014-02-19 15:55:31]

On 2014/02/19 13:17:57, Søren Gjesse wrote:
> On 2014/02/18 22:43:10, Cutch wrote:
> > Soren,
> > 
> > Please see https://code.google.com/p/dart/source/detail?r=32769 for the API
to
> > register your handlers with (i.e. Dart_RegisterIsolateServiceRequestCallback
> and
> > Dart_RegisterRootServiceRequestCallback).
> > 
> > John
> 
> John,
> 
> I have changed to use the new embedders API. However this only gives JSON data
> (using the path /isolates/7114/io/sockets. How will adding rendering templates
> for the path /#/isolates/7114/io/sockets work?
> 
> If I return an error (e.g. going to /#/isolates/7114/io/xxx) the error
template
> is used.

Soren,

The Observatory frontend uses a message's type field to pick which template to
instantiate. See my comments on how you should be using the type field. What we
need to do is add a template which is instantiated when we get a 'SocketList'
message and a second template which is instantiated for each 'Socket' message we
get.

[Cutch, 2014-02-19 15:56:01]

lgtm after comments are addressed. Once you land this I will add a skeleton UI
and then we can iterate on what to display and the best way to display it.

https://codereview.chromium.org/163903002/diff/320001/runtime/bin/socket_patc...
File runtime/bin/socket_patch.dart (right):

https://codereview.chromium.org/163903002/diff/320001/runtime/bin/socket_patc...
runtime/bin/socket_patch.dart:275: response['type'] = 'io';
This should probably be 'SocketList'.

https://codereview.chromium.org/163903002/diff/320001/runtime/bin/socket_patc...
runtime/bin/socket_patch.dart:277: response['sockets'] = socketsStat;
s/sockets/members

https://codereview.chromium.org/163903002/diff/320001/runtime/bin/socket_patc...
runtime/bin/socket_patch.dart:282: socket.isInternal ? "INTERNAL" : "NORMAL";
The "type" field has a special meaning in Observatory (we use it to decide which
Polymer template is instantiated). For a socket it should be "Socket".

You should probably rename your "type" here to be "kind" (or something...)

[Søren Gjesse, 2014-02-19 16:02:41]

Anders could you take a look?

John, how about testing this?

https://codereview.chromium.org/163903002/diff/320001/runtime/bin/socket_patc...
File runtime/bin/socket_patch.dart (right):

https://codereview.chromium.org/163903002/diff/320001/runtime/bin/socket_patc...
runtime/bin/socket_patch.dart:275: response['type'] = 'io';
On 2014/02/19 15:56:02, Cutch wrote:
> This should probably be 'SocketList'.

Done.

https://codereview.chromium.org/163903002/diff/320001/runtime/bin/socket_patc...
runtime/bin/socket_patch.dart:277: response['sockets'] = socketsStat;
On 2014/02/19 15:56:02, Cutch wrote:
> s/sockets/members

Done.

https://codereview.chromium.org/163903002/diff/320001/runtime/bin/socket_patc...
runtime/bin/socket_patch.dart:282: socket.isInternal ? "INTERNAL" : "NORMAL";
On 2014/02/19 15:56:02, Cutch wrote:
> The "type" field has a special meaning in Observatory (we use it to decide
which
> Polymer template is instantiated). For a socket it should be "Socket".
> 
> You should probably rename your "type" here to be "kind" (or something...)

Done.

[Cutch, 2014-02-19 16:05:02]

On 2014/02/19 16:02:41, Søren Gjesse wrote:
> Anders could you take a look?
> 
> John, how about testing this?
> 

You can see standalone/vmservice tests which launch a second VM and then signals
back to the test to run some queries.

[Cutch, 2014-02-19 16:05:13]

One last nit and I think we should get a test.

https://codereview.chromium.org/163903002/diff/390001/runtime/bin/socket_patc...
File runtime/bin/socket_patch.dart (right):

https://codereview.chromium.org/163903002/diff/390001/runtime/bin/socket_patc...
runtime/bin/socket_patch.dart:310: members.add({'kind': kind, 'protocol':
protocol,
Need a 'type': 'Socket' in this map.

[Anders Johnsen, 2014-02-19 16:14:34]

lgtm, but we should watch closely for performance impact.

https://codereview.chromium.org/163903002/diff/390001/runtime/bin/main.cc
File runtime/bin/main.cc (right):

https://codereview.chromium.org/163903002/diff/390001/runtime/bin/main.cc#new...
runtime/bin/main.cc:861: return strdup(json);
Is the string expected to be allocate with malloc or new?

https://codereview.chromium.org/163903002/diff/390001/runtime/bin/socket_patc...
File runtime/bin/socket_patch.dart (right):

https://codereview.chromium.org/163903002/diff/390001/runtime/bin/socket_patc...
runtime/bin/socket_patch.dart:251: sockets.remove(socket);
assert it is in the map.

https://codereview.chromium.org/163903002/diff/390001/runtime/bin/socket_patc...
runtime/bin/socket_patch.dart:575: _SocketsObservatory.add(this);
Move to setListening, where eventPort == null.

https://codereview.chromium.org/163903002/diff/390001/runtime/bin/socket_patc...
runtime/bin/socket_patch.dart:614: totalRead += result.length;
put in '!= null'.

https://codereview.chromium.org/163903002/diff/390001/runtime/bin/socket_patc...
runtime/bin/socket_patch.dart:697: _SocketsObservatory.remove(socket);
This can be removed with above change.

https://codereview.chromium.org/163903002/diff/390001/runtime/bin/socket_win.cc
File runtime/bin/socket_win.cc (left):

https://codereview.chromium.org/163903002/diff/390001/runtime/bin/socket_win....
runtime/bin/socket_win.cc:119: Log::PrintErr("Error getpeername: %d\n",
WSAGetLastError());
Ty :)

[Cutch, 2014-02-19 16:32:46]

On 2014/02/19 16:14:34, Anders Johnsen wrote:
> lgtm, but we should watch closely for performance impact.
> 

Agreed. Note that there is an API available now to detect if the service is
running (and if it isn't expensive statistics tracking should be disabled).

[Cutch, 2014-02-19 16:32:52]

https://codereview.chromium.org/163903002/diff/390001/runtime/bin/main.cc
File runtime/bin/main.cc (right):

https://codereview.chromium.org/163903002/diff/390001/runtime/bin/main.cc#new...
runtime/bin/main.cc:861: return strdup(json);
On 2014/02/19 16:14:34, Anders Johnsen wrote:
> Is the string expected to be allocate with malloc or new?

Strings returned from a service request handler are freed by a call to free. So,
malloc.

[Cutch, 2014-02-24 15:16:44]

On 2014/02/19 16:32:52, Cutch wrote:
> https://codereview.chromium.org/163903002/diff/390001/runtime/bin/main.cc
> File runtime/bin/main.cc (right):
> 
>
https://codereview.chromium.org/163903002/diff/390001/runtime/bin/main.cc#new...
> runtime/bin/main.cc:861: return strdup(json);
> On 2014/02/19 16:14:34, Anders Johnsen wrote:
> > Is the string expected to be allocate with malloc or new?
> 
> Strings returned from a service request handler are freed by a call to free.
So,
> malloc.

Ping.

[Søren Gjesse, 2014-03-06 11:12:18]

Committed patchset #7 manually as r33366 (presubmit successful).

[Søren Gjesse, 2014-03-06 11:13:43]

I ended up removing the rate timer, as it could keep the VM from terminating
when sockets where not closed.

Will revisit in a new CL.

https://codereview.chromium.org/163903002/diff/390001/runtime/bin/socket_patc...
File runtime/bin/socket_patch.dart (right):

https://codereview.chromium.org/163903002/diff/390001/runtime/bin/socket_patc...
runtime/bin/socket_patch.dart:251: sockets.remove(socket);
On 2014/02/19 16:14:34, Anders Johnsen wrote:
> assert it is in the map.

Done.

https://codereview.chromium.org/163903002/diff/390001/runtime/bin/socket_patc...
runtime/bin/socket_patch.dart:310: members.add({'kind': kind, 'protocol':
protocol,
On 2014/02/19 16:05:13, Cutch wrote:
> Need a 'type': 'Socket' in this map.

Done.

https://codereview.chromium.org/163903002/diff/390001/runtime/bin/socket_patc...
runtime/bin/socket_patch.dart:575: _SocketsObservatory.add(this);
On 2014/02/19 16:14:34, Anders Johnsen wrote:
> Move to setListening, where eventPort == null.

Done.

https://codereview.chromium.org/163903002/diff/390001/runtime/bin/socket_patc...
runtime/bin/socket_patch.dart:614: totalRead += result.length;
On 2014/02/19 16:14:34, Anders Johnsen wrote:
> put in '!= null'.

Done.

https://codereview.chromium.org/163903002/diff/390001/runtime/bin/socket_patc...
runtime/bin/socket_patch.dart:697: _SocketsObservatory.remove(socket);
On 2014/02/19 16:14:34, Anders Johnsen wrote:
> This can be removed with above change.

Done.