Add temporality to MetricDescriptor #140
Conversation
MrAlias
requested review from
arminru,
bogdandrutu,
c24t,
carlosalberto,
iredelmeier,
jmacd,
reyang,
SergeyKanzhelev,
tedsuo,
tigrannajaryan and
yurishkuro
as code owners
Co-Authored-By: Tigran Najaryan <4194920+tigrannajaryan@users.noreply.github.com>
Use an enum name suffix instead of prefix for the default values.
MrAlias
changed the title
|
Would be good later to add a table with combination of temporality and types and explain what it is possible what it is not and maybe map to some examples. |
| // 8. The 1 second collection cycle ends. A metric is exported for the | ||
| // number of requests received over the interval of time t_0+1 to | ||
| // t_0+2 with a value of 2. | ||
| DELTA = 2; |
There was a problem hiding this comment.
how is the interval represented when reporting delta? Is it by using start_time_unix_nano and time_unix_nano fields in Int64DataPoint etc, ?
There was a problem hiding this comment.
That is the plan. Planning to address this issue in a following PR (MrAlias@5cfae02).
The ask has been to split up #137 into smaller changes. The assumption that there would be breaking changes or inconsistencies between PRs was something planned and accepted.
There was a problem hiding this comment.
Thanks for explaining delta vs cumulative with actual examples!
Can you correct the following as well:
-
The part of this proto file which describes
Int64DataPoint,DoubleDataPoint,SummaryDataPointetc. is not updated to reflect the changes in this PR. They still refer to gauge/counter types which are replaced in this PR. -
A sample message example for each of the
Temporatily- showing the values forstart_time_unix_nanoandtime_unix_nano, so that there would be confusion.
We agree to do that in a separate PR. |
|
@cijothomas are you happy with the responses? Please remove the request changes if so :) |
Yes! Just did it. |
|
(I will agree that splitting this work into smaller PRs has helped. Nice work!) |
|
@MrAlias my fault, I thought that will not cause conflicts :(. Can you please rebase? |
| // The set of labels associated with the metric descriptor. Labels in this list apply to | ||
| // all data points. | ||
| repeated opentelemetry.proto.common.v1.StringKeyValue labels = 6; |
There was a problem hiding this comment.
This was removed in #144. Please revert this change.
|
@MrAlias I'm using this proto to create an OTLP metric exporter in JS. However, I'm confused about how metric kinds map to temporality. I was looking at the specs, and it didn't mention temporality. Is this correct? Counter, SumObserver: Cumulative |
|
It's not so straightforward, unfortunately. The temporality depends on the aggregation. For ValueRecorder, ValueObserver: A LastValue or Exact aggregation will have Instaneaneous temporality, whereas a MinMaxLastSumCount, Summary, or Histogram aggregation will have Delta. For the other instruments, you have it right: Please keep an eye on #162 Also see how in open-telemetry/opentelemetry-go#840 an SDK is configured to support converting between cumulative and delta exporters, but when working with OTLP the client should not have to do this. That's why SumObserver is cumulative and Counter is delta in OTLP, it requires no memory in the client--OTLP therefore is considered a PassThrough kind of exporter. |
|
@jmacd Thanks so much! This was such a big help! I just subscribed to that PR. |
Updates the
MetricDescriptorto specify the temporal quality of the data being transported. Consequentially,typeis now decoupled from this introduced concept.This is a partial replacement of #137
Relates to #125