[sdk-metrics] Remove ExemplarFilter classes

[CodeBlanch]

Follow-up to #5404

Changes

• Remove the ExemplarFilter class and the implementations.
• Refactor AggregatorStore to use ExemplarFilterType inline.

Benchmarks

I tried a bunch of different things to try and squeeze as much perf out of things as I could. Seems like doing a simple inline switch is the best:

Method	ExemplarFilterType	Mean	Error	StdDev	Allocated
UpdateOldStyle	AlwaysOff	1.328 ns	0.0293 ns	0.0259 ns	-
UpdateInlineStyle	AlwaysOff	1.302 ns	0.0235 ns	0.0220 ns	-
UpdateOldStyle	AlwaysOn	1.334 ns	0.0239 ns	0.0212 ns	-
UpdateInlineStyle	AlwaysOn	1.308 ns	0.0170 ns	0.0159 ns	-
UpdateOldStyle	TraceBased	2.676 ns	0.0438 ns	0.0388 ns	-
UpdateInlineStyle	TraceBased	2.373 ns	0.0643 ns	0.0570 ns	-

Code

#nullable enable

using System.Diagnostics;
using System.Runtime.CompilerServices;
using BenchmarkDotNet.Attributes;
using OpenTelemetry.Metrics;

namespace Benchmarks.Metrics;

public class ExemplarFilterBenchmarks
{
    private ExemplarFilter? exemplarFilter;

    [Params(ExemplarFilterType.AlwaysOff, ExemplarFilterType.AlwaysOn, ExemplarFilterType.TraceBased)]
    public ExemplarFilterType ExemplarFilterType { get; set; }

    [GlobalSetup]
    public void GlobalSetup()
    {
        this.exemplarFilter = this.ExemplarFilterType switch
        {
            ExemplarFilterType.AlwaysOn => new AlwaysOnExemplarFilter(),
            ExemplarFilterType.TraceBased => new TraceBasedExemplarFilter(),
            _ => new AlwaysOffExemplarFilter(),
        };
    }

    [Benchmark]
    public void UpdateOldStyle()
    {
        // TODO: can special case built-in filters to be bit faster.
        if (this.IsExemplarEnabled())
        {
            var shouldSample = this.exemplarFilter!.ShouldSample(0, tags: default);
            this.UpdateWithExemplar(0, tags: default, shouldSample);
        }
        else
        {
            this.Update(0);
        }
    }

    [Benchmark]
    public void UpdateInlineStyle()
    {
        switch (this.ExemplarFilterType)
        {
            case ExemplarFilterType.AlwaysOn:
                this.UpdateWithExemplar(
                    0,
                    tags: default,
                    isSampled: true);
                break;
            case ExemplarFilterType.TraceBased:
                this.UpdateWithExemplar(
                    0,
                    tags: default,
                    isSampled: Activity.Current?.Recorded ?? false);
                break;
            default:
#if DEBUG
                if (this.ExemplarFilterType != ExemplarFilterType.AlwaysOff)
                {
                    Debug.Fail("Unexpected ExemplarFilterType");
                }
#endif
                this.Update(0);
                break;
        }
    }

