Initial draft for a metrics, an API for exporters. #53

bogdandrutu · 2018-04-15T22:18:05Z

This API can be seen as the api for the metrics/stats exporters, and it is not intended to replace the stats API.

The metrics API is intended to be used by the stats API as well as other metric systems (dropwizard, etc.) to send data using the census metric exporters framework.

Why do we need a different API for exporters:

The current OpenCensus stats API is hard to be used for real GAUGE metrics (any pre-aggregated metric, e.g. machine-cpu-usage).
The current OpenCensus stats exporter API is focus only on supporting OpenCensus stats, but the library needs to support different external sources (dropwizard, etc.) and supporting these sources is very hard with the current OpenCensus stats exporter API.

Here is an example of how to map ViewData into the Metrics:

MetricPoint
  MetricDescriptor
     Name <- ViewData.View.name
     Description <- ViewData.View.description
     Unit <- ViewData.View.Measure.unit
     Type <- Cumulative (except LastValue which is GAUGE)
  TimeSeries
     Labels <- {ViewData.rows, ViewData.View.columns}
  CumulativePoint
     Value <- ViewData.AggregationData.value (for DistributionValue the bucket_bounds are defined in the ViewData.View.DistributionAggregation.bucket_bounds)
     Start_time <- ViewData.start_time
     End_time <- ViewData.end_time

bogdandrutu · 2018-04-15T22:23:30Z

This PR will allow to make progress on census-instrumentation/opencensus-specs#59

deadtrickster · 2018-04-15T23:18:39Z

If I want to push number of open fds to opencensus, how I set timestamps?

What "push" actually means? Will those Metrics be queried by Exporters somehow? There must be a Metrics registry somewhere for this.

songy23

A few nits but overall LGTM.

songy23 · 2018-04-16T16:28:07Z

build.gradle

