[feature request] Better handling of cardinality limits

[ManickaP]

Package

None

Is your feature request related to a problem?

Based on the code:
https://github.com/open-telemetry/opentelemetry-dotnet/blame/main/src/OpenTelemetry/Metrics/AggregatorStore.cs#L487
it looks like the cardinality limit preserve the first values instead of the newest. For more context see discussion in dotnet/runtime#122752 (comment)

The original problem is reported in Azure/azure-sdk-for-net#52674

What is the expected behavior?

Cardinality limit should preserve the most recent values.

Which alternative solutions or features have you considered?

Additional context

No response

Tip

_{React with 👍 to help prioritize this issue. Please use comments to provide useful context, avoiding +1 or me too, to help us triage it. Learn more here.}

[cijothomas]

Cardinality limit should preserve the most recent values.

Can you elaborate? If using Cumulative OTel spec mandates the original values MUST be preserved.

For Delta spec is flexible, and OTel .NET already deletes unused key-value pair combinations and hence, newer ones automatically get stored. If you don't see this behavior, could you share a minimal repro? There are unit-tests already in the repo testing these, and they can used as starting point.

[ManickaP]

Note that this is very synthetic repro I put it together to demonstrate the problem with minimum code. Issue is mainly with instruments with dimension with high cardinality over time. In the linked issues we talk about http.client.open_connections counter and recommended server.name tag which, for certain applications, might accumulate a lot of different values for the tag over time.

The skeleton for the code bellow comes from https://learn.microsoft.com/en-us/dotnet/core/diagnostics/observability-otlp-example.

using System.Diagnostics;
using System.Diagnostics.Metrics;
using OpenTelemetry;
using OpenTelemetry.Metrics;
using OpenTelemetry.Trace;


var builder = WebApplication.CreateBuilder(args);

// Custom metrics for the application
var meter = new Meter("OTel.Example", "1.0.0");
var counter = meter.CreateCounter<int>("count");

// Custom ActivitySource for the application
var counterActivitySource = new ActivitySource("OTel.Example");

var otel = builder.Services.AddOpenTelemetry();

otel.WithMetrics(metrics =>
{
    metrics.AddMeter(meter.Name).AddView(
        instrumentName: counter.Name,
        new MetricStreamConfiguration { CardinalityLimit = 10 });
});

// Export OpenTelemetry data via OTLP, using env vars for the configuration
var OtlpEndpoint = builder.Configuration["OTEL_EXPORTER_OTLP_ENDPOINT"];
if (OtlpEndpoint != null)
{
    otel.UseOtlpExporter();
}

var app = builder.Build();

int tag = 0;

app.MapGet("/", StartCounter);
async Task<string> StartCounter()
{
    for (int i = 0; i < 20; ++i) {
        TagList tags = default;
        tags.Add("tag", $"value={Interlocked.Increment(ref tag)}");
        counter.Add(1, tags);
    }
    return "Hello World!";
}
app.Run();

In the dashboard, I see only tags with values 1 through 10:

And the exporter is sending only the first ten measurements:

[cijothomas]

@ManickaP This repro just shows expected behavior. Cap is 10, Default Temporality (Cumulative) is used, so it'll only track 10 time-series., rest gets into "overflow". The Aspire dashboard has a unique (!!Yes) capability to determine overflow and show that warning in the UI.

If you switch to Delta temporality, and then unused KeyValue combinations, gets "cleaned up" after 1-2 exports, leaving room for new combinations. In other words, the new values gets tracked, old ones gone.

[noahfalk]

@cijothomas - I think the issue here is that combining the semantic conventions spec and the underlying metrics spec leads customers to get undesirable behavior. I'm not arguing you should break the spec (at least not unilaterally), but it would be good if we can try to get some understanding about how do we make the overall user experience good. I imagine we've got a couple options:

1. The metric implementer uses ObservableUpDownCounter and has the responsibility to avoid reporting zero-valued dimensions.
2. We change the metrics spec in some way so that SDKs stop reporting the older zero-valued data points
3. We change the semantic conventions to avoid the high cardinality dimensions that lead to the stale data points
4. We change the defaults and/or guidance for users so that Delta temporality is preferred.

Not sure if you are aware of any guidance or similar existing issues?

[cijothomas]

The metric implementer uses ObservableUpDownCounter

This gives me an idea as to what's going on.The root cause of this issue is the bug: #5950 which is a long existing bug! I added a new PR now just to re-confirm the bug (not a fix) #6820

The metric implementer uses ObservableUpDownCounter and has the responsibility to avoid reporting zero-valued dimensions.

^ Even if the metric implementer does the right thing, due to the SDK bug, we keep reporting all the old time series.

The way to address this issue is to fix the bug first. Once that is done, SDK would forget unreported time-series, making room for new ones.