Initial backend for metrics in Observatory

Initial backend for metrics in Observatory.

Two types of metrics:

* Counter - a measured value
* Gauge - A measured value with a min and max

Follow up CLs for the backend:

* Support for internal VM metrics (implemented in C++ but visible over the service).

Follow up CLs for the frontend:

* Query list of metrics
* Get latest value of a metric
* Collect values over time and graph them.

I'd like some feedback on the API and the data reported.

R=koda@google.com

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

[srdjan, 2014-07-29 00:30:02]

DBC

https://codereview.chromium.org/409213004/diff/1/runtime/lib/profiler.dart
File runtime/lib/profiler.dart (right):

https://codereview.chromium.org/409213004/diff/1/runtime/lib/profiler.dart#ne...
runtime/lib/profiler.dart:49: max = null,
Why setting it to null a second time (first at allocation, implicitly)?

https://codereview.chromium.org/409213004/diff/1/runtime/lib/profiler.dart#ne...
runtime/lib/profiler.dart:55: throw new ArgumentError('min and max cannot be
null.');
the min/max is! double should also cover min/max == null test.

[koda, 2014-07-29 15:34:31]

https://codereview.chromium.org/409213004/diff/20001/runtime/lib/profiler.dart
File runtime/lib/profiler.dart (right):

https://codereview.chromium.org/409213004/diff/20001/runtime/lib/profiler.dar...
runtime/lib/profiler.dart:30: /// [name] of this metric.
Consider describing a convention for hierarchical naming here (like the "a.b.c"
you mentioned).

https://codereview.chromium.org/409213004/diff/20001/runtime/lib/profiler.dar...
runtime/lib/profiler.dart:49: _value = v;
Why no range checking here?

https://codereview.chromium.org/409213004/diff/20001/runtime/lib/profiler.dar...
runtime/lib/profiler.dart:60: if (min >= max) {
If you change this to !(min < max) you also catch NaNs, but perhaps it's better
to check for them explicitly.

https://codereview.chromium.org/409213004/diff/20001/runtime/lib/profiler.dar...
runtime/lib/profiler.dart:81: /// A changing value. Initial value is 0.0.
There should somewhere be a comment noting the API uses double rather than int
for counters. I still have some doubts about that.

https://codereview.chromium.org/409213004/diff/20001/runtime/lib/profiler.dar...
runtime/lib/profiler.dart:113: if (_metrics.contains(metric)) {
Since you didn't override operator==, this will just check object identity.

Consider going further and forbidding multiple metrics having (just) the same
name, since that seems like a possible source of confusion with no benefit.

Actually, if we do enforce distinct names, is there a reason we need a separate
_id field?

https://codereview.chromium.org/409213004/diff/20001/runtime/lib/profiler.dar...
runtime/lib/profiler.dart:116: _metrics.add(metric);
Assignment of the id happens in Metric, so if the user creates metrics without
adding them, or adds them in a different order, the ids will no longer match the
indices in _metrics.

It seems like you don't use the indices, but it still feels bug-prone to have
the ids and indices usually match, but not always.

Any reason we can't assign the id here instead (except that the field could no
longer be final)? Or alternatively, call this method from the constructor of
Metric to ensure they are all always added.

https://codereview.chromium.org/409213004/diff/20001/runtime/lib/profiler.dar...
runtime/lib/profiler.dart:145: 'members': l,
I misread "l" as the digit one; please rename.

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

https://codereview.chromium.org/409213004/diff/20001/runtime/vm/service.cc#ne...
runtime/vm/service.cc:1611: return metrics_cls.IsClass();
A bit too late to test this here :) You probably just want to return true.

https://codereview.chromium.org/409213004/diff/20001/runtime/vm/service_test.cc
File runtime/vm/service_test.cc (right):

https://codereview.chromium.org/409213004/diff/20001/runtime/vm/service_test....
runtime/vm/service_test.cc:1886: "  var counter = new Counter('hello',
'apple');\n"
Consider using slightly more realistic names to help the reader remember the
meaning of the arguments.