@@ -21,6 +21,7 @@ sourceSets {
    main {
        proto {
            // In addition to the default 'src/main/proto'
+            srcDir 'opencensus/proto/metrics'


Should this be under stats/metrics?

songy23 · 2018-04-16T16:28:34Z

opencensus/proto/metrics/BUILD.bazel

@@ -0,0 +1,33 @@
+# Copyright 2017, OpenCensus Authors


nit: s/2017/2018

songy23 · 2018-04-16T16:31:27Z

opencensus/proto/metrics/metrics.proto

+
+// Defines a metric type and its schema.
+message MetricDescriptor {
+  // The metric type, including its DNS name prefix. It must be unique.


Should we mention the constraint (and possibly, sanitization rules) on MetricDescriptor names?

Added a TODO for the moment.

songy23 · 2018-04-16T16:34:10Z

opencensus/proto/metrics/metrics.proto

+}
+
+message Label {
+  // The label name.


Similarly, should we mention the constraints here?

Similar, added a TODO for the moment.

songy23 · 2018-04-16T16:34:40Z

opencensus/proto/metrics/metrics.proto

+message Point {
+  oneof point {
+    GaugePoint gauge_point = 1;
+    CumulativePoint points = 2;


nit: s/points/cumulative_point

bogdandrutu · 2018-04-16T17:19:43Z

@deadtrickster updated the comment to not mention push/pull model yet. The way how I envision this framework is that you have:

public class MetricSource {
  public MetricPointSet getMetricPointSet();
}

public class MetricsExporter {
  public void registerMetricSource(MetricSource);
}

The way it will work is based on how exporters are pull (Prometheus) vs push (Stackdriver). In case of Prometheus we wait for the request to come and then query all the registered MetricSources to get the MetricPoints, in case of Stackdriver we set an exporting interval on the Stackdriver exporter (e.g. interval=10sec) then we configure a timer to wakeup every 10seconds and pull the data from all MetricSources.

For reading the "fds" it depends on how you implement the MetricSource that reads the "fds", one example is to read the fds when the metrics framework calls getMetricPointSet then that will be the timestamp.

rakyll · 2018-04-16T17:58:44Z

opencensus/proto/stats/metrics/metrics.proto

+}
+
+// Defines a MetricPoint which has one or more timeseries for a single metric.
+message MetricPoint {


Shouldn't this be named MetricTimeSeries? MetricPoint makes it sound like it represents only one point.

Why not just "Metric"?
I agree that "point" is confusing.

went with Metric/MetricSet. Let me know if it makes sense.

g-easy · 2018-04-17T23:36:51Z

opencensus/proto/stats/metrics/metrics.proto

+}
+
+// Defines a MetricPoint which has one or more timeseries for a single metric.
+message MetricPoint {


Why not just "Metric"?
I agree that "point" is confusing.

g-easy · 2018-04-17T23:37:02Z

opencensus/proto/stats/metrics/metrics.proto

+// Defines a MetricPoint which has one or more timeseries for a single metric.
+message MetricPoint {
+  // The description of the metric.
+  MetricDescriptor desctriptor = 1;


trip -> rip

g-easy · 2018-04-17T23:37:34Z

opencensus/proto/stats/metrics/metrics.proto

+// A collection of data points that describes the time-varying values
+// of a metric.
+message TimeSeries {
+  // The set of label values that uniquely identify this metric. Apply to all


this metric -> this timeseries

semistrict · 2018-04-18T17:19:02Z

opencensus/proto/stats/metrics/BUILD.bazel

+java_proto_library(
+    name = "metrics_proto_java",
+    deps = [":metrics_proto"],
+)


line ending missing

semistrict · 2018-04-18T17:19:26Z

opencensus/proto/stats/metrics/metrics.proto

+option java_package = "io.opencensus.proto.metrics";
+option java_outer_classname = "MetricsProto";
+
+// A collection of MetricPoint's.


No need for apostrophe here I think

semistrict · 2018-04-18T17:21:58Z

opencensus/proto/stats/metrics/metrics.proto

+message TimeSeries {
+  // The set of label values that uniquely identify this metric. Apply to all
+  // points.
+  repeated Label label = 1;


Are duplicate keys permitted? If so, we should define here the behavior (i.e. last one wins) or we should switch to type map<string, string>. If not, we should say that duplicates are an error.

My opinion would be to copy what map<string, string> does or switch to that

…well as other metric systems.

bogdandrutu requested review from rakyll, semistrict, songy23, dinooliva, sebright, g-easy and deadtrickster April 15, 2018 22:18

songy23 approved these changes Apr 16, 2018

View reviewed changes

rakyll reviewed Apr 16, 2018

View reviewed changes

deadtrickster approved these changes Apr 16, 2018

View reviewed changes

g-easy reviewed Apr 17, 2018

View reviewed changes

semistrict approved these changes Apr 18, 2018

View reviewed changes

Bogdan Drutu added 3 commits April 19, 2018 11:14

Initial draft for a metrics API that can be used by the stats API as …

a3629f5

…well as other metric systems.

Fix comments, move metrics under stats/metrics

3128cad

Fix more comments. Rename MetricPoint to Metric.

9262bd4

bogdandrutu force-pushed the metrics branch from c08c47d to 9262bd4 Compare April 19, 2018 18:14

bogdandrutu merged commit c5d48ac into census-instrumentation:master Apr 30, 2018

semistrict mentioned this pull request Jun 15, 2018

Prometheus View of runtime.MemStats census-instrumentation/opencensus-go#784

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial draft for a metrics, an API for exporters. #53

Initial draft for a metrics, an API for exporters. #53

bogdandrutu commented Apr 15, 2018 •

edited

Loading

bogdandrutu commented Apr 15, 2018

deadtrickster commented Apr 15, 2018

songy23 left a comment

songy23 Apr 16, 2018

bogdandrutu Apr 16, 2018

songy23 Apr 16, 2018

bogdandrutu Apr 16, 2018

songy23 Apr 16, 2018

bogdandrutu Apr 16, 2018

songy23 Apr 16, 2018

bogdandrutu Apr 16, 2018

songy23 Apr 16, 2018

bogdandrutu Apr 16, 2018

bogdandrutu commented Apr 16, 2018

rakyll Apr 16, 2018

g-easy Apr 17, 2018

bogdandrutu Apr 19, 2018

g-easy Apr 17, 2018

g-easy Apr 17, 2018

bogdandrutu Apr 19, 2018

g-easy Apr 17, 2018

bogdandrutu Apr 19, 2018

semistrict Apr 18, 2018

bogdandrutu Apr 19, 2018

semistrict Apr 18, 2018

bogdandrutu Apr 19, 2018

semistrict Apr 18, 2018

semistrict Apr 18, 2018

bogdandrutu Apr 19, 2018

Initial draft for a metrics, an API for exporters. #53

Initial draft for a metrics, an API for exporters. #53

Conversation

bogdandrutu commented Apr 15, 2018 • edited Loading

bogdandrutu commented Apr 15, 2018

deadtrickster commented Apr 15, 2018

songy23 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bogdandrutu commented Apr 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bogdandrutu commented Apr 15, 2018 •

edited

Loading