Support piping log data over the service protocol

Support piping log data over the service protocol

BUG=
R=rmacnak@google.com

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

[Cutch, 2015-07-17 00:02:59]

johnmccutchan@google.com changed reviewers:
+ turnidge@google.com

[rmacnak, 2015-07-17 00:17:55]

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

[rmacnak, 2015-07-17 00:17:56]

DBC

https://codereview.chromium.org/1241683005/diff/60001/sdk/lib/developer/devel...
File sdk/lib/developer/developer.dart (right):

https://codereview.chromium.org/1241683005/diff/60001/sdk/lib/developer/devel...
sdk/lib/developer/developer.dart:38: /// [sequenceNumber] is a monotonically
increasing sequence number.
What happens if this is called with the same sequence number repeatedly? In
particular, independent event sources seem likely to choose conflicting sequence
numbers.

https://codereview.chromium.org/1241683005/diff/60001/sdk/lib/developer/devel...
sdk/lib/developer/developer.dart:40: /// [level] is the severity level (value
between 0 and 2000 inclusive).
Are high or low values more severe?

[turnidge, 2015-07-17 00:19:55]

General comment: It would be nice to build a system that could snoop on the Log
automatically, w/o the user having to change their program.  Have you thought
about how we might do this?

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

https://codereview.chromium.org/1241683005/diff/60001/runtime/vm/service.cc#n...
runtime/vm/service.cc:2534: const Instance& stack_trace) {
Can this work like most of the other ServiceEvents?  For those we add the data
to the ServiceEvent object and then the ServiceEvent knows how to print itself
as json.

I know there are a few exceptions, but they are old.

https://codereview.chromium.org/1241683005/diff/60001/runtime/vm/service.cc#n...
runtime/vm/service.cc:2555: event.AddProperty("stackTrace", stack_trace);
This stuff would move into the ServiceEvent::PrintJSON or whatever it is called.

[Cutch, 2015-07-17 00:50:13]

I agree it would be nice to have a system that didn't require modifying the
users program. This is tricky to do for logging because it's a package and there
can be multiple "Logger" instances (which have no central registry) and each
instance of Logger has an independent stream that must be listened on by the
program. 

That being said, I have a bunch of ideas of how we could accomplish this down
the road.

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

https://codereview.chromium.org/1241683005/diff/60001/runtime/vm/service.cc#n...
runtime/vm/service.cc:2534: const Instance& stack_trace) {
On 2015/07/17 00:19:55, turnidge wrote:
> Can this work like most of the other ServiceEvents?  For those we add the data
> to the ServiceEvent object and then the ServiceEvent knows how to print itself
> as json.
> 
> I know there are a few exceptions, but they are old.

I deliberately didn't make this work like the other ServiceEvents. My reasoning
was that in order to do that we need to allocate 4 extra handles and bloat the
ServiceEvent with 8 fields. That felt wasteful and I decided instead to refactor
the code so that the practically all of the code is shared.

https://codereview.chromium.org/1241683005/diff/60001/runtime/vm/service.cc#n...
runtime/vm/service.cc:2555: event.AddProperty("stackTrace", stack_trace);
On 2015/07/17 00:19:54, turnidge wrote:
> This stuff would move into the ServiceEvent::PrintJSON or whatever it is
called.

Acknowledged.

https://codereview.chromium.org/1241683005/diff/60001/sdk/lib/developer/devel...
File sdk/lib/developer/developer.dart (right):

https://codereview.chromium.org/1241683005/diff/60001/sdk/lib/developer/devel...
sdk/lib/developer/developer.dart:38: /// [sequenceNumber] is a monotonically
increasing sequence number.
On 2015/07/17 00:17:56, rmacnak wrote:
> What happens if this is called with the same sequence number repeatedly? In
> particular, independent event sources seem likely to choose conflicting
sequence
> numbers.

Note that all of these fields and their restrictions are dictated by the logging
package.

With my UI right now, it makes no difference. In the future, we may sort log
items based on their sequence number. If a program can't keep their sequence
numbers straight they may have a problem.

https://codereview.chromium.org/1241683005/diff/60001/sdk/lib/developer/devel...
sdk/lib/developer/developer.dart:40: /// [level] is the severity level (value
between 0 and 2000 inclusive).
On 2015/07/17 00:17:56, rmacnak wrote:
> Are high or low values more severe?