https://codereview.chromium.org/409213004/diff/20001/runtime/vm/service_test....
runtime/vm/service_test.cc:1913: EXPECT_SUBSTRING("\"type\":\"MetricList\"",
handler.msg());
Check that it contains exactly one member?

https://codereview.chromium.org/409213004/diff/20001/runtime/vm/service_test....
runtime/vm/service_test.cc:1950: // Expect MetricList.
MetricList -> Counter

https://codereview.chromium.org/409213004/diff/20001/tests/lib/profiler/metri...
File tests/lib/profiler/metrics_test.dart (right):

https://codereview.chromium.org/409213004/diff/20001/tests/lib/profiler/metri...
tests/lib/profiler/metrics_test.dart:17: Expect.equals(44.0, gauge.value);
Test setting to out-of-range value.

[Cutch, 2014-07-29 18:27:01]

https://codereview.chromium.org/409213004/diff/20001/runtime/lib/profiler.dart
File runtime/lib/profiler.dart (right):

https://codereview.chromium.org/409213004/diff/20001/runtime/lib/profiler.dar...
runtime/lib/profiler.dart:30: /// [name] of this metric.
On 2014/07/29 15:34:30, koda wrote:
> Consider describing a convention for hierarchical naming here (like the
"a.b.c"
> you mentioned).

Done.

https://codereview.chromium.org/409213004/diff/20001/runtime/lib/profiler.dar...
runtime/lib/profiler.dart:49: _value = v;
On 2014/07/29 15:34:30, koda wrote:
> Why no range checking here?

I clamp now. I do not want these code paths to be slowed down with checks and
throws.

https://codereview.chromium.org/409213004/diff/20001/runtime/lib/profiler.dar...
runtime/lib/profiler.dart:60: if (min >= max) {
On 2014/07/29 15:34:29, koda wrote:
> If you change this to !(min < max) you also catch NaNs, but perhaps it's
better
> to check for them explicitly.

Switched to !(min < max). I'm not overly concerned with catching user errors.

https://codereview.chromium.org/409213004/diff/20001/runtime/lib/profiler.dar...
runtime/lib/profiler.dart:81: /// A changing value. Initial value is 0.0.
On 2014/07/29 15:34:29, koda wrote:
> There should somewhere be a comment noting the API uses double rather than int
> for counters. I still have some doubts about that.

Don't the type annotations make it clear enough?

https://codereview.chromium.org/409213004/diff/20001/runtime/lib/profiler.dar...
runtime/lib/profiler.dart:113: if (_metrics.contains(metric)) {
On 2014/07/29 15:34:30, koda wrote:
> Since you didn't override operator==, this will just check object identity.
> 
> Consider going further and forbidding multiple metrics having (just) the same
> name, since that seems like a possible source of confusion with no benefit.
> 
> Actually, if we do enforce distinct names, is there a reason we need a
separate
> _id field?

I like using the name as the id and requiring that programs keep their ids
unique.

https://codereview.chromium.org/409213004/diff/20001/runtime/lib/profiler.dar...
runtime/lib/profiler.dart:116: _metrics.add(metric);
On 2014/07/29 15:34:30, koda wrote:
> Assignment of the id happens in Metric, so if the user creates metrics without
> adding them, or adds them in a different order, the ids will no longer match
the
> indices in _metrics.
> 
> It seems like you don't use the indices, but it still feels bug-prone to have
> the ids and indices usually match, but not always.
> 
> Any reason we can't assign the id here instead (except that the field could no
> longer be final)? Or alternatively, call this method from the constructor of
> Metric to ensure they are all always added.

Acknowledged.

https://codereview.chromium.org/409213004/diff/20001/runtime/lib/profiler.dar...
runtime/lib/profiler.dart:145: 'members': l,
On 2014/07/29 15:34:29, koda wrote:
> I misread "l" as the digit one; please rename.

Done.

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

https://codereview.chromium.org/409213004/diff/20001/runtime/vm/service.cc#ne...
runtime/vm/service.cc:1611: return metrics_cls.IsClass();
On 2014/07/29 15:34:30, koda wrote:
> A bit too late to test this here :) You probably just want to return true.

