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

[sdk-metrics] ExemplarReservoir dedicated diagnostic and custom ExemplarReservoir support #5558

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions OpenTelemetry.sln
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,7 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "experimental-apis", "experi
docs\diagnostics\experimental-apis\OTEL1001.md = docs\diagnostics\experimental-apis\OTEL1001.md
docs\diagnostics\experimental-apis\OTEL1002.md = docs\diagnostics\experimental-apis\OTEL1002.md
docs\diagnostics\experimental-apis\OTEL1003.md = docs\diagnostics\experimental-apis\OTEL1003.md
docs\diagnostics\experimental-apis\OTEL1004.md = docs\diagnostics\experimental-apis\OTEL1004.md
docs\diagnostics\experimental-apis\README.md = docs\diagnostics\experimental-apis\README.md
EndProjectSection
EndProject
Expand Down
2 changes: 1 addition & 1 deletion build/Common.props
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
<Nullable>enable</Nullable>
<ImplicitUsings>enable</ImplicitUsings>
<!-- Suppress warnings for repo code using experimental features -->
<NoWarn>$(NoWarn);OTEL1000;OTEL1001;OTEL1002;OTEL1003</NoWarn>
<NoWarn>$(NoWarn);OTEL1000;OTEL1001;OTEL1002;OTEL1003;OTEL1004</NoWarn>
<!--temporarily disable. See 3958-->
<!--<AnalysisLevel>latest-All</AnalysisLevel>-->
</PropertyGroup>
Expand Down
8 changes: 4 additions & 4 deletions docs/diagnostics/experimental-apis/OTEL1002.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,12 @@

This is an Experimental API diagnostic covering the following APIs:

* `AlwaysOnExemplarFilter`
* `AlwaysOffExemplarFilter`
* `Exemplar`
* `ExemplarFilter`
* `ExemplarFilterType`
* `MeterProviderBuilder.SetExemplarFilter` extension method
* `TraceBasedExemplarFilter`
* `ReadOnlyExemplarCollection`
* `ReadOnlyFilteredTagCollection`
* `MetricPoint.TryGetExemplars`

Experimental APIs may be changed or removed in the future.

Expand Down
62 changes: 62 additions & 0 deletions docs/diagnostics/experimental-apis/OTEL1004.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
# 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 for the following reasons:

* `FixedSizeExemplarReservoir` is not part of the spec. It is meant to help
custom reservoir authors and takes care of correctly creating & updating
`struct Exemplar`s (managing tag filtering when views are used), handles
`Exemplar` collection, and ensures all operations are safe to be called
concurrency (spec requirement). We want to see if this is helpful and meets
the needs of users.

* There is currently no way to use
`MetricStreamConfiguration.ExemplarReservoirFactory` to switch a metric to a
different built-in reservoir (`AlignedHistogramBucketExemplarReservoir` or
`SimpleFixedSizeExemplarReservoir`). This is something supported by the spec
but we want to understand the use cases and needs before exposing these types.
Also it seems the default reservoirs may change.

* There is currently no way to get access to the bucket index inside a reservoir
when a measurement is recorded against a histogram with explicit bounds. The
spec says the reservoir should calculate this given the
definition/configuration of the bounds but the SDK has already done this
computation. It seems unncessarily complicated to expose the configuration and
wasteful to do the work twice. We want to understand the types of algorithms
which users will want to implement before exposing something.

**TL;DR** We want to gather feedback on the usability of the API and for the
need(s) in general for custom reservoirs before exposing a stable API.

<!--
## Provide feedback

Please provide feedback on [this issue](TODO) if you need stable support for
custom `ExemplarReservoir`s.
-->
6 changes: 6 additions & 0 deletions docs/diagnostics/experimental-apis/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,12 @@ Description: MetricStreamConfiguration CardinalityLimit Support

Details: [OTEL1003](./OTEL1003.md)

### OTEL1004

Description: ExemplarReservoir Support

Details: [OTEL1004](./OTEL1004.md)

## Inactive

