Replace Temporality with Kind; Type with ValueType #168

jmacd · 2020-07-15T09:02:16Z

This is a partial re-do of #162 with only the Kind and ValueType changes and more extensive comments.

Type renamed ValueType
Temporality values (CUMULATIVE, DELTA, INSTANTANEOUS) move into Kind
Structure values (ADDING_MONOTONIC, ADDING, GROUPING) included in Kind
Continuity values (SNAPSHOT, CONTINUOUS) included in Kind
Monotonic becomes a Structure

Part of #125
Resolves #124
Resolves #78

opentelemetry/proto/metrics/v1/gen.go

jmacd · 2020-07-15T18:11:15Z

FYI in 162 I measured the cost of a oneof -ike arrangement and we continue to rule out this approach.

bogdandrutu

LGTM (minor nits) except the async bit which unfortunately I fail to understand the implication on the consumer.

opentelemetry/proto/metrics/v1/gen.go

opentelemetry/proto/metrics/v1/metrics.proto

bogdandrutu · 2020-07-15T19:09:08Z

@jmacd you had a go mod/sum in the original PR consider to send a quick PR to add them to the repo (independent of any proto changes)

opentelemetry/proto/metrics/v1/metrics.proto

jmacd · 2020-07-20T08:10:47Z

@lubingfeng

I sure hope we have resolved the issues and can get these changes merged in 2 weeks. @open-telemetry/specs-metrics-approvers Let's Really Do This.

lubingfeng · 2020-07-20T12:19:43Z

I sure hope we have resolved the issues and can get these changes merged in 2 weeks. @open-telemetry/specs-metrics-approvers Let's Really Do This.

This is great. I plan to attend this Thursday's SIG meeting for metrics and would be good to hear latest progress / status on those four pull requests:
#168
#170
#159
#171

cnnradams · 2020-07-20T17:17:11Z

opentelemetry/proto/metrics/v1/metrics.proto

-    SUMMARY = 6;
+    // Summary implies that the value is found in
+    // Metric.summary_data_points[].summary.
+    SUMMARY = 4;


Thinking ahead to the exemplars changes; in #162 we had SUMMARY_INT64 and SUMMARY_DOUBLE which lets us determine the data type of exemplars (int or double). But if we only specify double types for summary, does it make sense to have a SUMMARY_INT64 and SUMMARY_DOUBLE type for only exemplars or should there be a different enum for raw measurement type? Same thing for histogram

I was on the fence about creating _INT64 and _DOUBLE variations of Histogram and Summary data points. I wouldn't object, but I fear this is more detail than most people want. I draw a distinction between single-value data points (SCALAR, RAW) and multi-value data points (HISTOGRAM, SUMMARY). I think it's more OK to support only floats for multi-value data points, basically, and really not OK to do that for scalars and raw values.

I asked around whether this was a real problem at Google, and in cases it was. If you count your network traffic in bits per second, your router is likely to overflow a 2^50 counter but not a 2^64 counter.

opentelemetry/proto/metrics/v1/metrics.proto

bogdandrutu · 2020-07-20T20:35:56Z

opentelemetry/proto/metrics/v1/metrics.proto