Done.

https://codereview.chromium.org/409213004/diff/20001/runtime/vm/service_test.cc
File runtime/vm/service_test.cc (right):

https://codereview.chromium.org/409213004/diff/20001/runtime/vm/service_test....
runtime/vm/service_test.cc:1913: EXPECT_SUBSTRING("\"type\":\"MetricList\"",
handler.msg());
On 2014/07/29 15:34:30, koda wrote:
> Check that it contains exactly one member?

Oi, testing this in C++ is going to be painful. Permission to ignore?

https://codereview.chromium.org/409213004/diff/20001/runtime/vm/service_test....
runtime/vm/service_test.cc:1950: // Expect MetricList.
On 2014/07/29 15:34:30, koda wrote:
> MetricList -> Counter

Done.

https://codereview.chromium.org/409213004/diff/20001/tests/lib/profiler/metri...
File tests/lib/profiler/metrics_test.dart (right):

https://codereview.chromium.org/409213004/diff/20001/tests/lib/profiler/metri...
tests/lib/profiler/metrics_test.dart:17: Expect.equals(44.0, gauge.value);
On 2014/07/29 15:34:30, koda wrote:
> Test setting to out-of-range value.

Done.

[koda, 2014-07-29 19:44:13]

Mostly nits remaining.

BTW, do you have some particular example use case for Gauge?

https://codereview.chromium.org/409213004/diff/20001/runtime/lib/profiler.dart
File runtime/lib/profiler.dart (right):

https://codereview.chromium.org/409213004/diff/20001/runtime/lib/profiler.dar...
runtime/lib/profiler.dart:81: /// A changing value. Initial value is 0.0.
On 2014/07/29 18:27:01, Cutch wrote:
> On 2014/07/29 15:34:29, koda wrote:
> > There should somewhere be a comment noting the API uses double rather than
int
> > for counters. I still have some doubts about that.
> 
> Don't the type annotations make it clear enough?

Sure, I guess. Actually, I guess what I meant was the reason why double was
chosen rather than the arguably more natural int. If JS did not exist, there
would be no reason to use double here, right?

https://codereview.chromium.org/409213004/diff/20001/runtime/vm/service_test.cc
File runtime/vm/service_test.cc (right):

https://codereview.chromium.org/409213004/diff/20001/runtime/vm/service_test....
runtime/vm/service_test.cc:1913: EXPECT_SUBSTRING("\"type\":\"MetricList\"",
handler.msg());
On 2014/07/29 18:27:01, Cutch wrote:
> On 2014/07/29 15:34:30, koda wrote:
> > Check that it contains exactly one member?
> 
> Oi, testing this in C++ is going to be painful. Permission to ignore?

Please make it a TODO, since I have a feeling there will be a way to test it in
Dart code very soon :)

https://codereview.chromium.org/409213004/diff/40001/runtime/lib/profiler.dart
File runtime/lib/profiler.dart (right):

https://codereview.chromium.org/409213004/diff/40001/runtime/lib/profiler.dar...
runtime/lib/profiler.dart:6: import 'dart:convert';
Inconsistent quotes.

