Metrics specification updates

[jmacd]

This (draft) specification is pulled together from RFCs 0003, 0008, 0009, 0010, and a still-pending RFC on LabelSet as specified here.

https://github.com/open-telemetry/oteps/blob/master/text/0003-measure-metric-type.md
https://github.com/open-telemetry/oteps/blob/master/text/0008-metric-observer.md
https://github.com/open-telemetry/oteps/blob/master/text/0009-metric-handles.md
open-telemetry/oteps#52
open-telemetry/oteps#51
open-telemetry/oteps#49

resolves 83
resolves 144
resolves 145
resolves 146

[a-feld]

I had trouble understanding the difference between a recorder and a handle? Based on some of the discussions it sounds like this is an attempt to unify the instrument type methods into a single Record(value) interface. If that's true, I wonder if it's possible to omit this section entirely and instead unify Add, Set, and Record into a single Record interface?

[jmacd]

I re-worded this section. I think the term Recorder was confusing, and I've replaced it by saying it is language-specific, since it comes down to precisely how the API implements handles and the natural way this is expressed in the language. Seeking to reduce the number of terms overall, I replaced the "Recorder" string with "Handle". This is now described as Meter.NewHandle(Instrument, LabelSet) -> language-specific.

[a-feld]

Rather than having separate interfaces for each instrument type, would the interface be better if we converged on a single Record interface?

Counter.Record(value), Gauge.Record(value), Measure.Record(value) so that all instruments could share a common interface.

[jmacd]

Each language can decide how best to approach this, in other words.

[jmacd]

(This refers to a recent rephrasing of how the Meter returns new handles. It's now declared as language-specific because it doesn't matter to the end user.)

[lzchen]

Shouldn't we use only one indicator of whether or not negative values are supported? Either NonNegative or Signed as a boolean.

[jmacd]

I agree--but this may be an implementation detail. Logically, each kind of instrument has two options so we only need one bit to store it. Still, I think we want to use names like NonNegative vs Signed to describe the two options. I found myself conflicted over how to describe this, definitely want your suggestions.

[AloisReitbauer]

Why don't we use absolute ? This by definition means that the value is not negative. Signed for me describes a data type.

[jmacd]

I'm happy to continue this discussion. If we use Absolute it describes the default scenario, where measured values are >= 0. What do you name the opposite, non-default case? That's what we need a name for. I'm not sure. "NonAbsolute"?

[lzchen]

The wording of the disabled functionality is kind of confusing. There is a disabled flag which indicates to the SDK whether to DISABLE the instrument. So by default, the value of disabled FALSE, so the SDK by default will have the instrument as enabled?

[jmacd]

I pulled this into a dedicated section, now containing:

Instruments support a `Disabled` option.  Metric instruments
configured with `Disabled: True` are considered "off" unless
explicitly requested via SDK configuration.  Unless the SDK is
otherwise configured to enable a disabled metric, operations on these
instruments will be treated as no-ops.

Metric instruments are enabled by default (i.e., have `Disabled: False`).

[lzchen]

Unit is usually required no? @c24t

[GreyCat]

Vast majority of existing metric systems implementations have no notion of "unit" anyway, so at least from retrofitting perspective I would keep it as "optional". Also, there are quite a few true unitless quantities out there — like ratios, percentages, power factors, etc.

[Oberon00]

Dimensionless quantities have the unit "1": https://en.wikipedia.org/wiki/Dimensionless_quantity

[jmacd]

I could add that the unit defaults to "1". The notion of units was carried over from OpenCensus, which supported "1", "ms", and "bytes".

[c24t]

Units are required in OC, where we use the UCUM definitions. This is a bit of information with well defined semantics, so we can't just stuff it in the description. As I understand it, the goal is to let vendors display different types of metrics differently, or even aggregate metrics with compatible types.

Defaulting to "1" makes sense to me, but I suspect we're going to cause some problems in practice by not distinguishing between dimensionless metrics and metrics where the unit exists, but is unspecified.

See the OC metrics proto, relevant to some other comments here.

[jmacd]

I added "Metric units are defined according to the
UCUM specification."

[lzchen]

Are keys optional or mandatory? Above there is the concept of "required" label keys. How do those look like in the signature?

[jmacd]

I updated the section above,

Metric instruments list an optional set of _required label keys_ to
support potential optimizations in the metric export pipeline.  This
is an option in the sense that the empty set (of required label keys)
is a valid setting.  If required label keys are provided, it implies
that the SDK can infer a value for the required label from the
`LabelSet`.

