opentelemetry-dotnet icon indicating copy to clipboard operation
opentelemetry-dotnet copied to clipboard

Cannot restart metrics

Open vidommet opened this issue 3 years ago • 5 comments

Bug Report

List of all OpenTelemetry NuGet packages and version that you are using (e.g. OpenTelemetry 1.0.2):

<PackageReference Include="OpenTelemetry.Exporter.Console" Version="1.4.0" />

Runtime version (e.g. net462, net48, netcoreapp3.1, net6.0 etc. You can find this information from the *.csproj file):

net6.0

Symptom

Once a meter has been disposed, a new meter with the same name doesn't work anymore.

What is the expected behavior?

The new meter with the same name should produce metrics as expected

What is the actual behavior?

Once the first meter has been disposed, no new metrics will be reported for the same meter.

Reproduce

var MyMeterInner = new Meter("MyCompany.MyProduct.MyLibrary.Inner", "1.0");
using var meterProvider = Sdk.CreateMeterProviderBuilder()
    .AddMeter("MyCompany.MyProduct.*")
    .AddConsoleExporter((a, b) => b.PeriodicExportingMetricReaderOptions.ExportIntervalMilliseconds = 1000)
    .Build();

MyMeterInner.CreateObservableGauge("testb", () => new Measurement<int>(new System.Random().Next(), new[] { new KeyValuePair<string, object?>("A", "a") }));

await Task.Delay(5000);
MyMeterInner.Dispose();
Console.WriteLine("Disposed");
// We will see Metrics until here

MyMeterInner = new("MyCompany.MyProduct.MyLibrary.Inner", "1.0");
MyMeterInner.CreateObservableGauge("testb", () => new Measurement<int>(new System.Random().Next(), new[] { new KeyValuePair<string, object?>("A", "a") }));

// No metrics here at all
await Task.Delay(5000);

Steps to repro

  1. Create a listener for a meter namespace and ConsoleExporter. 
  2. Create a meter and an observableguage
  3. Console Exporter works as expected
  4. Dispose the meter
  5. Console Exporter stops output
  6. Create a new meter with the same name as the original meter
  7. Create a new observableguage on the new meter
  8. Nothing in the console!!

vidommet avatar Apr 12 '23 15:04 vidommet

Use case: We have a HttpClient that wants a metric published. 'CurrentPoolSize'. This is an observable updown counter with some tags.

We have a bunch of clients that come up and go down regularly. Each of them wants to publish their metrics when they are up and stop when they are disposed of.

public HttpClient(TagList tagList)
        {
            this.meter = new Meter($"HttpClient.Metering");
            this.currentHTTPPoolSize = this.meter.CreateObservableUpDownCounter<int>(
                name: "CurrentPoolSize",
                observeValue: () => new Measurement<int>(this.poolImplementation?.PoolSize ?? 0, tagList));

I have to create the meter here and not use a static because diposal of the client doesn't work as expected otherwise. Because the counter captures "this" in its callback function, my disposal of the client doesn't mean the object can be GC'ed. The meter will have a reference to the counter which has a reference to my client.

vidommet avatar Apr 13 '23 20:04 vidommet

This is a bug in the SDK. When a Meter is disposed, MeterListener.MeasurementsCompleted for the instrument calls this method and marks metric.InstrumentDisposed to true.

https://github.com/open-telemetry/opentelemetry-dotnet/blob/eaaf10ab00064ed83e010c5020ce17bf22e19c8a/src/OpenTelemetry/Metrics/MetricReaderExt.cs#L208-L211

If a meter and an instrument with the same name is created again, before a Collect is called, it would use the same Metric that was already created for the instrument before its Meter was disposed. If a Collect is called after the meter are instrument are created again it would find metric.InstrumentDisposed to be true so it would collect the data for this metric one more time and then mark it as null. The subsequent Collect runs will ignore this metric as it would now be null.

https://github.com/open-telemetry/opentelemetry-dotnet/blob/eaaf10ab00064ed83e010c5020ce17bf22e19c8a/src/OpenTelemetry/Metrics/MetricReaderExt.cs#L249-L266

utpilla avatar Apr 14 '23 00:04 utpilla

@vidommet As a workaround, you could still use a static Meter for observable instruments that capture a reference to a disposable object. You could make use of the CreateObservableGauge overload which takes in an Func<IEnumerable<Measurement<T>>> as an input parameter.

Here's a sample code:

public class Program
{
    internal static readonly List<Func<Measurement<int>>> MeasurementFuncs = new List<Func<Measurement<int>>>();
    internal static readonly List<string> InstrumentNames = new List<string>();
    internal static Func<IEnumerable<Measurement<int>>> MeasurementFunc = () => GetMeasurements();

    internal static Meter CustomMeter = new Meter("MyCompany.MyProduct.MyLibrary");

    private static IEnumerable<Measurement<int>> GetMeasurements()
    {
        lock (MeasurementFuncs)
        {
            for (int i = 0; i < MeasurementFuncs.Count; i++)
            {
                yield return MeasurementFuncs[i]();
            }
        }
    }

    public static void Main()
    {
        using var meterProvider = Sdk.CreateMeterProviderBuilder()
            .AddMeter("MyCompany.MyProduct.MyLibrary")
            .AddConsoleExporter((exporterOptions, metricReaderOptions) =>
            {
                // Set the export interval to high number to avoid periodic exports
                metricReaderOptions.PeriodicExportingMetricReaderOptions.ExportIntervalMilliseconds = 100000;
            })
            .Build();

        var tags = new KeyValuePair<string, object>[] { new("key1", "value1"), new("key2", "value2") };
        var tagListforCustomClassA = new TagList(tags);

        var customClassA = new CustomClass("A", 100, 500, tagListforCustomClassA);

        tags = new KeyValuePair<string, object>[] { new("key3", "value3"), new("key4", "value4") };
        var tagListforCustomClassB = new TagList(tags);

        var customClassB = new CustomClass("B", 1000, 5000, tagListforCustomClassB);

        meterProvider.ForceFlush();

        customClassA.Dispose();
        customClassB.Dispose();

        Console.WriteLine();
        Console.WriteLine("Disposed custom classes");

        customClassA = new CustomClass("A", 100, 500, tagListforCustomClassA);
        customClassB = new CustomClass("B", 1000, 5000, tagListforCustomClassB);

        // Metrics continue to be exported
        meterProvider.ForceFlush();
        meterProvider.ForceFlush();
        meterProvider.ForceFlush();
        meterProvider.ForceFlush();
    }
}

public class CustomClass : IDisposable
{
    private readonly Random random;
    private readonly ObservableGauge<int> observableGauge;

    public CustomClass(string name, int min, int max, TagList tags)
    {
        this.random = new Random();
        this.observableGauge = Program.CustomMeter.CreateObservableGauge("ObservableGauge", Program.MeasurementFunc);

        this.Name = name;

        var measurementFunc = () => new Measurement<int>(this.random.Next(min, max), tags);

        lock (Program.MeasurementFuncs)
        {
            Program.MeasurementFuncs.Add(measurementFunc);
            Program.InstrumentNames.Add(this.Name);
        }
    }

    public string Name { get; set; }

    public void Dispose()
    {
        lock (Program.MeasurementFuncs)
        {
            var index = Program.InstrumentNames.IndexOf(this.Name);
            Program.InstrumentNames.RemoveAt(index);
            Program.MeasurementFuncs.RemoveAt(index);
        }
    }
}

utpilla avatar Apr 14 '23 01:04 utpilla

Update...

A change went in on #4958 which smooths this out a bit. If a meter and an instrument with the same name is created again, before a Collect is called, we will now create a new metric for it. If you do this a lot, you'll run out of metrics because we have a limited number. You'll also get duplicates on that first export. It isn't perfect, but it should be a little better.

There is an issue open to resolve it completely: #5064

CodeBlanch avatar Nov 22 '23 00:11 CodeBlanch