Note that all of these fields and their restrictions are dictated by the logging
package.

High values are more severe.

[Cutch, 2015-07-20 23:28:53]

johnmccutchan@google.com changed reviewers:
- turnidge@google.com

[Cutch, 2015-07-20 23:28:59]

I've moved all the data into ServiceEvent and made the printing regular.

PTAL.

[rmacnak, 2015-07-21 00:20:15]

LGTM w/ comments

https://codereview.chromium.org/1241683005/diff/80001/runtime/observatory/tes...
File runtime/observatory/tests/service/logging_test.dart (right):

https://codereview.chromium.org/1241683005/diff/80001/runtime/observatory/tes...
runtime/observatory/tests/service/logging_test.dart:27:
isolateStreamEvent('Logging', (ServiceEvent event) {
Isolate.kLoggingStream

https://codereview.chromium.org/1241683005/diff/80001/runtime/observatory/tes...
runtime/observatory/tests/service/logging_test.dart:34:
isolateStreamEvent('Logging', (ServiceEvent event) {
Isolate.kLoggingStream

https://codereview.chromium.org/1241683005/diff/80001/runtime/observatory/tes...
File runtime/observatory/tests/service/test_helper.dart (right):

https://codereview.chromium.org/1241683005/diff/80001/runtime/observatory/tes...
runtime/observatory/tests/service/test_helper.dart:258: IsolateTest
isolateStreamEvent(stream, onEvent) {
This name doesn't suggest it resumes the isolate.

resumeAndAwaitEvent?

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

https://codereview.chromium.org/1241683005/diff/80001/runtime/vm/service.cc#n...
runtime/vm/service.cc:90: StreamInfo Service::logging_stream("Logging");
Switch to _Logging or add to service.md

[Cutch, 2015-07-21 14:51:44]

https://codereview.chromium.org/1241683005/diff/80001/runtime/observatory/tes...
File runtime/observatory/tests/service/logging_test.dart (right):

https://codereview.chromium.org/1241683005/diff/80001/runtime/observatory/tes...
runtime/observatory/tests/service/logging_test.dart:27:
isolateStreamEvent('Logging', (ServiceEvent event) {
On 2015/07/21 00:20:15, rmacnak wrote:
> Isolate.kLoggingStream

Done.

https://codereview.chromium.org/1241683005/diff/80001/runtime/observatory/tes...
runtime/observatory/tests/service/logging_test.dart:34:
isolateStreamEvent('Logging', (ServiceEvent event) {
On 2015/07/21 00:20:15, rmacnak wrote:
> Isolate.kLoggingStream

Done.

https://codereview.chromium.org/1241683005/diff/80001/runtime/observatory/tes...
File runtime/observatory/tests/service/test_helper.dart (right):

https://codereview.chromium.org/1241683005/diff/80001/runtime/observatory/tes...
runtime/observatory/tests/service/test_helper.dart:258: IsolateTest
isolateStreamEvent(stream, onEvent) {
On 2015/07/21 00:20:15, rmacnak wrote:
> This name doesn't suggest it resumes the isolate.
> 
> resumeAndAwaitEvent?

Done.

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

https://codereview.chromium.org/1241683005/diff/80001/runtime/vm/service.cc#n...
runtime/vm/service.cc:90: StreamInfo Service::logging_stream("Logging");
On 2015/07/21 00:20:15, rmacnak wrote:
> Switch to _Logging or add to service.md

Done.

[Cutch, 2015-07-21 14:54:50]

Committed patchset #6 (id:100001) manually as
f818f9dc0f00c56c257398724430d9a7eddb19f2 (presubmit successful).