+  //
+  // 1. CUMULATIVE GROUPING kinds typically use Histogram data points
+  // 2. CUMULATIVE is rarely used with Summary data points
+  enum Kind {


Spent time during the weekend trying this new proposal, and found a lot of things that don't make a lot of sense.

See more details here and a proposed different approach of thinking about the data model:
https://docs.google.com/document/d/1UBpke_e_HWl7UV3piNIMAZYw8eNjfGjucdwwsEOqUI4/edit#

Please don't hesitate to comment in the doc or here. Will try to answer all the comments :)

bogdandrutu

Some more comments, I read this proposal now at least 5 times and I still don't understand the subtle things and edge cases which makes me feel that this is too complicated and will not be able to use it as a generic protocol.

bogdandrutu · 2020-07-21T19:55:13Z

opentelemetry/proto/metrics/v1/metrics.proto

+  // may be interpreted differently depending on Structure.  For
+  // example, a Histogram DataPoint may be computed for a Counter
+  // (ADDING) instrument or a ValueRecorder (GROUPING) instrument, and
+  // the Sum and Count of the resulting aggregation may be interpreted


I don't see how Count relates to "Structure", I see "Count" being related with Temporality not structure.

bogdandrutu · 2020-07-21T20:00:59Z

opentelemetry/proto/metrics/v1/metrics.proto

+  // by equal Resource and TimeUnixNano values.  SNAPSHOT metrics
+  // MUST NOT have more than one data point per TimeUnixNano,
+  // Resource, and LabelSet.  If the interpreter of this data finds


SNAPSHOT metrics MUST NOT have more than one data point per TimeUnixNano, Resource, and LabelSet.

This is true for every metric correct?

… the aggregation and temporality. This PR takes a more descriptive approach where the Aggregation (the last applied aggregation if any) is clearly defined and Temporality is available only where it makes sense. This can help clearly identify what are the possible values and properties for different types of Aggregations. There are some things that can be considered (TODOs left in the code): * open-telemetry/opentelemetry-specification#725 * Histogram/Summary sum - monotonicity? * Previous aggregation measurements: raw measurements or "derived" measurements (results of another aggregation). * Support for RawMeasurements (recorded via the Sync Instruments) IMPORTANT: * This PR is not equivalent with open-telemetry#168 or open-telemetry#181, this is inspired by these PRs but makes some changes that are not compatible with that PR. * This PR is an incremental update, without any significant semantic changes (except adding the notion of GAUGE), from the current state, more changes will be added in the future to resolve the TODOs. Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>

… the aggregation and temporality. This PR takes a more descriptive approach where the Aggregation (the last applied aggregation if any) is clearly defined and Temporality is available only where it makes sense. This can help clearly identify what are the possible values and properties for different types of Aggregations. There are some things that can be considered (TODOs left in the code): * open-telemetry/opentelemetry-specification#725 * Histogram/Summary sum - monotonicity? * Previous aggregation measurements: raw measurements or "derived" measurements (results of another aggregation). * Support for RawMeasurements (recorded via the Sync Instruments) * Decide if support for INSTANTANEOUS temporality is still needed. IMPORTANT: * This PR is not equivalent with open-telemetry#168 or open-telemetry#181, this is inspired by these PRs but makes some changes that are not compatible with that PR. * This PR is an incremental update, without any significant semantic changes (except adding the notion of GAUGE), from the current state, more changes will be added in the future to resolve the TODOs. Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>

jmacd · 2020-07-29T20:11:46Z

See #182.

…tails about the aggregation and temporality. (#182) * Change Metric type to be a more detailed structure with details about the aggregation and temporality. This PR takes a more descriptive approach where the Aggregation (the last applied aggregation if any) is clearly defined and Temporality is available only where it makes sense. This can help clearly identify what are the possible values and properties for different types of Aggregations. There are some things that can be considered (TODOs left in the code): * open-telemetry/opentelemetry-specification#725 * Histogram/Summary sum - monotonicity? * Previous aggregation measurements: raw measurements or "derived" measurements (results of another aggregation). * Support for RawMeasurements (recorded via the Sync Instruments) * Decide if support for INSTANTANEOUS temporality is still needed. IMPORTANT: * This PR is not equivalent with #168 or #181, this is inspired by these PRs but makes some changes that are not compatible with that PR. * This PR is an incremental update, without any significant semantic changes (except adding the notion of GAUGE), from the current state, more changes will be added in the future to resolve the TODOs. Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com> * Address feedback, added more TODOs and created issues Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com> * Update opentelemetry/proto/metrics/v1/metrics.proto Co-authored-by: Aaron Abbott <aaronabbott@google.com> * Update opentelemetry/proto/metrics/v1/metrics.proto Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com> * Update opentelemetry/proto/metrics/v1/metrics.proto Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com> * Update opentelemetry/proto/metrics/v1/metrics.proto Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com> * Update opentelemetry/proto/metrics/v1/metrics.proto Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com> * Update opentelemetry/proto/metrics/v1/metrics.proto Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com> * Fix indentation and comment for Type Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com> Co-authored-by: Aaron Abbott <aaronabbott@google.com> Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>

lubingfeng · 2020-08-05T15:48:40Z

I sure hope we have resolved the issues and can get these changes merged in 2 weeks. @open-telemetry/specs-metrics-approvers Let's Really Do This.

This is great. I plan to attend this Thursday's SIG meeting for metrics and would be good to hear latest progress / status on those four pull requests:
#168
#170
#159
#171

I see the closure of #168, #170. #159 is still in draft stage while #171 is merged. Are we close to get #159 and #171 closed this week or next week?

jmacd added 19 commits June 30, 2020 21:46

Rough draft pre-comments

7dc68e8

Comments and restoring order

8838618

Remove gen.go

907f253

metric_descriptor->descriptor

01cdd17

whitespace

538e599

Define structure

5700d38

Comments after @bogdandrutu and @MrAlias feedback

58ecfab

Revert data point changes, keep Kind & ValueType changes

67f20c4

Comment gen.go

e577247

Checkpoint

50e0331

Document each kind

0c3a7f6

Fix numbering

c5f291a

One more open-telemetry#159 TODO

39e7967

One more open-telemetry#159 TODO

5e7fa7f

Formatting

88acc0c

Undo generated files

92ddee2

Upstream

82322a2

Remove go.mod

e89503f

Comments

d870316

jmacd requested a review from a team July 15, 2020 09:02

jmacd added the spec:metrics label Jul 15, 2020

MrAlias reviewed Jul 15, 2020

View reviewed changes

opentelemetry/proto/metrics/v1/gen.go Outdated Show resolved Hide resolved

opentelemetry/proto/metrics/v1/gen.go Outdated Show resolved Hide resolved

More comments on Kind

4a823d7

Format me

306d559

bogdandrutu reviewed Jul 15, 2020

View reviewed changes

tsloughter reviewed Jul 15, 2020

View reviewed changes

opentelemetry/proto/metrics/v1/metrics.proto Outdated Show resolved Hide resolved

tsloughter reviewed Jul 15, 2020

View reviewed changes

opentelemetry/proto/metrics/v1/metrics.proto Outdated Show resolved Hide resolved

Comments

18271f2

jmacd mentioned this pull request Jul 20, 2020

Merge Int64DataPoint and DoubleDataPoint into ScalarDataPoint #172

Closed

cnnradams reviewed Jul 20, 2020

View reviewed changes

bogdandrutu reviewed Jul 20, 2020

View reviewed changes

opentelemetry/proto/metrics/v1/metrics.proto Outdated Show resolved Hide resolved

Upstream

ced5068

bogdandrutu reviewed Jul 20, 2020

View reviewed changes

jmacd mentioned this pull request Jul 21, 2020

Metrics terminology: Grouping instruments as (opposed to Adding instruments) open-telemetry/opentelemetry-specification#625

Closed

bogdandrutu reviewed Jul 21, 2020

View reviewed changes

This was referenced Jul 21, 2020

Update metrics top comments and structure #178

Merged

Change Metric type to be a more detailed structure with details about the aggregation and temporality. #179

Closed

jmacd added 2 commits July 22, 2020 22:21

Top of KindElement

fbcf615

Rewrite KindElementdocs

376d80e

bogdandrutu closed this Jul 23, 2020

bogdandrutu reopened this Jul 23, 2020

jmacd added 2 commits July 23, 2020 09:56

Improve comments

bb9689f

Comment all Kind values

15c7f92

bogdandrutu requested a review from MrAlias July 23, 2020 23:27

Merge 178

ab75b67

This was referenced Jul 24, 2020

Changing OTLP exporters to export cumulative metrics by default open-telemetry/opentelemetry-specification#731

Closed

Add Structure and Continuity; remove instantaneous Temporality #181

Closed

bogdandrutu mentioned this pull request Jul 29, 2020

Add Gauge. Change Metric type to be a more detailed structure with details about the aggregation and temporality. #182

Merged

jmacd closed this Jul 29, 2020

jmacd mentioned this pull request Jul 30, 2020

Is performance important enough for the MetricDescriptor Type to encode it as a unit64? #189

Closed

huyan0 mentioned this pull request Aug 30, 2021

Proposal: Metric Aggregation Processor open-telemetry/opentelemetry-collector-contrib#4968

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace Temporality with Kind; Type with ValueType #168

Replace Temporality with Kind; Type with ValueType #168

jmacd commented Jul 15, 2020 •

edited

Loading

jmacd commented Jul 15, 2020

bogdandrutu left a comment

bogdandrutu commented Jul 15, 2020

jmacd commented Jul 20, 2020 •

edited

Loading

lubingfeng commented Jul 20, 2020 •

edited

Loading

cnnradams Jul 20, 2020

jmacd Jul 23, 2020

bogdandrutu Jul 20, 2020

bogdandrutu Jul 20, 2020

bogdandrutu left a comment •

edited

Loading

bogdandrutu Jul 21, 2020

bogdandrutu Jul 21, 2020

jmacd commented Jul 29, 2020

lubingfeng commented Aug 5, 2020

Replace Temporality with Kind; Type with ValueType #168

Replace Temporality with Kind; Type with ValueType #168

Conversation

jmacd commented Jul 15, 2020 • edited Loading

jmacd commented Jul 15, 2020

bogdandrutu left a comment

Choose a reason for hiding this comment

bogdandrutu commented Jul 15, 2020

jmacd commented Jul 20, 2020 • edited Loading

lubingfeng commented Jul 20, 2020 • edited Loading

cnnradams Jul 20, 2020

Choose a reason for hiding this comment

jmacd Jul 23, 2020

Choose a reason for hiding this comment

bogdandrutu Jul 20, 2020

Choose a reason for hiding this comment

bogdandrutu Jul 20, 2020

Choose a reason for hiding this comment

bogdandrutu left a comment • edited Loading

Choose a reason for hiding this comment

bogdandrutu Jul 21, 2020

Choose a reason for hiding this comment

bogdandrutu Jul 21, 2020

Choose a reason for hiding this comment

jmacd commented Jul 29, 2020

lubingfeng commented Aug 5, 2020

jmacd commented Jul 15, 2020 •

edited

Loading

jmacd commented Jul 20, 2020 •

edited

Loading

lubingfeng commented Jul 20, 2020 •

edited

Loading

bogdandrutu left a comment •

edited

Loading