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

Expand event-model description to more clearly delineate instrument v… #1614

Merged
merged 12 commits into from
Apr 22, 2021

Conversation

jsuereth
Copy link
Contributor

…s. metric stream and some reasoning behind it.

Fixes ##1366

Changes

Improve the documentation around the "Event Model" and the relationship of "Instruments" to "metrics"

…s. metric stream and some reasoning behind it.
specification/metrics/datamodel.md Outdated Show resolved Hide resolved
specification/metrics/datamodel.md Outdated Show resolved Hide resolved
specification/metrics/datamodel.md Outdated Show resolved Hide resolved
specification/metrics/datamodel.md Outdated Show resolved Hide resolved
specification/metrics/datamodel.md Outdated Show resolved Hide resolved
specification/metrics/datamodel.md Outdated Show resolved Hide resolved
- Adding vs. Grouping aggregation.
- Adding instruments express a sum. All points recorded via this instrument
are parts of a whole.
- Grouping instruments characterize a group of measurements. All points
Copy link
Member

Choose a reason for hiding this comment

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

Where does this leave things like quantiles?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Quantile is a form of grouping.

Copy link
Contributor

Choose a reason for hiding this comment

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

I still believe this is a useful concept, but I am worried we've lost the connection to the API design by now. There's something about how Gauge and Histogram instruments are the same semantics with different default aggregations: both group individual measurements.

The point is that Gauge and Histogram inputs are semantically different than Sum inputs, because of individuality. This is meant to help the user choose between Counter and Histogram, for example. @reyang I mean to follow up on last week's API/SDK SIG meeting, in which we discussed some of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The goal of this is NOT to define what instruments the API uses, but show the flexibility of the model to adapt to differing instruments.

I'm thinking about doing the following:

  • Focus on adapting instruments (OTel + non-otel) into the data model
  • Focus on general concepts that may be used (quantiles, grouping, etc.)
  • Direct to the API specification for specific instruments / instrument names.

This specification should be about WHAT metrics mean, not what instruments are available in the API.

specification/metrics/datamodel.md Outdated Show resolved Hide resolved
other system. OpenTelemetry metrics are designed such that the same instrument
and events can be used in different ways to generate metric streams.

![Events → Streams](img/model-event-layer.png)
Copy link
Member

Choose a reason for hiding this comment

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

does total_latency aggregation ever makes sense? It may not be the best idea to include example like this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do want an example that shows how a single instrument could turn into all of Sum, Histogram + Gauge.

While I can synthesize hair-brained scenarios where I think "total latency" might make sense (e.g. ridiculous statistics like How many days have users waited for our website in aggregate), You're right it's not a super useful example. Going to take a day to brainstorm and looking for other ideas on this example.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the example was a count (e.g., request_size), then max, sum, and histogram outputs all make sense.

However, one drawback with this example (sum alongside a histogram) is that a histogram data point already contains a sum, so exposing a separate metric with the sum is somehow not useful. The max function makes a better example, you could export the maximum over [1m], [10m], [1hr], and so on. (Related: open-telemetry/opentelemetry-proto#279)

The example export one histogram by metric_by_a_and_b{attributeA,attributeB}, one one histogram by metric_by_a_and_c{attributeA,attributeC}, maybe? I would emphasize that when doing this kind of output, separate metric names MUST be used to avoid metric data being recombined with itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated to use request size. I agree that histogram already has sum. I've also added some caveats to the image to denote it's meant to show the power, not a practical sceanrio.

PTAL at the new verbage. Also, I don't want to throw the baby out with the bathwater here. Look at the whole context of the doc as an introduction to the space of metrics. We need to both:

  • Give people a mental model of why/how Instruments differ from Metric.
  • Give people a grounding in what mapping from Event => Metric Stream looks like and the decisions that need to be made.

We do not need to specify the Otel API or its behavior here. We only need to specify what concepts are allowed in OTel metric streams. Lots of these nuances and details belong in the API docs.

I see these docs server two users primarily:

  • API authors looking to generate OTLP by mapping their Events/Instruments into streams.
  • Exporter authors looking to consume OTLP by mapping streams into their backend timeseries.

Instrumentation users who are generating metrics should be using the API specification.

Copy link
Member

@SergeyKanzhelev SergeyKanzhelev left a comment

Choose a reason for hiding this comment

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

lgtm except the example on the picture. I suggest to fix it before merging

other system. OpenTelemetry metrics are designed such that the same instrument
and events can be used in different ways to generate metric streams.

![Events → Streams](img/model-event-layer.png)
Copy link
Contributor

Choose a reason for hiding this comment

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

If the example was a count (e.g., request_size), then max, sum, and histogram outputs all make sense.

However, one drawback with this example (sum alongside a histogram) is that a histogram data point already contains a sum, so exposing a separate metric with the sum is somehow not useful. The max function makes a better example, you could export the maximum over [1m], [10m], [1hr], and so on. (Related: open-telemetry/opentelemetry-proto#279)

The example export one histogram by metric_by_a_and_b{attributeA,attributeB}, one one histogram by metric_by_a_and_c{attributeA,attributeC}, maybe? I would emphasize that when doing this kind of output, separate metric names MUST be used to avoid metric data being recombined with itself.

specification/metrics/datamodel.md Outdated Show resolved Hide resolved
specification/metrics/datamodel.md Outdated Show resolved Hide resolved
specification/metrics/datamodel.md Outdated Show resolved Hide resolved
- Adding vs. Grouping aggregation.
- Adding instruments express a sum. All points recorded via this instrument
are parts of a whole.
- Grouping instruments characterize a group of measurements. All points
Copy link
Contributor

Choose a reason for hiding this comment

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

I still believe this is a useful concept, but I am worried we've lost the connection to the API design by now. There's something about how Gauge and Histogram instruments are the same semantics with different default aggregations: both group individual measurements.

The point is that Gauge and Histogram inputs are semantically different than Sum inputs, because of individuality. This is meant to help the user choose between Counter and Histogram, for example. @reyang I mean to follow up on last week's API/SDK SIG meeting, in which we discussed some of this.

specification/metrics/datamodel.md Outdated Show resolved Hide resolved
Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM.

@SergeyKanzhelev
Copy link
Member

@jmacd do you want more changes in PR or merge as-is for now?

@jmacd jmacd merged commit d0d2b48 into open-telemetry:main Apr 22, 2021
@jsuereth jsuereth deleted the wip-instruments branch April 22, 2021 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants