Metrics API RFCs to eliminate Raw statistics #4

jmacd · 2019-06-28T22:56:44Z

No description provided.

Co-Authored-By: Isobel Redelmeier <iredelmeier@gmail.com>

bogdandrutu · 2019-06-30T00:14:35Z

Sorry, I am a bit lost. Does this represent 4 RFCs? If yes should we split them? Otherwise should we make them one RFC?

jmacd · 2019-07-01T17:40:49Z

Answered your question here:
open-telemetry/opentelemetry-specification#169

jmacd · 2019-07-01T17:41:21Z

I think we should review these as a block, and if any need to be removed from this PR, they will be.

iredelmeier

Overall LGTM

bogdandrutu

I think there is a lot of confusion about this, maybe worth discussing today in the SIG meeting. I will give a 10 minutes presentation about metrics.

bogdandrutu · 2019-07-02T13:33:25Z

0001-metric-pre-defined-labels.md

+
+## Explanation
+
+In the current proposal, Metrics are used for pre-aggregated metric types, whereas Raw statistics are used for uncommon and vendor-specific aggregations.  The optimization and the usability advantages gained with pre-defined labels should be extended to Raw statistics because they are equally important and equally applicable. This is a new requirement.


"Raw statistics" which probably is not a good name (I made it up, and probably confused more than helped) is not for "uncommon and vendor-specific aggregations". We use them for example in gRPC and HTTP metrics where we don't want the framework developers (gRPC devs) to define the aggregation and labels for the end user.

