-
Notifications
You must be signed in to change notification settings - Fork 782
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
[sdk-metrics] ExemplarReservoir dedicated diagnostic and custom ExemplarReservoir support #5558
Changes from 5 commits
a5105e8
9972819
2ef5120
02b3751
b7182b4
b317f2b
36b0a45
f73bad5
8eff6b2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
# OpenTelemetry .NET Diagnostic: OTEL1004 | ||
|
||
## Overview | ||
|
||
This is an Experimental API diagnostic covering the following APIs: | ||
|
||
* `ExemplarReservoir` | ||
* `FixedSizeExemplarReservoir` | ||
* `ExemplarMeasurement<T>` | ||
* `MetricStreamConfiguration.ExemplarReservoirFactory.get` | ||
* `MetricStreamConfiguration.ExemplarReservoirFactory.set` | ||
|
||
Experimental APIs may be changed or removed in the future. | ||
|
||
## Details | ||
|
||
The OpenTelemetry Specification defines an [ExemplarReservoir | ||
API](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#exemplarreservoir) | ||
and a mechanism for configuring `ExemplarReservoir` via the [View | ||
API](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#stream-configuration) | ||
in the Metrics SDK. | ||
|
||
From the specification: | ||
|
||
> The SDK MUST provide a mechanism for SDK users to provide their own | ||
> ExemplarReservoir implementation. This extension MUST be configurable on a | ||
> metric View, although individual reservoirs MUST still be instantiated per | ||
> metric-timeseries... | ||
|
||
We are exposing these APIs experimentally until the specification declares them | ||
stable and the maintainers are comfortable with the usability of the design and | ||
the need for the feature. | ||
|
||
<!-- | ||
## Provide feedback | ||
|
||
Please provide feedback on [this issue](TODO) if you need stable support for | ||
custom `ExemplarReservoir`s. | ||
--> |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,10 +12,10 @@ namespace OpenTelemetry.Metrics; | |
/// <summary> | ||
/// Represents an Exemplar measurement. | ||
/// </summary> | ||
/// <remarks><inheritdoc cref="Exemplar" path="/remarks/para[@experimental-warning='true']"/></remarks> | ||
/// <remarks><inheritdoc cref="ExemplarReservoir" path="/remarks/para[@experimental-warning='true']"/></remarks> | ||
/// <typeparam name="T">Measurement type.</typeparam> | ||
#if NET8_0_OR_GREATER | ||
[Experimental(DiagnosticDefinitions.ExemplarExperimentalApi, UrlFormat = DiagnosticDefinitions.ExperimentalApiUrlFormat)] | ||
[Experimental(DiagnosticDefinitions.ExemplarReservoirExperimentalApi, UrlFormat = DiagnosticDefinitions.ExperimentalApiUrlFormat)] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we not need this exposed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The spec design is you give the This Should it be exposed? It isn't part of the spec so I don't know if we can/should expose it. Chatting with @alanwest, it does seem like a useful thing to have available to custom reservoirs. The spec could have "MAY" language about exposing it. But it isn't a blocker for the current plan for the first stable release. In general, we (me, @alanwest, @cijothomas) have a lot of questions/concerns about the reservoir design which is why I think it is a good idea to keep it in preview initially. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good to me 👍 . I asked because of this reason only - |
||
#endif | ||
public | ||
#else | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nits: Spec is already stable, so we can remove that part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cijothomas I removed the stable part and added a lot more detail. Please re-review and LMK what you think.