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

Make it clear that Exemplar is opt-in #2105

Closed

Conversation

cijothomas
Copy link
Member

Its not clear if the Exemplar feature is enabled by default or disabled by default. Trying to make it obvious.
I assume this is an opt-in feature. If that's the not the case, happy to reword it accordingly.

Only goal of this PR is to make it very clear whether its enabled/disabled by default.

Also removed a statement about "allocations" as thats implementation details, and not required in spec.

@cijothomas
Copy link
Member Author

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#exemplar-defaults This section feels like the intent was to have this as the default behavior. Hoping to get clarification here.

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

lgtm

specification/metrics/sdk.md Outdated Show resolved Hide resolved
@@ -478,8 +478,6 @@ minimum, joining together attributes on an `Exemplar` with those available
on its associated metric data point should result in the full set of attributes
from the original sample measurement.

The `ExemplarReservoir` SHOULD avoid allocations when sampling exemplars.
Copy link
Member

Choose a reason for hiding this comment

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

is deleting this line relevant to PR's intent?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was unrelated, yes. I mentioned it in the description.

Copy link
Contributor

Choose a reason for hiding this comment

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

This was here intentionally. Allocations can really hurt performance and it's an optional reequirement.

Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
@@ -478,8 +478,6 @@ minimum, joining together attributes on an `Exemplar` with those available
on its associated metric data point should result in the full set of attributes
from the original sample measurement.

The `ExemplarReservoir` SHOULD avoid allocations when sampling exemplars.
Copy link
Contributor

Choose a reason for hiding this comment

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

This was here intentionally. Allocations can really hurt performance and it's an optional reequirement.

@@ -420,7 +420,7 @@ information:

A Metric SDK MUST provide a mechanism to sample `Exemplar`s from measurements.

A Metric SDK MUST allow `Exemplar` sampling to be disabled. In this instance the SDK SHOULD not have overhead related to exemplar sampling.
`Exemplar` sampling MUST be an opt-in feature. When not enabled, the SDK SHOULD NOT have overhead related to exemplar sampling.
Copy link
Contributor

Choose a reason for hiding this comment

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

Exemplar sampling is meant to be default-on, but only with sampled traces. Default is documented here.

This means you only get exemplar sampling if you also have trace instrumentation.

@jsuereth
Copy link
Contributor

jsuereth commented Nov 5, 2021

This was not the intention of the specification.

@@ -420,7 +420,7 @@ information:

A Metric SDK MUST provide a mechanism to sample `Exemplar`s from measurements.

A Metric SDK MUST allow `Exemplar` sampling to be disabled. In this instance the SDK SHOULD not have overhead related to exemplar sampling.
`Exemplar` sampling MUST be an opt-in feature. When not enabled, the SDK SHOULD NOT have overhead related to exemplar sampling.

A Metric SDK MUST sample `Exemplar`s only from measurements within the context of a sampled trace BY DEFAULT.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the line that contradicts the PR. By default exemplar sampling should following the tracing configuration.

@reyang reyang added area:sdk Related to the SDK spec:metrics Related to the specification/metrics directory labels Nov 5, 2021
@reyang reyang added this to In progress in Spec - Metrics API/SDK Stable Release via automation Nov 5, 2021
@jonatan-ivanov
Copy link
Member

Fyi: Prometheus' Exemplars support is opt-in too.
Do you happen to know about any other stable Metrics backend that supports Exemplars?

I have a slightly connected question: should OTel Metrics support any Exemplar source or only OTel Tracing?

If so, I would introduce an ExemplarProvider/ExemplarSupplier component that returns an Exemplar from any arbitrary source the user wants to interact with. OTel would provide a default implementation for OTel Tracing but users could implement and configure their own provider.

Also, should the ExemplarFilter know about the name of the Meter/Instrument so that users can enable/disable Exemplars for certain Meters/Instruments?

@cijothomas
Copy link
Member Author

The original intention of this PR was to make the wording clear that Exemplar is an opt-in feature. But @jsuereth has clarified that the intention was to have it on-by-default. Closing this PR as no longer relevant. Will open a separate issue to ask opinion on making this opt-in/other qns.

@cijothomas
Copy link
Member Author

Fyi: Prometheus' Exemplars support is opt-in too. Do you happen to know about any other stable Metrics backend that supports Exemplars?

I have a slightly connected question: should OTel Metrics support any Exemplar source or only OTel Tracing?

If so, I would introduce an ExemplarProvider/ExemplarSupplier component that returns an Exemplar from any arbitrary source the user wants to interact with. OTel would provide a default implementation for OTel Tracing but users could implement and configure their own provider.

Also, should the ExemplarFilter know about the name of the Meter/Instrument so that users can enable/disable Exemplars for certain Meters/Instruments?

As this PR is closed, could you open this in a separate issue, to ensure its not lost. Thanks for reviewing!

@reyang reyang moved this from In progress to Done in Spec - Metrics API/SDK Stable Release Nov 15, 2021
@jsuereth
Copy link
Contributor

@jonatan-ivanov @cijothomas I think there's an important distinction here that's possibly missing.

By Default, Exemplar sampling is on only if there exists a Sampled Trace. The default requires:

  • Users have configured tracing
  • Tracing is in context
  • Tracing is sampling

Exemplar sampling, by default, is controlled via Trace sampling. The ExemplarFilter/ExemplarResorvoir hooks are only for advanced Exemplar usage.

This is unlike other systems where Exemplar sampling may or may not correlate with the existence of Traces. I'd expect a user of just the otel metrics SDK to never see an exemplar (by default). It's only once they've also instrumented traces that they see exemplars be generated.

I have a slightly connected question: should OTel Metrics support any Exemplar source or only OTel Tracing?

An Exemplar != a trace. The exemplar is the measurement, we just attach the trace_id to it. OTel is the only library (I know of) where attaching traces is first-class, e.g. in prometheus exemplars just attach labels to measurements.

That said, to the extent there's another system instrumenting traces, I'd hope they expose OTEL-friendly hooks for interacting with trace. I think it's out-of-scope for OTEL (which already provides pure-apis for tracing) to support backends which don't leverage those APIs. Instead we should just provide a bridge (similar to what we've done for OpenTracing + OpenCensus).

@jonatan-ivanov
Copy link
Member

I understand the behavior of Exemplar sampling (only sample if there is a sampled span), and I think everything should behave like that but what I'm saying is if the user configured OTel tracing and they have a sampled span, sampling an Exemplar can be useless if the backend does not support them and as far as I know only Prometheus supports Exemplars and this feature is off by default.

I did not say Exemplar == a trace but I think this answers my question:

I'd hope they expose OTEL-friendly hooks for interacting with trace

If this means that another tracing libraries can implement a public interface that has a goal of providing the Exemplar data and OTel Metrics would call this during sampling an Exemplar, I think we should be able to plug-in any tracer implementation to provide the data for Exemplars.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:sdk Related to the SDK spec:metrics Related to the specification/metrics directory
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants