Degraded performance in metering when creating a lot of instruments

[xakep139]

Description

One of our partner services discovered that there's an issue when one creates a lot of instruments (e.g. Counter's) and drop them on the floor. The bottleneck is here: MeterListener.cs,57.

As access to DiagLinkedList is protected with lock, very likely there are too many nodes in the LinkedList, every time when CreateHistogram() or CreateCounter() is called, a new node will be added to the LinkedList, because EnableMeasurementEvents() uses object.ReferenceEquals comparer.

So, all the references to the created instruments will be held in a MeterListener instance and will cause _enabledMeasurementInstruments.AddIfNotExist(instrument, object.ReferenceEquals); to take longer and longer - while keeping the lock. That significantly increases CPU usage and causes a memory leak.

Configuration

Here's a code snippet that showcases the issue:

const int Iterations = 10_000;

var milliseconds = new List<long>(Iterations);
var sw = Stopwatch.StartNew();

using var meter = new Meter("My.Company");
using var meterListener = new MeterListener();
meterListener.InstrumentPublished += (instrument, listener) =>
{
    sw.Restart();

    listener.EnableMeasurementEvents(instrument);

    milliseconds.Add(sw.ElapsedMilliseconds);
};

meterListener.Start();

for (int i = 0; i < 100_000_000; i++)
{
    var counter = meter.CreateCounter<int>("myCounter");
    counter.Add(1);
    if (i % Iterations == 0)
    {
        Console.WriteLine("Iteration {0,10}: avg time is {1} ms, max is {2} ms", i, milliseconds.Average(), milliseconds.Max());
        milliseconds.Clear();
    }
}

Regression?

I tried the code above on different combinations of net7.0 and net6.0 vs. System.Diagnostics.DiagnosticSource versions 6.0.0 and 7.0.2.

Data

Analysis

Surely this can be worked around when using a cache of created instruments.
Since the Instrument doesn't implement IDisposable, it's hard to figure out that it's not recommended to create a lot of instruments.

[xakep139]

cc @dpk83

[tommcdon]

@noahfalk @tarekgh

[noahfalk]

@xakep139 - Thanks for issue report! As part of work for .NET 8 we were already investigating making Meter.CreateXXX() cache the instruments it creates and return those cached values if you call it with the same arguments. In the example shown above that would change it from having 100M instruments to only having one. However would the scenario in your real production code benefit from that type of caching in the same way?

[xakep139]

@noahfalk, thanks for the reply! Indeed, our partner service creates most of the instruments with the same name, so if it was/will be addressed in .NET 8, then it would solve the issue. Can you point me to the related issue/PR please?

[noahfalk]

Its hidden away as a subset of the work in this issue: #77514
Check out bullet-point 3 of this part of the design-proposal for addressing that issue.

[tarekgh]

so if it was/will be addressed in .NET 8, then it would solve the issue.

For now, can't you just cache the created instrument instead of creating it every time?

[xakep139]

so if it was/will be addressed in .NET 8, then it would solve the issue.

For now, can't you just cache the created instrument instead of creating it every time?

This workaround was covered in Analysis section

[hoyosjs]

We're closing this as a dupe of #77514 since the work comprised there will solve this issue.