To support efficient pre-aggregation in the metrics export pipeline,
we must be able to infer the label value from the `LabelSet`, which
implies that they are not dependent on dynamic context.  Required
label keys address this relationship.  The value of a required label
key is explicitly unspecified unless it is provided in the `LabelSet`,
by definition, and not to be taken from other context.

[GreyCat]

It's very convoluted wording for the first paragraph:

• "Metrics API" is introduced with as "reporting diagnostic measurements" using "instruments", known as "Counters, Gauges and Measures".
• "Instruments" are being defined then as "objects [...] used to gain visibility into [...] metrics", which is a classic "A is B", "B is A" definition fallacy.
• No understanding of what "Counters, Gauges and Measures" are until line 86, that's 3 sections later.

All this puts extra pressure on the reader, and is actually pretty hard to grasp firmly if you don't know a lot of prior context already — and, to my understanding, the very point of specification is to let those who don't know to learn.

[jmacd]

I'll try to clear this up.

[jmacd]

I've replaced the first paragraph with:

The _Metrics API_ supports reporting diagnostic measurements using
three basic kinds of instrument.  "Metrics" are the thing being
produced--mathematical, statistical summaries of certain observable
behavior in the program.  "Instruments" are the devices used by the
program to record observations about their behavior.  Therefore, we
use "metric instrument" to refer to a program object, allocated
through the API, used for recording metrics.  There are three distinct
instruments in the Metrics API, commonly known as Counters, Gauges,
and Measures.

[GreyCat]

Nit: API can't "provide the [...] SDK". SDK is "software development kit", API is "application programming interface". "Interface" can be implemented by certain code. "Providing the SDK" typically refers to making it a certain file available for download.

[jmacd]

I've rewritten this first sentence as

The user-facing OpenTelemetry API consists of an SDK-independent part
for defining metric instruments and a part named `Meter` that is
implemented by the SDK.  

[GreyCat]

I'm not sure if pushing "primary interest" to the user is the right thing to do.

For example, consider monitoring NTP jitter offset.

• Does the measurement represent a quantity of something? Is it also non-negative?

Yes, it is quanity of, say, microseconds. It can be negative or positive.

• Is the sum a matter of primary interest?

Sum might not be "primary interest", but average is definitely a primary interest here, and that requires collection of sums.

• Is the event count a matter of primary interest?

On larger scale, we might not be able to ensure consistent emission (like once per certain interval), and it's much easier to just store count + sum, and be able to derive average as sum / count.

• Is the distribution (p50, p99, etc.) a matter of primary interest?

Actually, yes, distribution is also a matter of primary interest.

So, what do I choose as a user?

[jmacd]

There's some philosophizing here. I believe that "negative quantity" is not meaningful without a lot of context and would prefer "quantity" to be a strictly positive number. Usually a negative quantity indicates the number describes a change of some quantity. I recognize that this is my interpretation and we need to find common ground.

I chose "primary interest" as a device to explain this because it's clear from the API/SDK separation that any metric instrument could conceivably perform any task. The choice should be driven by what the user wants to achieve.

In RFC 0004 (now removed) I proposed that users should be able to configure which aggregations they want. If this were supported, you could create the instrument of your choice based on which verb you want to read in your code (Add vs. Record), and then say that you want both COUNT and SUM in order to compute an average, for example . See https://github.com/open-telemetry/oteps/blob/accb510feeecb10c886ef38c44c58639c5c7ea48/text/0004-metric-configurable-aggregation.md. This RFC was rejected by the group.

So to compute an average either requires:

1. Two counters. Use one counter for the COUNT and one for the SUM.
2. Custom behavior. The SDK can be told to compute an average from any of the metric kinds, if so desired.

The point about Distribution is that it's an expensive computation, as it can't be summarized in O(1) per interval. If this is of primary interest, you should choose a Measure-kind metric, but be prepared for an expensive instrument. It should be clear that Measure-kind metrics are expensive whereas the other two are not.

I'm really interested in how you think this could improve, or whether you think RFC 0004 should be revisited. I personally like the proposal in 0004 and have frequently used metric instruments configured for MAX, SUM, and COUNT aggregations because I think it's a nice / cheap solution. Given that 0004 was rejected, I'm leaning toward the "Custom behavior" solution, which is to say that I want an SDK that let's me configure a metric to export MAX, SUM, and COUNT--I would choose to use a Measure-kind of instrument in my code because I prefer to read Record() when updating this kind of max-sum-count metric.

[GreyCat]