@bogdandrutu HTTP and gRPC metrics seem to me like they should be regular metrics/not "raw statistics". Using them in this way seems inline with, for instance, Prometheus practices - see, e.g., the metric and label naming guide, the label examples in their querying examples, and [suggestions around label usage](https://prometheus.io/docs/practices/instrumentation/#things-to-watch-out-for.

Could you clarify why you think that HTTP and gRPC are better served by raw statistics?

See one of the example I gave to you in the other comment. I think this is what I mentioned in the SIG call that we do have different understanding of the "raw statistics" (delayed metrics). We do not want the gRPC/HTTP devs to decide how the metrics are aggregated, we want them to only record all these measurements and let the application owner decide how these will be aggregated.

One important thing is that the result of the "raw statistics" (delayed metrics) after the "Views" are applied are regular metrics, the main difference being who decides what are the label keys and aggregation function to be used.

bogdandrutu · 2019-07-02T13:38:10Z

0001-metric-pre-defined-labels.md

@@ -0,0 +1,37 @@
+# Pre-defined label support for all metric operations
+
+Let all Metric objects (Cumulative, Gauge, ...) and Raw statistics support pre-defined label values.


For the "raw statistics" we don't know in advance all the label keys that will be used to break down these metrics. In OpenCensus what we did was to allow users to call record with what we called extra labels. We cannot do the same optimization that we do for the pre-defined metrics (aggregation, labels are pre-defined) because we don't know at compilation time the set of labels that we need to use. Also for "raw statistics" the final set of label values will be a subset of Tags (a.k.a DistributedContext) and the extraLabels.

At least from my understanding, the idea isn't that label values are known at compile time, but that label keys are known. Does that clarify things?

That is opposite to the idea of the "raw statistics" ("delayed metrics"). The whole point to support this is to allow deferred/delayed label keys and aggregation. See for example https://github.com/census-instrumentation/opencensus-specs/blob/master/stats/gRPC.md we do record all these measurements and recommend some views, but application owners (who configured the Views) can define any label keys they want. One example is when an A/B test is performed in ServiceA (ServiceA calls ServiceB), you would want to add the test_label as a label key for all rpc stats (maybe just some of them) to monitor the effects. But you cannot change the gRPC instrumented code so then we need to allow this mechanism.

That sounds like an issue with the gRPC instrumentation that can be solved with increased configurability, e.g., allowing end users to override the default label names upfront.

It is not about "not allowing" to override the default label names, is about adding extra labels that are propagated from different services with the context (tags/distributedContext). This is the whole idea. Also I mentioned the aggregation function (for example do I want to build a histogram with these buckets or that buckets, or just build a Sum).

If you try to come up with that "flexible configuration" you will see that you get very close to what we propose in this framework "raw statistics" + views.

+1000 to being able to pre-specify defaults for some tags, rather than having all the tags need to be re-checked at runtime. To be all technical... this feels like the metrics version of performing partial application.

bogdandrutu · 2019-07-02T13:42:44Z

0001-metric-pre-defined-labels.md

+
+## Internal details
+
+This RFC is accompanied by RFC 0002-metric-measure which proposes to create a new Metric type to replace Raw statistics.  The metric type, named "Measure", would replace the existing concept and type named "Measure" in the metrics API.  The new MeasureMetric object would support a `Record` method to record measurements.


I agree and also I talked with @SergeyKanzhelev about this. One important optimization for these metrics is "batch" recording. Because we don't know the labels and aggregation in advance what we do is we put every record entry into a producer-consumer queue and the consumer thread "applies" all defined views (views define the aggregation and labels for these metrics so that we can produce a Metric data). Alternative consideration is to do the work on the critical path and extract the necessary labels and apply the aggregation there, based on my experience with this system (used extensively in Google) this has more overhead than the previous implementation.

An option that I had in mind is to have Measure and MeasureCollection where MeasureCollection is a set of Measures that allow batch record for all the Measures in this collection.

bogdandrutu · 2019-07-02T13:47:31Z

0001-metric-pre-defined-labels.md

+
+## Open questions
+
+This RFC is co-dependent on several others; it's an open question how to address this concern if the other RFCs are not accepted.


I shared the same concern in my previous question on this PR. I think it makes more sense to have one or two independent RFC than a chain. I don't feel that strong right now during the review but probably when we merge this we can also merge the RFCs into one or two independent once.

bogdandrutu · 2019-07-02T13:50:31Z

0002-metric-measure.md

+
+Define a new Metric type named "Measure" to cover existing "Raw" statistics uses.
+
+## Motivation


I agree with the motivation see open-telemetry/opentelemetry-specification#145 which proposes to remove the Measurement class and move the Record method on the Measure.

bogdandrutu · 2019-07-02T14:06:07Z

0002-metric-measure.md

+
+## Explanation
+
+This proposal suggests we think about which aggregations apply to a metric independently from its type.  A MeasureMetric could be used to aggregate a Histogram, or a Summary, or _both_ of these aggregations simultaneously.  This proposal makes metric type independent of aggregation type, whereas there is a precedent for combining these types into one.


I think I confused you a bit because we did not define the Histograms/Summaries see open-telemetry/opentelemetry-specification#146.

The idea is the following, we offer support for users to record metrics where the developer that instruments the code can choose between exporting a pre-defined aggregation/label metrics (type A) or a "raw statistics" (type B) (no aggregation, labels pre-defined). The way how this decision is made tries to follow this rules:

If the metric is pre-aggregated (e.g. CPU usage is a counter in /proc) and no labels (here I mean that there are no labels used to break down this metric in /proc, but constant labels can be added to this metric if necessary. Then a type A metric should be used because we cannot apply any other aggregation (if you want to calculate rate which is possible for any counter (monotonic increasing metric) that can be done in all modern backends Prometheus, Stackdriver, etc.).

If the metric is recorded with every request (rpc latency, bytes received, bytes send) and the developer of the instrumentation is not the application owner (third-party library like OpenTelemetry itself) then type B should be used. This way the application owner can decide how to aggregate these metrics.

If the metric is a simple metric that does not make sense to aggregate in a different way (e.g. queue length - this is a gauge and probably does not require any labels to be used to breakdown) then a type A metric should be used.

bogdandrutu · 2019-07-08T20:36:44Z

We will have a meeting tomorrow (July 9) at 1pm PST. Please use https://lightstep.zoom.us/j/516112682

bogdandrutu · 2019-07-09T21:32:34Z

0001-metric-pre-defined-labels.md

@@ -0,0 +1,37 @@
+# Pre-defined label support for all metric operations


One thing that I remember during the meeting about this RFC is how for example gRPC will set the grpc_method. In OpenCensus we decided to set the method as a Tag and the measure will read the value from the tags not from the extra labels provided by gRPC. The main reason is that we want other metrics recorded for this request to be able to use the grpc_method tag as a label as well.

For the status we used the extra labels (the measure labels proposed in this RFC) because this is known at the end of the request and cannot be used in other metrics.

So the main label that a system like gRPC will add which is the request (method) name is read from the Tags (a.k.a. DistributedContext).

bogdandrutu · 2019-07-09T21:35:56Z

0004-metric-configurable-aggregation.md

@@ -0,0 +1,73 @@
+# Let Metrics support configrable, recommended aggregations


We need to allow users to configure the aggregation for the "raw statistics" as well as the set of labels. In the OpenCensus case the full set of labels supported are Tags (available in the context when the record method is called) + extraLables (specifically recorded with every request), and users should define a subset from there.

bogdandrutu · 2019-07-09T21:40:03Z

During the meeting there was a mention about CounterVec in Prometheus. Based on my understanding the CounterVec represents what in OpenTelemetry we call Counter and Prometheus Counter represents what we call a TimeSeries.

So we do support the same things for Counters and Gauges.

jmacd · 2019-07-20T00:04:53Z

I've put together a PR to demonstrate how I would satisfy these RFCs in Go.

jmacd/opentelemetry-go#2

I'd prefer not to drag out the individual threads above. We have a meeting to discuss on Monday.

I think RFC 0001 has good support so far.

RFC 0002 and 0004 created some confusion. I believe the requirement is to allow creation of metrics that do not have a semantic interpretation, to allow SDKs to do what's best and not necessarily to follow the programmer's recommendation. I agree with this goal, and see the main change here as a compromise. Let the programmer recommend a good default at the place where metrics are defined, but call it "advisory". This way the SDK can still implement the behavior it wants, potentially disregarding the programmer's input.

jmacd · 2019-07-31T04:53:12Z

I will merge this in the "proposed" state, as discussed in the spec meeting today.

jmacd · 2019-07-31T04:53:50Z

@iredelmeier Actually, I can't merge this. I wasn't sure where to write the current status, so I put it at the top. Will you have a look?

bhs · 2019-08-04T20:45:51Z

@jmacd what's the current status on this PR? What are we blocked on, if anything? Thanks in advance...

bogdandrutu · 2019-08-12T18:20:38Z

Some highlevel comments before we can merge this:

Use the path suggested in the readme text/XYZ.
Consider to merge this RFCs into one.
Consider to add details about the MeasureBatch.

jmacd · 2019-08-12T20:52:22Z

@iredelmeier I've assigned numbers 3, 4, 5, and 6 to these RFCs, which I think can now be submitted in the Status: proposed state as discussed at this morning's otel-metrics meeting.

bogdandrutu

Sorry for being probably a bit late into this. I feel that I am not 100% confident on the last RFC, can we split this PR and merge the first 3 which we have no objections and discuss a bit more the 4th one?

bogdandrutu · 2019-08-12T22:15:51Z

text/0004-metric-measure.md

+
+This change, while it eliminates the need for a Raw statistics concept, potentially introduces new required concepts.  Whereas Raw statistics have no directly-declared aggregations, introducing MeasureMetric raises the question of which aggregations apply.  We will propose how a programmer can declare recommended aggregations (and good defaults) in RFC 0006-configurable-aggregation.
+
+## Prior art and alternatives


I think based on the discussions we do agree that Measure is different than Summary/Histogram. Measure can produce a Summary or Histogram but can also produce a Counter.

bogdandrutu · 2019-08-12T22:16:37Z

text/0005-eliminate-stats-record.md

+There are two reasons to maintain a low-level API that we know of:
+
+1. For _generality_.  An application that forwards metrics from another source may need to handle metrics in generic code.  For these applications, having type-specific Metric handles could actually require more code to be written, whereas the low-level `stats.Record` API is more amenable to generic use.
+1. For _atomicity_.  An application that wishes to record multiple statistics in a single operation can feel confident computing formulas based on multiple metrics, not worry about inconsistent views of the data.


We decided that we will support a MeasureBatch.

bogdandrutu · 2019-08-12T22:23:41Z

text/0006-metric-configurable-aggregation.md

+1. Unless the programmer declares otherwise, suggesting a default aggregation
+   1. For Gauges: LAST_VALUE is interesting, SUM is likely not interesting
+   1. For Cumulatives: SUM is interesting, LAST_VALUE is likely not interesting
+   1. For Measures: all aggregations apply, default is MIN, MAX, SUM, COUNT.


Don't think you need min/max. Also based on the OpenMetrics and also Prometheus/Stackdriver (it has a max/min but does not use it) they do not have a min/max.

bogdandrutu · 2019-08-12T22:28:09Z

Please also rebase

jmacd · 2019-08-12T23:21:21Z

OK. I will merge the first three into one RFC and leave the last one separate. I will post this later today.

jmacd · 2019-08-13T00:21:35Z

Please take another look @bogdandrutu. I've rebased and merged the first three RFCs into one. The remaining RFCs are number 3 and 4.

bogdandrutu · 2019-08-13T00:24:24Z

Sorry probably my English was not the best. I meant to merge this PR with the first 3 RFCs (or currently I think we should merge the PR number 3) and open a new PR with RFC 4. I have some questions and concerns about letting users to define the aggregation during the instrumentation.

Does that make sense?

jmacd · 2019-08-13T04:59:45Z

I thought our goal was to merge these PRs so that individual RFCs can be assigned unique numbers, and then we can discuss them via new PRs against these drafts, which will have Status: proposed. There are two RFCs and we can discuss the now-combined-for-approval RFC 3 independently from the still-needs-discussion RFC 4 about aggregation support.

* Four metrics RFCs to eliminate raw statistics * Create 4 RFC drafts on metrics * Update 0001-metric-pre-defined-labels.md Co-Authored-By: Isobel Redelmeier <iredelmeier@gmail.com> * Update 0001-metric-pre-defined-labels.md Co-Authored-By: Isobel Redelmeier <iredelmeier@gmail.com> * Refinements * Set status to proposed * Assign numbers 3-4-5-6 * Renumber refs * Update status styling * Fix misspellings * Combine the first three into one

jmacd and others added 6 commits June 27, 2019 14:31

Four metrics RFCs to eliminate raw statistics

7f2937e

merge master

b03b87a

Create 4 RFC drafts on metrics

6fc4353

Update 0001-metric-pre-defined-labels.md

04acacb

Co-Authored-By: Isobel Redelmeier <iredelmeier@gmail.com>

Update 0001-metric-pre-defined-labels.md

fb47377

Co-Authored-By: Isobel Redelmeier <iredelmeier@gmail.com>

Refinements

8f6917c

jmacd requested review from AloisReitbauer, bogdandrutu, c24t, carlosalberto, iredelmeier, reyang, SergeyKanzhelev, songy23, tedsuo and yurishkuro as code owners June 28, 2019 22:56

jmacd mentioned this pull request Jul 1, 2019

Review Metrics API RFCs 1-4 open-telemetry/opentelemetry-specification#169

Closed

iredelmeier approved these changes Jul 2, 2019

View reviewed changes

bogdandrutu reviewed Jul 2, 2019

View reviewed changes

bogdandrutu reviewed Jul 9, 2019

View reviewed changes

c24t mentioned this pull request Jul 16, 2019

Add metrics API open-telemetry/opentelemetry-python#48

Closed

jmacd mentioned this pull request Jul 19, 2019

Metrics proposal for Go jmacd/opentelemetry-go#2

Closed

iredelmeier referenced this pull request in iredelmeier/oteps Jul 26, 2019

Add explanation of the spec layout, etc. (#4)

e4886e7

Merge branch 'master' into jmacd/metrics_part1

04e1342

Set status to proposed

89a8fef

c24t mentioned this pull request Jul 31, 2019

Metrics API open-telemetry/opentelemetry-python#68

Closed

jmacd added 5 commits August 12, 2019 13:39

Upstream

1665bfe

Assign numbers 3-4-5-6

3b21a6e

Renumber refs

69671c4

Update status styling

b938b3a

Fix misspellings

c28d3bc

bogdandrutu approved these changes Aug 12, 2019

View reviewed changes

Combine the first three into one

bc2ce91

Merge branch 'master' into jmacd/metrics_part1

cb8488f

bogdandrutu merged commit 25ab28a into open-telemetry:master Aug 13, 2019

lzchen mentioned this pull request Aug 14, 2019

Metrics API with RFC 0003 open-telemetry/opentelemetry-python#87

Merged

jmacd deleted the jmacd/metrics_part1 branch September 13, 2019 18:16


		## Explanation

		In the current proposal, Metrics are used for pre-aggregated metric types, whereas Raw statistics are used for uncommon and vendor-specific aggregations. The optimization and the usability advantages gained with pre-defined labels should be extended to Raw statistics because they are equally important and equally applicable. This is a new requirement.

		@@ -0,0 +1,37 @@
		# Pre-defined label support for all metric operations

		Let all Metric objects (Cumulative, Gauge, ...) and Raw statistics support pre-defined label values.


		## Internal details

		This RFC is accompanied by RFC 0002-metric-measure which proposes to create a new Metric type to replace Raw statistics. The metric type, named "Measure", would replace the existing concept and type named "Measure" in the metrics API. The new MeasureMetric object would support a `Record` method to record measurements.


		## Open questions

		This RFC is co-dependent on several others; it's an open question how to address this concern if the other RFCs are not accepted.


		Define a new Metric type named "Measure" to cover existing "Raw" statistics uses.

		## Motivation


		## Explanation

		This proposal suggests we think about which aggregations apply to a metric independently from its type. A MeasureMetric could be used to aggregate a Histogram, or a Summary, or _both_ of these aggregations simultaneously. This proposal makes metric type independent of aggregation type, whereas there is a precedent for combining these types into one.

		@@ -0,0 +1,73 @@
		# Let Metrics support configrable, recommended aggregations


		This change, while it eliminates the need for a Raw statistics concept, potentially introduces new required concepts. Whereas Raw statistics have no directly-declared aggregations, introducing MeasureMetric raises the question of which aggregations apply. We will propose how a programmer can declare recommended aggregations (and good defaults) in RFC 0006-configurable-aggregation.

		## Prior art and alternatives

Metrics API RFCs to eliminate Raw statistics #4

Metrics API RFCs to eliminate Raw statistics #4

Conversation

jmacd commented Jun 28, 2019

bogdandrutu commented Jun 30, 2019

jmacd commented Jul 1, 2019

jmacd commented Jul 1, 2019

iredelmeier left a comment

Choose a reason for hiding this comment

bogdandrutu 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

iredelmeier Jul 2, 2019 • edited Loading

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 Jul 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bogdandrutu commented Jul 9, 2019

jmacd commented Jul 20, 2019

jmacd commented Jul 31, 2019

jmacd commented Jul 31, 2019

bhs commented Aug 4, 2019

bogdandrutu commented Aug 12, 2019

jmacd commented Aug 12, 2019

bogdandrutu 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

bogdandrutu commented Aug 12, 2019

jmacd commented Aug 12, 2019

jmacd commented Aug 13, 2019

bogdandrutu commented Aug 13, 2019

jmacd commented Aug 13, 2019

iredelmeier Jul 2, 2019 •

edited

Loading