Experimental APIs which have been released stable or removed:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,12 @@ OpenTelemetry.Metrics.ExemplarMeasurement<T>.ExemplarMeasurement() -> void
OpenTelemetry.Metrics.ExemplarMeasurement<T>.Tags.get -> System.ReadOnlySpan<System.Collections.Generic.KeyValuePair<string!, object?>>
OpenTelemetry.Metrics.ExemplarMeasurement<T>.Value.get -> T
OpenTelemetry.Metrics.ExemplarReservoir
OpenTelemetry.Metrics.ExemplarReservoir.ExemplarReservoir() -> void
OpenTelemetry.Metrics.ExemplarReservoir.ResetOnCollect.get -> bool
OpenTelemetry.Metrics.FixedSizeExemplarReservoir
OpenTelemetry.Metrics.FixedSizeExemplarReservoir.Capacity.get -> int
OpenTelemetry.Metrics.FixedSizeExemplarReservoir.FixedSizeExemplarReservoir(int capacity) -> void
OpenTelemetry.Metrics.FixedSizeExemplarReservoir.UpdateExemplar(int exemplarIndex, in OpenTelemetry.Metrics.ExemplarMeasurement<double> measurement) -> void
OpenTelemetry.Metrics.FixedSizeExemplarReservoir.UpdateExemplar(int exemplarIndex, in OpenTelemetry.Metrics.ExemplarMeasurement<long> measurement) -> void
OpenTelemetry.Metrics.MetricPoint.TryGetExemplars(out OpenTelemetry.Metrics.ReadOnlyExemplarCollection exemplars) -> bool
OpenTelemetry.Metrics.MetricStreamConfiguration.CardinalityLimit.get -> int?
OpenTelemetry.Metrics.MetricStreamConfiguration.CardinalityLimit.set -> void
Expand All @@ -46,6 +50,7 @@ OpenTelemetry.ReadOnlyFilteredTagCollection.Enumerator.Enumerator() -> void
OpenTelemetry.ReadOnlyFilteredTagCollection.Enumerator.MoveNext() -> bool
OpenTelemetry.ReadOnlyFilteredTagCollection.GetEnumerator() -> OpenTelemetry.ReadOnlyFilteredTagCollection.Enumerator
OpenTelemetry.ReadOnlyFilteredTagCollection.ReadOnlyFilteredTagCollection() -> void
override sealed OpenTelemetry.Metrics.FixedSizeExemplarReservoir.Collect() -> OpenTelemetry.Metrics.ReadOnlyExemplarCollection
static OpenTelemetry.Logs.LoggerProviderBuilderExtensions.AddProcessor(this OpenTelemetry.Logs.LoggerProviderBuilder! loggerProviderBuilder, OpenTelemetry.BaseProcessor<OpenTelemetry.Logs.LogRecord!>! processor) -> OpenTelemetry.Logs.LoggerProviderBuilder!
static OpenTelemetry.Logs.LoggerProviderBuilderExtensions.AddProcessor(this OpenTelemetry.Logs.LoggerProviderBuilder! loggerProviderBuilder, System.Func<System.IServiceProvider!, OpenTelemetry.BaseProcessor<OpenTelemetry.Logs.LogRecord!>!>! implementationFactory) -> OpenTelemetry.Logs.LoggerProviderBuilder!
static OpenTelemetry.Logs.LoggerProviderBuilderExtensions.AddProcessor<T>(this OpenTelemetry.Logs.LoggerProviderBuilder! loggerProviderBuilder) -> OpenTelemetry.Logs.LoggerProviderBuilder!
Expand All @@ -63,3 +68,4 @@ static OpenTelemetry.Sdk.CreateLoggerProviderBuilder() -> OpenTelemetry.Logs.Log
static Microsoft.Extensions.Logging.OpenTelemetryLoggingExtensions.UseOpenTelemetry(this Microsoft.Extensions.Logging.ILoggingBuilder! builder) -> Microsoft.Extensions.Logging.ILoggingBuilder!
static Microsoft.Extensions.Logging.OpenTelemetryLoggingExtensions.UseOpenTelemetry(this Microsoft.Extensions.Logging.ILoggingBuilder! builder, System.Action<OpenTelemetry.Logs.LoggerProviderBuilder!>! configure) -> Microsoft.Extensions.Logging.ILoggingBuilder!
static Microsoft.Extensions.Logging.OpenTelemetryLoggingExtensions.UseOpenTelemetry(this Microsoft.Extensions.Logging.ILoggingBuilder! builder, System.Action<OpenTelemetry.Logs.LoggerProviderBuilder!>? configureBuilder, System.Action<OpenTelemetry.Logs.OpenTelemetryLoggerOptions!>? configureOptions) -> Microsoft.Extensions.Logging.ILoggingBuilder!
virtual OpenTelemetry.Metrics.FixedSizeExemplarReservoir.OnCollected() -> void
6 changes: 6 additions & 0 deletions src/OpenTelemetry/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,12 @@
which could have led to a measurement being dropped.
([#5546](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5546))

* **Experimental (pre-release builds only):** Exposed
`FixedSizeExemplarReservoir` as a public API to support custom implementations
of `ExemplarReservoir` which may be configured using the
`ExemplarReservoirFactory` property on the View API.
([#5558](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5558))
cijothomas marked this conversation as resolved.
Show resolved Hide resolved

## 1.8.1

Released 2024-Apr-17
Expand Down
4 changes: 2 additions & 2 deletions src/OpenTelemetry/Metrics/Exemplar/ExemplarMeasurement.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Copy link
Member

Choose a reason for hiding this comment

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

Do we not need this exposed?

Copy link
Member Author

Choose a reason for hiding this comment

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

The spec design is you give the AlignedHistogramBucketExemplarReservoir the buckets/configuration for the histogram and then when an exemplar is offered you calculate the index and always keep the latest update:

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#alignedhistogrambucketexemplarreservoir

This internal int ExplicitBucketHistogramBucketIndex allows the SDK to pass it in so we don't have to do the calculation twice.

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.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to me 👍 . I asked because of this reason only - it does seem like a useful thing to have available to custom reservoirs. The AlignedHistogramBucketExemplarReservoir uses this so I thought someone writing the custom reservoir for histogram may also need this information.

#endif
public
#else
Expand Down
11 changes: 9 additions & 2 deletions src/OpenTelemetry/Metrics/Exemplar/ExemplarReservoir.cs
Original file line number Diff line number Diff line change
Expand Up @@ -13,19 +13,26 @@ namespace OpenTelemetry.Metrics;
/// ExemplarReservoir base implementation and contract.
/// </summary>
/// <remarks>
/// <inheritdoc cref="Exemplar" path="/remarks/para[@experimental-warning='true']"/>
/// <para experimental-warning="true"><b>WARNING</b>: This is an experimental API which might change or be removed in the future. Use at your own risk.</para>
/// Specification: <see
/// href="https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#exemplarreservoir"/>.
/// </remarks>
#if NET8_0_OR_GREATER
[Experimental(DiagnosticDefinitions.ExemplarExperimentalApi, UrlFormat = DiagnosticDefinitions.ExperimentalApiUrlFormat)]
[Experimental(DiagnosticDefinitions.ExemplarReservoirExperimentalApi, UrlFormat = DiagnosticDefinitions.ExperimentalApiUrlFormat)]
#endif
public
#else
internal
#endif
abstract class ExemplarReservoir
{
// Note: This constructor is internal because we don't allow custom
// ExemplarReservoir implementations to be based directly on the base class
// only FixedSizeExemplarReservoir.
internal ExemplarReservoir()
{
}

/// <summary>
/// Gets a value indicating whether or not the <see
/// cref="ExemplarReservoir"/> should reset its state when performing
Expand Down
Original file line number Diff line number Diff line change
@@ -1,25 +1,56 @@
// Copyright The OpenTelemetry Authors
// SPDX-License-Identifier: Apache-2.0

#if EXPOSE_EXPERIMENTAL_FEATURES && NET8_0_OR_GREATER
using System.Diagnostics.CodeAnalysis;
#endif
using OpenTelemetry.Internal;

namespace OpenTelemetry.Metrics;

internal abstract class FixedSizeExemplarReservoir : ExemplarReservoir
#if EXPOSE_EXPERIMENTAL_FEATURES
/// <summary>
/// An <see cref="ExemplarReservoir"/> implementation which contains a fixed
/// number of <see cref="Exemplar"/>s.
/// </summary>
/// <remarks><inheritdoc cref="ExemplarReservoir" path="/remarks/para[@experimental-warning='true']"/></remarks>
#if NET8_0_OR_GREATER
[Experimental(DiagnosticDefinitions.ExemplarReservoirExperimentalApi, UrlFormat = DiagnosticDefinitions.ExperimentalApiUrlFormat)]
#endif
public
#else
internal
#endif
abstract class FixedSizeExemplarReservoir : ExemplarReservoir
{
private readonly Exemplar[] runningExemplars;
private readonly Exemplar[] snapshotExemplars;

/// <summary>
/// Initializes a new instance of the <see cref="FixedSizeExemplarReservoir"/> class.
/// </summary>
/// <param name="capacity">The capacity (number of <see cref="Exemplar"/>s)
/// to be contained in the reservoir.</param>
#pragma warning disable RS0022 // Constructor make noninheritable base class inheritable
protected FixedSizeExemplarReservoir(int capacity)
#pragma warning restore RS0022 // Constructor make noninheritable base class inheritable
{
// Note: RS0022 is suppressed because we do want to allow custom
// ExemplarReservoir implementations to be created by deriving from
// FixedSizeExemplarReservoir.

Guard.ThrowIfOutOfRange(capacity, min: 1);

this.runningExemplars = new Exemplar[capacity];
this.snapshotExemplars = new Exemplar[capacity];
this.Capacity = capacity;
}

internal int Capacity { get; }
/// <summary>
/// Gets the capacity (number of <see cref="Exemplar"/>s) contained in the
/// reservoir.
/// </summary>
public int Capacity { get; }

/// <summary>
/// Collects all the exemplars accumulated by the Reservoir.
Expand Down Expand Up @@ -56,12 +87,48 @@ internal sealed override void Initialize(AggregatorStore aggregatorStore)
base.Initialize(aggregatorStore);
}

internal void UpdateExemplar<T>(
int exemplarIndex,
in ExemplarMeasurement<T> measurement)
where T : struct
{
this.runningExemplars[exemplarIndex].Update(in measurement);
}

/// <summary>
/// Fired when <see cref="Collect"/> has finished before returning a <see cref="ReadOnlyExemplarCollection"/>.
/// </summary>
/// <remarks>
/// Note: This method is typically used to reset the state of reservoirs and
/// is called regardless of the value of <see
/// cref="ExemplarReservoir.ResetOnCollect"/>.
/// </remarks>
protected virtual void OnCollected()
{
}

protected void UpdateExemplar<T>(int exemplarIndex, in ExemplarMeasurement<T> measurement)
where T : struct
/// <summary>
/// Updates the <see cref="Exemplar"/> stored in the reservoir at the
/// specified index with an <see cref="ExemplarMeasurement{T}"/>.
/// </summary>
/// <param name="exemplarIndex">Index of the <see cref="Exemplar"/> to update.</param>
/// <param name="measurement"><see cref="ExemplarMeasurement{T}"/>.</param>
protected void UpdateExemplar(
int exemplarIndex,
in ExemplarMeasurement<long> measurement)
{
this.runningExemplars[exemplarIndex].Update(in measurement);
}

/// <summary>
/// Updates the <see cref="Exemplar"/> stored in the reservoir at the
/// specified index with an <see cref="ExemplarMeasurement{T}"/>.
/// </summary>
/// <param name="exemplarIndex">Index of the <see cref="Exemplar"/> to update.</param>
/// <param name="measurement"><see cref="ExemplarMeasurement{T}"/>.</param>
protected void UpdateExemplar(
int exemplarIndex,
in ExemplarMeasurement<double> measurement)
{
this.runningExemplars[exemplarIndex].Update(in measurement);
}
Expand Down
6 changes: 6 additions & 0 deletions src/OpenTelemetry/Metrics/MetricPoint.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@
// SPDX-License-Identifier: Apache-2.0

using System.Diagnostics;
#if EXPOSE_EXPERIMENTAL_FEATURES && NET8_0_OR_GREATER
using System.Diagnostics.CodeAnalysis;
#endif
using System.Runtime.CompilerServices;
using OpenTelemetry.Internal;

Expand Down Expand Up @@ -370,6 +373,9 @@ public readonly bool TryGetHistogramMinMaxValues(out double min, out double max)
/// <param name="exemplars"><see cref="ReadOnlyExemplarCollection"/>.</param>
/// <returns><see langword="true" /> if exemplars exist; <see langword="false" /> otherwise.</returns>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
#if NET8_0_OR_GREATER
[Experimental(DiagnosticDefinitions.ExemplarExperimentalApi, UrlFormat = DiagnosticDefinitions.ExperimentalApiUrlFormat)]
#endif
public
#else
[MethodImpl(MethodImplOptions.AggressiveInlining)]
Expand Down
4 changes: 2 additions & 2 deletions src/OpenTelemetry/Metrics/MetricStreamConfiguration.cs
Original file line number Diff line number Diff line change
Expand Up @@ -144,15 +144,15 @@ public string[]? TagKeys
/// when storing <see cref="Exemplar"/>s.
/// </summary>
/// <remarks>
/// <inheritdoc cref="Exemplar" path="/remarks/para[@experimental-warning='true']"/>
/// <inheritdoc cref="ExemplarReservoir" path="/remarks/para[@experimental-warning='true']"/>
/// <para>Note: Returning <see langword="null"/> from the factory function will
/// result in the default <see cref="ExemplarReservoir"/> being chosen by
/// the SDK based on the type of metric.</para>
/// Specification: <see
/// href="https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#stream-configuration"/>.
/// </remarks>
#if NET8_0_OR_GREATER
[Experimental(DiagnosticDefinitions.ExemplarExperimentalApi, UrlFormat = DiagnosticDefinitions.ExperimentalApiUrlFormat)]
[Experimental(DiagnosticDefinitions.ExemplarReservoirExperimentalApi, UrlFormat = DiagnosticDefinitions.ExperimentalApiUrlFormat)]
#endif
public Func<ExemplarReservoir?>? ExemplarReservoirFactory { get; set; }
#else
Expand Down
1 change: 1 addition & 0 deletions src/Shared/DiagnosticDefinitions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,5 @@ internal static class DiagnosticDefinitions
public const string LogsBridgeExperimentalApi = "OTEL1001";
public const string ExemplarExperimentalApi = "OTEL1002";
public const string CardinalityLimitExperimentalApi = "OTEL1003";
public const string ExemplarReservoirExperimentalApi = "OTEL1004";
}
Loading