Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot restart metrics #4384

Closed
vidommet opened this issue Apr 12, 2023 · 6 comments
Closed

Cannot restart metrics #4384

vidommet opened this issue Apr 12, 2023 · 6 comments
Labels
bug Something isn't working Stale Issues and pull requests which have been flagged for closing due to inactivity

Comments

@vidommet
Copy link

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 vidommet added the bug Something isn't working label Apr 12, 2023
@vidommet
Copy link
Author

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.

@utpilla
Copy link
Contributor

utpilla commented Apr 14, 2023

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.

internal void CompleteSingleStreamMeasurement(Metric metric)
{
metric.InstrumentDisposed = true;
}

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.

if (metric != null)
{
if (metric.InstrumentDisposed)
{
metricPointSize = metric.Snapshot();
this.instrumentIdentityToMetric.TryRemove(metric.InstrumentIdentity, out var _);
this.metrics[i] = null;
}
else
{
metricPointSize = metric.Snapshot();
}
if (metricPointSize > 0)
{
this.metricsCurrentBatch[metricCountCurrentBatch++] = metric;
}
}

@utpilla
Copy link
Contributor

utpilla commented Apr 14, 2023

@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);
        }
    }
}

@CodeBlanch
Copy link
Member

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

Copy link
Contributor

This issue was marked stale due to lack of activity and will be closed in 7 days. Commenting will instruct the bot to automatically remove the label. This bot runs once per day.

@github-actions github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Oct 15, 2024
Copy link
Contributor

Closed as inactive. Feel free to reopen if this issue is still a concern.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Stale Issues and pull requests which have been flagged for closing due to inactivity
Projects
None yet
Development

No branches or pull requests

3 participants