https://codereview.chromium.org/409213004/diff/40001/runtime/lib/profiler.dar...
runtime/lib/profiler.dart:53: if (v > max) {
else if

https://codereview.chromium.org/409213004/diff/40001/runtime/lib/profiler.dar...
runtime/lib/profiler.dart:102: 'id': 'metrics/$name',
I guess we should avoid having slashes in the name then? Maybe overkill to
verify it, though.

https://codereview.chromium.org/409213004/diff/40001/runtime/lib/profiler.dar...
runtime/lib/profiler.dart:111: /// Register [Metric]s to make them visible to
Observatory.
You use the term "register" here and above, but the method is called "add".
Consider renaming/rewording.

[Cutch, 2014-07-30 00:01:24]

Use case for Gauge is when a program has a limit of some value, we can visualize
them better knowing what these limits are.

https://codereview.chromium.org/409213004/diff/20001/runtime/lib/profiler.dart
File runtime/lib/profiler.dart (right):

https://codereview.chromium.org/409213004/diff/20001/runtime/lib/profiler.dar...
runtime/lib/profiler.dart:81: /// A changing value. Initial value is 0.0.
On 2014/07/29 19:44:13, koda wrote:
> On 2014/07/29 18:27:01, Cutch wrote:
> > On 2014/07/29 15:34:29, koda wrote:
> > > There should somewhere be a comment noting the API uses double rather than
> int
> > > for counters. I still have some doubts about that.
> > 
> > Don't the type annotations make it clear enough?
> 
> Sure, I guess. Actually, I guess what I meant was the reason why double was
> chosen rather than the arguably more natural int. If JS did not exist, there
> would be no reason to use double here, right?

I don't want to put that in the API documentation. I'm not sure Dart developers
will care.

https://codereview.chromium.org/409213004/diff/20001/runtime/vm/service_test.cc
File runtime/vm/service_test.cc (right):

https://codereview.chromium.org/409213004/diff/20001/runtime/vm/service_test....
runtime/vm/service_test.cc:1913: EXPECT_SUBSTRING("\"type\":\"MetricList\"",
handler.msg());
On 2014/07/29 19:44:13, koda wrote:
> On 2014/07/29 18:27:01, Cutch wrote:
> > On 2014/07/29 15:34:30, koda wrote:
> > > Check that it contains exactly one member?
> > 
> > Oi, testing this in C++ is going to be painful. Permission to ignore?
> 
> Please make it a TODO, since I have a feeling there will be a way to test it
in
> Dart code very soon :)

Done.

https://codereview.chromium.org/409213004/diff/40001/runtime/lib/profiler.dart
File runtime/lib/profiler.dart (right):

https://codereview.chromium.org/409213004/diff/40001/runtime/lib/profiler.dar...
runtime/lib/profiler.dart:6: import 'dart:convert';
On 2014/07/29 19:44:13, koda wrote:
> Inconsistent quotes.

Done.

