Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rationalize naming of metric instruments and their default aggregations #96

Closed
wants to merge 10 commits into from

Conversation

jmacd
Copy link
Contributor

@jmacd jmacd commented Apr 10, 2020

Based on recent feedback for #93 and lingering questions from #88, I've proposed a new naming scheme with more consistency. This also hopes to address an open question about aggregating last-values.

| | UpDownCounter | **UpDownCounter** | Sync | Add() | Sum | Yes |
| Measure | Distribution | **Recorder** | Sync | Record() | MinMaxSumCount | No |
| | Timing | **TimingRecorder** | Sync | Record() | MinMaxSumCount | No |
| Observer | LastValueObserver | **GaugeObserver** | Async | Observe() | MinMaxSumCount | No |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really want this to be Sum because of memory-usage metric. Look at the Go example for process metrics open-telemetry/opentelemetry-specification#549 (comment) I don't want to see "process/sys_heap" as a minmaxsumcount by default.

Another argument is default cost (money), if I am using a system like Stackdriver exporting "minmaxsumcount" will cost 4 times more than just sum. So I think it is important that default aggregations are less "expensive".

PS: I know I work for a company that makes money if the user sends more data :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Firstly, I think minmaxsumcount is perfectly sensible for a heap. If you aggregate this across a cluster, you'd like to know what the min/max values are. If you don't aggregate the count as well, the metric system will requires domain-level knowledge to construct an average using some other knowledge (i.e.,, that there's a corresponding count available, implicitly).

Moreover, I've documented (here) how there's an obvious optimization that results in sending just one value for the min/max/sum/count when there is just one value, which reduces the data-size problem for the case you care about. Each machine will report one min/max/sum/count as a single value in the ordinary case, but when we aggregate those values, we'll get the proper min/max/sum and count out--no domain-knowledge required.

Lastly, the metric instrument you're describing is exactly the hypothetical UpDownCumulativeObserver detailed in OTEP 88. If you want a Sum-only aggregation, then you should use a Sum-only instrument. That instrument does exactly what you want, should we standardize it?

text/0096-metric-instrument-terminology.md Outdated Show resolved Hide resolved
text/0096-metric-instrument-terminology.md Show resolved Hide resolved
@arminru arminru added the metrics Relates to the Metrics API/SDK label Apr 14, 2020
as the latter is dependent on SDK configuration. A "Recorder" records
a value that is part of a distribution. A "Counter" counts a value
that is part of a sum. An "GaugeObserver" observes an instantaneous
value ("reads a gauge"). A "Recorder" records an arbitrary value. A
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is confusing that "Recorder" is included twice in this list. Once saying it records a distribution and the other saying it records an arbitrary value. I think the latter is more accurate. If a view of a Recorder were to 'aggregate' with an array (i.e. no aggregation) saying a "Recorder" records a value that is part of a distribution, while not incorrect, isn't precise.

Clearly, when Count equals 1, the Min, Max, and Sum are equal to the
value. Exporters may be able take advantage of this fact when
exporting data from these instruments. In particular, since it is
known that asynchronous instruments produce only one valiue per
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
known that asynchronous instruments produce only one valiue per
known that asynchronous instruments produce only one value per

Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good. +1 on the change from Measure -> Recorder.

Only small comment on duplicate listing of Recorder.

@jmacd
Copy link
Contributor Author

jmacd commented Apr 18, 2020

This will be replaced by #98.

@jmacd jmacd closed this Apr 18, 2020
@jmacd jmacd deleted the jmacd/abstract_terms branch May 6, 2020 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
metrics Relates to the Metrics API/SDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants