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

[CodeBlanch]

Relates to #2527

Changes

• Defines OTEL1004 and moves ExemplarReservoir, ExemplarMeasurement<T>, and MetricStreamConfiguration.ExemplarReservoirFactory under it
• Exposes FixedSizeExemplarReservoir under OTEL1004 so that users may author custom ExemplarReservoirs

Details

@alanwest, @cijothomas, and I have been working on a plan for exposing Exemplars in 1.9.0. See: #2527

This PR is performing the work as described in that plan. Primarily, we have decided that ExemplarReservoir and custom ExemplarReservoir support will NOT be part of the first stable release. It has been given its own diagnostic so that Exemplar, SetExemplarFilter, and ExemplarFilterType can be released on their own first.

As far as the plan for supporting custom ExemplarReservoir in prerelease builds...

The spec says:

The SDK MUST provide a mechanism for SDK users to provide their own ExemplarReservoir implementation.

We have public abstract class ExemplarReservoir but that isn't the full story. There is currently no way set or update a struct Exemplar so actually implementing an ExemplarReservoir is impossible.

We could expose an update method or setters on struct Exemplar but that also isn't the full story. Exemplars have to be initialized with some magic in order for the tag filtering parts to work.

The idea here is to expose FixedSizeExemplarReservoir which takes care of all the heavy lifting and management of Exemplars allowing implementations to focus on their algorithms. Java seems to have similar but not totally sure if/how it is exposed.

Chatting with @cijothomas we may want to just ignore this MUST from the spec. What I did is give FixedSizeExemplarReservoir its own experimental API definition. The goal there is allow us to ship the other parts we are comfortable with independently of the support for custom reservoirs while still allowing users to try it out in prerelease builds.

Example custom reservoir implementation

internal sealed class HighestValueExemplarReservoir : FixedSizeExemplarReservoir
{
    private readonly object lockObject = new();
    private long? previousValueLong;
    private double? previousValueDouble;

    public HighestValueExemplarReservoir()
        : base(1)
    {
    }

    public override void Offer(in ExemplarMeasurement<long> measurement)
    {
        if (!this.previousValueLong.HasValue || measurement.Value > this.previousValueLong.Value)
        {
            lock (this.lockObject)
            {
                if (!this.previousValueLong.HasValue || measurement.Value > this.previousValueLong.Value)
                {
                    this.UpdateExemplar(0, in measurement);
                    this.previousValueLong = measurement.Value;
                }
            }
        }
    }

    public override void Offer(in ExemplarMeasurement<double> measurement)
    {
        if (!this.previousValueDouble.HasValue || measurement.Value > this.previousValueDouble.Value)
        {
            lock (this.lockObject)
            {
                if (!this.previousValueDouble.HasValue || measurement.Value > this.previousValueDouble.Value)
                {
                    this.UpdateExemplar(0, in measurement);
                    this.previousValueDouble = measurement.Value;
                }
            }
        }
    }

    protected override void OnCollected()
    {
        if (this.ResetOnCollect)
        {
            lock (this.lockObject)
            {
                this.previousValueLong = null;
                this.previousValueDouble = null;
            }
        }
    }
}

Merge requirement checklist

• CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
• Appropriate CHANGELOG.md files updated for non-trivial changes
• Changes in public API reviewed (if applicable)

[cijothomas]

Chatting with @cijothomas we may want to just ignore this MUST from the spec. What I did is give FixedSizeExemplarReservoir its own experimental API definition. The goal there is allow us to ship the other parts we are comfortable with independently of the support for custom reservoirs while still allowing users to try it out in prerelease builds.

To be more precise: I think we should refrain from exposing the ability to write custom Reservoirs in the initial release.
There are already certain aspects on Metrics spec we are not yet exposing public API for.
eg1 - View API does not allow changing aggregation though spec allows that.
eg2 - The temporality is applied at MeterProvider with built-ins, and no API to define it per instrument type, though spec allows that.

[codecov]

Codecov Report

Attention: Patch coverage is 71.42857% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 85.48%. Comparing base (6250307) to head (8eff6b2).
Report is 215 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5558      +/-   ##
==========================================
+ Coverage   83.38%   85.48%   +2.10%     
==========================================
  Files         297      265      -32     
  Lines       12531    11337    -1194     
==========================================
- Hits        10449     9692     -757     
+ Misses       2082     1645     -437     

Flag	Coverage Δ
unittests	?
unittests-Solution-Experimental	85.69% <71.42%> (?)
unittests-Solution-Stable	85.69% <71.42%> (?)
unittests-Unstable-Core	19.85% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...nTelemetry/Metrics/Exemplar/ExemplarMeasurement.cs	100.00% <ø> (ø)
...penTelemetry/Metrics/Exemplar/ExemplarReservoir.cs	100.00% <100.00%> (ø)
src/OpenTelemetry/Metrics/MetricPoint.cs	94.24% <ø> (+25.76%)	⬆️
...OpenTelemetry/Metrics/MetricStreamConfiguration.cs	88.88% <ø> (+13.88%)	⬆️
...try/Metrics/Exemplar/FixedSizeExemplarReservoir.cs	92.85% <60.00%> (ø)

... and 107 files with indirect coverage changes

[cijothomas]

nits: Spec is already stable, so we can remove that part.

[CodeBlanch]

@cijothomas I removed the stable part and added a lot more detail. Please re-review and LMK what you think.

[vishweshbankwar]

Do we not need this exposed?

[CodeBlanch]

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.

[vishweshbankwar]

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.

[vishweshbankwar]

LGTM