Usually a negative quantity indicates the number describes a change of some quantity.

Not necessarily, it might be a delta between two values, not necessarily a "change". And certain scales (a very simple example: temperature in C and F) genuinely go below 0.

For everything else — i.e. aggregation strategies holistically — I completely agree that it's probably out of scope of this GH discussion and I'll try to reach out to you to schedule some kind of discussion, if that's of any interest.

[jmacd]

Continuing this tangent, I think that Gauge is how I would choose to monitor NTP jitter offset. Each time the jitter value changes, you would Set() the new value when it changes. A monitoring system would know the current value as a function of time. You mentioned wanting to know the average, but there are two well-defined averages.

There's an average-set value, i.e., the sum divided by the event count, but to make use of this data you would have to continuously set the variable.

There's a time average, which would take into account the value times its duration being current. If the value spends 10 seconds at value X and 1 second at value Y, we want (10X+Y)/11, not (X+Y)/2.

Gauges are meant for this second case, which I think best reflects your intention. If you want to look at the distribution of current values, you could:

(1) ask your monitoring system about a time-weighted distribution (assumes the value varies slowly)
(2) reconfigure your SDK to compute a distribution from a gauge (which is a fine, if rare, thing to do)
(3) decide you always care about this distribution, and that it does not vary slowly. in this case, I think you should use a Measure metric to indicate that the distribution is your primary interest.

I'd just use a gauge in this case and go with strategy (1).

[GreyCat]

It's probably too late to argue, but, in my opinion, "Handle" is not an ideal naming choice — it's too generic, and it clashes with so many things out there.

Also, phrasing it as "Metric instruments support a Handle interface" is somewhat misleading and doesn't really give much clarity to the reader. From what's going after that, it eventually becomes clearer than "Metric instruments" actually can be included in a "handle" (and even that is not 100% clear — do we mean class or object here?)

[jmacd]

It's not too late to change the term "Handle".

For context, the OpenCensus system used the term "Entry". The method was "GetEntry". One objection to this term is that "Entry" implies there is a cell in a table or map somewhere, which is not actually a requirement.

The earlier OpenTelemetry specification used "TimeSeries" instead, which we've discussed as being confusing. The exported data is called a "TimeSeries", but we found it confusing that the programmatic object was also named TimeSeries.

We discussed changing this to "Handle" in the 8/21 metrics working group meeting.

Important Note. This is only part of the specification, there is not a requirement that a concrete type have this name in any language's API. The return of GetHandle() may be an interface named Counter, for example. So the only term that really enters play for the end user is "GetHandle". Other terms that work (for me personally) would be "Reference". This is a reference to an (Instrument, LabelSet) after all.

[pauldraper]

Handler is truly a terrible name.

It could have been called "Reference", "Value", or "Thing" with equal levels of clarity.

I wonder if the decorator pattern should be used instead, so there is only one interface: Instrument. Or if the whole idea should be scrapped in favor of LevelSet.

If it's kept, at least name it something sensible, like LabeledInstrument.

[jmacd]

@GreyCat Thank you. I appreciate your feedback and will address all of your suggestions in an editing pass later tonight. I'll ping you when it's ready.

[bogdandrutu]

I would suggest to move all the export and SDK related information to sdk-metrics

[jmacd]

I needed to introduce this concept in order to explain the purpose of "required label keys". It's not specified here, I mean.

[jmacd]

I think it is not a good idea to enforce all the functions to accept a tracer/meter (or have a tracer/meter associated with every request which forces all APIs to accept a tracer/meter for every function). We should ensure that if I pass a Meter/Tracer instance in the ctor of a class that is used to handle multiple requests I can cache it and use for all the requests.

@bogdandrutu I said nothing about enforcing that practice. I said that should be possible for a programmer. I wouldn't pass it to every function, but I would pass it to a ctor of a class. Then, inside that class, I do not want to allocate one field per metric instrument or one field per instrument handle.

[yurishkuro]

In order to capture measurements, you need two more things, the list-of-labels and the meter itself.

@jmacd apologies, I may have misunderstood your proposal. It would be very useful if you could summarize the whole proposal with a sample/primer of the code, i.e. a single block of code that shows all aspects of the proposal (with comments). I think reading code is much easier than peeling apart the design from the long descriptions.

[jmacd]

@bogdandrutu and I agreed to cut out a bunch of still-not-agreed stuff while I rework it, but this leaves a lot of the important things we have agreed to in this PR and placeholders for the remaining stuff.

[bogdandrutu]

