Initial backend for metrics in Observatory

runtime/lib/profiler.dart

Index: runtime/lib/profiler.dart
diff --git a/runtime/lib/profiler.dart b/runtime/lib/profiler.dart
index 2c60141a492552be8f36be050e44dc0d772f7422..ede2148627305545059b4cb76eb5365d200429ec 100644
--- a/runtime/lib/profiler.dart
+++ b/runtime/lib/profiler.dart
@@ -3,6 +3,7 @@
 // BSD-style license that can be found in the LICENSE file.
 
 import "dart:_internal";
+import 'dart:convert';
 
 patch class UserTag {
   /* patch */ factory UserTag(String label) {
@@ -22,3 +23,127 @@ patch UserTag getCurrentTag() => _getCurrentTag();
 UserTag _getCurrentTag() native "Profiler_getCurrentTag";
 
 UserTag _getDefaultTag() native "UserTag_defaultTag";
+
+/// Abstract [Metric] class.
+abstract class Metric {
+  final String _id;
+  /// [name] of this metric.

[koda, 2014/07/29 15:34:30]

Consider describing a convention for hierarchical naming here (like the "a.b.c"
you mentioned).

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

Done.

+  final String name;
+  /// [description] of this metric.
+  final String description;
+
+  Metric(this.name, this.description)
+      : _id = '${Metrics._idCounter++}';
+
+  Map _toJSON();
+}
+
+/// A measured value with a min and max. Initial value is min.
+class Gauge extends Metric {
+  final double min;
+  final double max;
+
+  double _value;
+  double get value => _value;
+  set value(double v) {
+    _value = v;

[koda, 2014/07/29 15:34:30]

Why no range checking here?

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

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

+  }
+
+  Gauge(String name, String description, this.min, this.max)
+      : super(name, description) {
+    if (min is! double) {
+      throw new ArgumentError('min must be a double');
+    }
+    if (max is! double) {
+      throw new ArgumentError('max must be a double');
+    }
+    if (min >= max) {

[koda, 2014/07/29 15:34:29]

If you change this to !(min < max) you also catch NaNs, but perhaps it's better
to check for them explicitly.

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

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

+      throw new ArgumentError('min must be less than max');
+    }
+    _value = min;
+  }
+
+  Map _toJSON() {
+    var map = {
+      'type': 'Gauge',
+      'id': 'metrics/$_id',
+      'name': name,
+      'description': description,
+      'value': value,
+    };
+    map['min'] = min;
+    map['max'] = max;
+    return map;
+  }
+}
+
+
+/// A changing value. Initial value is 0.0.

[koda, 2014/07/29 15:34:29]

There should somewhere be a comment noting the API uses double rather than int
for counters. I still have some doubts about that.

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

Don't the type annotations make it clear enough?

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

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?

[Cutch, 2014/07/30 00:01:23]

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

+class Counter extends Metric {
+  Counter(String name, String description)
+      : super(name, description);
+
+  double _value = 0.0;
+  double get value => _value;
+  set value(double v) {
+    _value = v;
+  }
+
+  Map _toJSON() {
+    var map = {
+      'type': 'Counter',
+      'id': 'metrics/$_id',
+      'name': name,
+      'description': description,
+      'value': value,
+    };
+    return map;
+  }
+}
+
+/// Register [Metric] to make them visible over the service.
+class Metrics {
+  static int _idCounter = 0;
+  static final List<Metric> _metrics = new List<Metric>();
+
+  static void add(Metric metic) {
+    if (m == null) {
+      throw new ArgumentError('metric cannot be null');
+    }
+    if (_metrics.contains(metric)) {

[koda, 2014/07/29 15:34:30]

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?

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

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

+      throw new ArgumentError('metric already registered');
+    }
+    _metrics.add(metric);

[koda, 2014/07/29 15:34:30]

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.

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

Acknowledged.

+  }
+
+  static void remove(Metric m) {
+    if (m == null) {
+      throw new ArgumentError('metric cannot be null');
+    }
+    _metrics.remove(m);
+  }
+
+  static String _printMetric(String id) {
+    for (var i = 0; i < _metrics.length; i++) {
+      var m = _metrics[i];
+      if (m._id == id) {
+        return JSON.encode(m._toJSON());
+      }
+    }
+    return null;
+  }
+
+  static String _printMetrics() {
+    var l = [];
+    for (var i = 0; i < _metrics.length; i++) {
+      var m = _metrics[i];
+      l.add(m._toJSON());
+    }
+    var map = {
+      'type': 'MetricList',
+      'id': 'metrics',
+      'members': l,

[koda, 2014/07/29 15:34:29]

I misread "l" as the digit one; please rename.

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

Done.

+    };
+    return JSON.encode(map);
+  }
+}