https://codereview.chromium.org/409213004/diff/40001/runtime/lib/profiler.dar...
runtime/lib/profiler.dart:53: if (v > max) {
On 2014/07/29 19:44:13, koda wrote:
> else if

Done.

https://codereview.chromium.org/409213004/diff/40001/runtime/lib/profiler.dar...
runtime/lib/profiler.dart:102: 'id': 'metrics/$name',
On 2014/07/29 19:44:13, koda wrote:
> I guess we should avoid having slashes in the name then? Maybe overkill to
> verify it, though.

We need to verify. Things will fall over when we parse the request url if the
name has a /. I now throw in the Metric constructor if the name contains a /.

https://codereview.chromium.org/409213004/diff/40001/runtime/lib/profiler.dar...
runtime/lib/profiler.dart:111: /// Register [Metric]s to make them visible to
Observatory.
On 2014/07/29 19:44:13, koda wrote:
> You use the term "register" here and above, but the method is called "add".
> Consider renaming/rewording.

Done.

[koda, 2014-07-30 00:07:12]

lgtm

[turnidge, 2014-07-30 19:15:00]

https://codereview.chromium.org/409213004/diff/80001/runtime/lib/profiler.dart
File runtime/lib/profiler.dart (right):

https://codereview.chromium.org/409213004/diff/80001/runtime/lib/profiler.dar...
runtime/lib/profiler.dart:50: final double max;
double seems reasonable, but will we ever want more types?  generics?

https://codereview.chromium.org/409213004/diff/80001/runtime/lib/profiler.dar...
runtime/lib/profiler.dart:93: class Counter extends Metric {
The name Counter implies monotonic to me, but the class doesn't enforce this. 
I'm happy to leave this as is until we get a feel for how these are used.

https://codereview.chromium.org/409213004/diff/80001/runtime/lib/profiler.dar...
runtime/lib/profiler.dart:117: static final List<Metric> _metrics = new
List<Metric>();
Why not make this a Map?  Then you can avoid the loops in _nameExists and
_printMetric (and implicilty in deregister).

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

https://codereview.chromium.org/409213004/diff/80001/runtime/vm/service.cc#ne...
runtime/vm/service.cc:1633: ASSERT(!result.IsNull());
You should handle null here.  The _printMetric function can return null.

https://codereview.chromium.org/409213004/diff/80001/runtime/vm/service.cc#ne...
runtime/vm/service.cc:2177: { "metrics", HandleMetrics },
Will we ever want to have per-vm metrics instead of per-isolate metrics?  Just
curious.  This is ok for now.

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

https://codereview.chromium.org/409213004/diff/80001/runtime/vm/service_test....
runtime/vm/service_test.cc:2059: EXPECT_SUBSTRING("\"type\":\"Counter\"",
handler.msg());
Add test for a metric that is not found.

[Cutch, 2014-07-30 21:55:04]

https://codereview.chromium.org/409213004/diff/80001/runtime/lib/profiler.dart
File runtime/lib/profiler.dart (right):

https://codereview.chromium.org/409213004/diff/80001/runtime/lib/profiler.dar...
runtime/lib/profiler.dart:50: final double max;
On 2014/07/30 19:15:00, turnidge wrote:
> double seems reasonable, but will we ever want more types?  generics?

I could imagine people wanting integers but they get 53-bit integers with
double. I don't want to use <T> here because we must be able to know what this
data will look like over the service.

https://codereview.chromium.org/409213004/diff/80001/runtime/lib/profiler.dar...
runtime/lib/profiler.dart:93: class Counter extends Metric {
On 2014/07/30 19:15:00, turnidge wrote:
> The name Counter implies monotonic to me, but the class doesn't enforce this. 
> I'm happy to leave this as is until we get a feel for how these are used.

Initially I had this monotonic but decided to relax that for now.

https://codereview.chromium.org/409213004/diff/80001/runtime/lib/profiler.dar...
runtime/lib/profiler.dart:117: static final List<Metric> _metrics = new
List<Metric>();
On 2014/07/30 19:15:00, turnidge wrote:
> Why not make this a Map?  Then you can avoid the loops in _nameExists and
> _printMetric (and implicilty in deregister).

Done.

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

https://codereview.chromium.org/409213004/diff/80001/runtime/vm/service.cc#ne...
runtime/vm/service.cc:1633: ASSERT(!result.IsNull());
On 2014/07/30 19:15:00, turnidge wrote:
> You should handle null here.  The _printMetric function can return null.

Excellent catch.

https://codereview.chromium.org/409213004/diff/80001/runtime/vm/service.cc#ne...
runtime/vm/service.cc:2177: { "metrics", HandleMetrics },
On 2014/07/30 19:15:00, turnidge wrote:
> Will we ever want to have per-vm metrics instead of per-isolate metrics?  Just
> curious.  This is ok for now.

Yes, for example, the number of isolates could be a metric reported by the VM.

These will be implemented in C++ though.

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

https://codereview.chromium.org/409213004/diff/80001/runtime/vm/service_test....
runtime/vm/service_test.cc:2059: EXPECT_SUBSTRING("\"type\":\"Counter\"",
handler.msg());
On 2014/07/30 19:15:00, turnidge wrote:
> Add test for a metric that is not found.

Done.

[Cutch, 2014-07-31 18:27:47]

Committed patchset #8 manually as r38793 (presubmit successful).