Overall looks good, some more details will be clarified in the future updates.

[bogdandrutu]

Not 100% sure what will be in these 2 documents. Can we add them later when needed?

[jmacd]

I'm editing them in this PR, otherwise we have an incomplete specification.

I've updated the first of the documents with the major change we discussed yesterday.

https://github.com/open-telemetry/opentelemetry-specification/pull/250/files?short_path=dd80165#diff-dd80165873789e299283d20adcb24570

The second document will contain all the method-level detail and is the least important.

[lzchen]

To clarify, what was the major change that was discussed?

[jmacd]

Previously, metric instruments were allocated as pure API objects without an SDK relationship. This allowed them to be legitimate static variables.

The change is to require instruments to be allocated through the SDK, with reasons: (1) it automatically registers instruments, so they can be exported in the "zero" state, (2) it automatically namespaces the instruments, so they can co-exist with the same name from other components, (3) it shortens the call-site to avoid passing a meter more than once.

All the LabelSet stuff remains, though I want to propose shortening DefineLabels to just Labels, which I'll do as I continue this work tomorrow.

[bogdandrutu]

I thought that you will do this in a separate PR :)) Also advertise for that :). Do you think it is too much to submit the api-metrics-user.md with just a TODO and start a new PR to discuss that?

Very hard to see everything.

[jmacd]

I removed the new content for another PR.

[bogdandrutu]

Maybe this section should just be here instead of a separate doc.

[jmacd]

It'll get very large.

[jmacd]

Ack.

[bogdandrutu]

I think we need to clarify a bit because users may understand that monotonic allows positive values vs non-monotonic allows positive/negative. But what we want is the set value to be larger than the previous set value.

[bogdandrutu]

Merging this as it is, then we will incrementally update it.

[bogdandrutu]

@jmacd consider to touch this PR (maybe just simply squash the commits) to trigger a new CLA check.

[bogdandrutu]

Closing and reopening the PR fixed the CLA check.

[AloisReitbauer]

i know this is what other tools like Prometheus are doing. I find the use of counter, gauges and metrics. always a bit counter intuitive names. It would make sense to define these metric types in more detail for people reading the spec first - i.e. how is a counter, gauge etc. defined and which characteristics do they have?

[jmacd]

This was a conundrum to me. The industry has very little in the way of common terminology, and I had to start somewhere. The purpose of this document is to get clear about what these mean, but I'm not sure how to avoid these terms in the front-matter. I'm open to suggestions as to how I can avoid these terms so soon in the document.

[AloisReitbauer]

Usually alerting and event generation is done at the backend and not at the telemetry level. Do I get something wrong here

[jmacd]

No, I'm saying that this metric data usually feeds into those systems, but there are other uses for metrics data in the context of a tracing system, for example. The point of this paragraph is to justify the API/SDK separation.

[AloisReitbauer]

Shouldn't this be produce rather than report?

[jmacd]

Yes. I'm going to put minor edits into #299.

[AloisReitbauer]

I think this is an esential point. I wonder whether language that is more focussed on decoupling collection from presentation would make more sense.

[jmacd]

I'm open to suggestions. I've been trying to stick with purely semantic descriptions--I'm not sure the collection vs. presentation distinction is the most important.

[AloisReitbauer]

You cannot report deltas in prometheus because they would depend on scrape timing. If counters were actual time series this problem might go away.

Another downside of the Prometheus approach is that data fidelity depends on scrape time rather than reporting precision.

[AloisReitbauer]

Then they are technically a counter - right?

[jmacd]

Yes. This distinction confused me for a while, too. If we think of gauges as things that we set based on some computed value, then if there's a monotonic value that is expensive to compute, you can use a monotonic gauge. It's a counter, but the caller is not required to remember the last value.

In a system like Prometheus, these export as counters. @bogdandrutu please add any clarifying remarks.

[AloisReitbauer]

Why isn't this an API method but a separate type? We should also add a use cases here.

[jmacd]

OK, more uses-cases will be required in the spec, I agree. They are listed in the OTEP: https://github.com/open-telemetry/oteps/blob/master/text/0008-metric-observer.md

The reason this can't be the same type as a Gauge is that we want to disallow Set() calls on observers, and you can't get handles on observers.

[AloisReitbauer]

Why don't we use absolute ? This by definition means that the value is not negative. Signed for me describes a data type.

[jmacd]

@AloisReitbauer I will respond to your feedback here, but since this PR was merged already I suggest future discussion goes towards #299

I'm happy to edit this spec in the future, thanks for your input.