    private bool IsExemplarEnabled()
    {
        // Using this filter to indicate On/Off
        // instead of another separate flag.
        return this.exemplarFilter is not AlwaysOffExemplarFilter;
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    private void Update(long value)
    {
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    private void UpdateWithExemplar(long value, ReadOnlySpan<KeyValuePair<string, object?>> tags, bool isSampled)
    {
    }

    internal abstract class ExemplarFilter
    {
        public abstract bool ShouldSample(long value, ReadOnlySpan<KeyValuePair<string, object?>> tags);

        public abstract bool ShouldSample(double value, ReadOnlySpan<KeyValuePair<string, object?>> tags);
    }

    internal sealed class AlwaysOffExemplarFilter : ExemplarFilter
    {
        public override bool ShouldSample(long value, ReadOnlySpan<KeyValuePair<string, object?>> tags)
            => false;

        public override bool ShouldSample(double value, ReadOnlySpan<KeyValuePair<string, object?>> tags)
            => false;
    }

    internal sealed class AlwaysOnExemplarFilter : ExemplarFilter
    {
        public override bool ShouldSample(long value, ReadOnlySpan<KeyValuePair<string, object?>> tags)
            => true;

        public override bool ShouldSample(double value, ReadOnlySpan<KeyValuePair<string, object?>> tags)
            => true;
    }

    internal sealed class TraceBasedExemplarFilter : ExemplarFilter
    {
        public override bool ShouldSample(long value, ReadOnlySpan<KeyValuePair<string, object?>> tags)
            => Activity.Current?.Recorded ?? false;

        public override bool ShouldSample(double value, ReadOnlySpan<KeyValuePair<string, object?>> tags)
            => Activity.Current?.Recorded ?? false;
    }
}

Merge requirement checklist

• CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
• Unit tests added/updated

[codecov]

Codecov Report

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

Project coverage is 84.73%. Comparing base (6250307) to head (08a51f8).
Report is 110 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5408      +/-   ##
==========================================
+ Coverage   83.38%   84.73%   +1.34%     
==========================================
  Files         297      281      -16     
  Lines       12531    12078     -453     
==========================================
- Hits        10449    10234     -215     
+ Misses       2082     1844     -238     

Flag	Coverage Δ
unittests	?
unittests-Solution-Experimental	84.71% <72.05%> (?)
unittests-Solution-Stable	84.67% <72.05%> (?)

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

Files	Coverage Δ
.../Metrics/Builder/MeterProviderBuilderExtensions.cs	98.42% <100.00%> (-1.58%)	⬇️
...lemetry/Metrics/Builder/MeterProviderBuilderSdk.cs	98.36% <100.00%> (ø)
src/OpenTelemetry/Metrics/Metric.cs	98.38% <100.00%> (+1.86%)	⬆️
src/OpenTelemetry/Metrics/MetricReaderExt.cs	91.50% <ø> (+1.50%)	⬆️
src/OpenTelemetry/Metrics/AggregatorStore.cs	87.18% <70.76%> (+6.79%)	⬆️

... and 51 files with indirect coverage changes

[utpilla]

It would probably be faster to use if than switch? If yes, we should optimize the non-exemplar updates.

[Yun-Ting]

Found an old benchmark saying that switch usually will be faster cause compiler will create jump table for switch statement: https://www.blackwasp.co.uk/SpeedTestIfElseSwitch.aspx.
I guess the assembly generated for if/switch here will be identical given there are only 3 cases.

[utpilla]

If you know your most common condition, you can make that the very first check. That way you only have to check if(true) to execute the most common path. That might be faster than the jump table lookup. In fact, you can use a mixed approach as well. Something like this:

if (mostFrequentlyHitCondition)
{
   RunSomething();
}

switch (lessFrequentlyHitConditions)
{
case Condition1:

case Condition2:

...

default:

}

[CodeBlanch]

I updated the code to use an if block. But it has been interesting to try and prove what is best.

Here are some benchmarks run without PGO enabled:

Method	ExemplarFilterType	Mean	Error	StdDev	Allocated
UpdateOldStyle	AlwaysOff	2.458 ns	0.0722 ns	0.0676 ns	-
UpdateInlineSwitchStyle	AlwaysOff	1.572 ns	0.0366 ns	0.0342 ns	-
UpdateInlineIfStyle	AlwaysOff	1.353 ns	0.0457 ns	0.0626 ns	-
UpdateOldStyle	AlwaysOn	2.938 ns	0.0206 ns	0.0172 ns	-
UpdateInlineSwitchStyle	AlwaysOn	1.800 ns	0.0343 ns	0.0304 ns	-
UpdateInlineIfStyle	AlwaysOn	1.800 ns	0.0366 ns	0.0342 ns	-
UpdateOldStyle	TraceBased	4.666 ns	0.0629 ns	0.0588 ns	-
UpdateInlineSwitchStyle	TraceBased	3.297 ns	0.0470 ns	0.0417 ns	-
UpdateInlineIfStyle	TraceBased	3.198 ns	0.0382 ns	0.0319 ns	-

Here are some benchmarks run with PGO enabled:

Method	ExemplarFilterType	Mean	Error	StdDev	Allocated
UpdateOldStyle	AlwaysOff	1.299 ns	0.0410 ns	0.0383 ns	-
UpdateInlineSwitchStyle	AlwaysOff	1.345 ns	0.0374 ns	0.0313 ns	-
UpdateInlineIfStyle	AlwaysOff	1.106 ns	0.0316 ns	0.0296 ns	-
UpdateOldStyle	AlwaysOn	1.359 ns	0.0141 ns	0.0132 ns	-
UpdateInlineSwitchStyle	AlwaysOn	1.373 ns	0.0514 ns	0.0481 ns	-
UpdateInlineIfStyle	AlwaysOn	1.325 ns	0.0310 ns	0.0275 ns	-
UpdateOldStyle	TraceBased	2.756 ns	0.0709 ns	0.0663 ns	-
UpdateInlineSwitchStyle	TraceBased	3.392 ns	0.0452 ns	0.0422 ns	-
UpdateInlineIfStyle	TraceBased	2.234 ns	0.0231 ns	0.0204 ns	-

PGO is smart enough to see that one value is used repeatedly and optimize the code on its own for that value. It is great for what we're doing here.

I ended up going with the if because PGO may not be there in AOT deployments/older runtimes so it seems best to optimize for it not being there.

[Yun-Ting]

Found an old benchmark saying that switch usually will be faster cause compiler will create jump table for switch statement: https://www.blackwasp.co.uk/SpeedTestIfElseSwitch.aspx.
I guess the assembly generated for if/switch here will be identical given there are only 3 cases.