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

CreateCounter can create corruptable counter #4843

Closed
davhdavh opened this issue Aug 8, 2024 · 4 comments · Fixed by #4938
Closed

CreateCounter can create corruptable counter #4843

davhdavh opened this issue Aug 8, 2024 · 4 comments · Fixed by #4938
Assignees
Milestone

Comments

@davhdavh
Copy link

davhdavh commented Aug 8, 2024

Description

👍 This will correctly register 1 metrics with 2 different tag values for type:

Meter meter = new Meter("HatCo.Store");
Counter<int> blueHatsSold = meter.CreateCounter<int>("hatco.store.hats_sold");
Counter<int> redHatsSold = meter.CreateCounter<int>("hatco.store.hats_sold");

Console.WriteLine("Press any key to exit");
while (!Console.KeyAvailable)
{
   // Pretend our store has a transaction each second that sells 4 hats
   Thread.Sleep(1000);
   redHatsSold.Add(4, tag: new("type", "red"));
   blueHatsSold.Add(5, tag: new("type", "blue"));
}

👎 This creates 2 counter with same name, but tags specified at creation:

Meter meter = new Meter("HatCo.Store");
Counter<int> blueHatsSold = meter.CreateCounter<int>("hatco.store.hats_sold", null, null, [new("type", "blue")]);
Counter<int> redHatsSold = meter.CreateCounter<int>("hatco.store.hats_sold", null, null, [new("type", "red")]);

Console.WriteLine("Press any key to exit");
while (!Console.KeyAvailable)
{
   // Pretend our store has a transaction each second that sells 4 hats
   Thread.Sleep(1000);
   redHatsSold.Add(4);
   blueHatsSold.Add(5);
}

dotnet-counters monitor -n ConsoleApp10 --counters HatCo.Store --showDeltas shows just 1 metric label for type=blue, that consistently shows a value that matches the blue; but a delta value that is the difference between blue and red.
So somehow the same metrics is reported on top of itself with wrong tags.

Reproduction Steps

See above code examples

Expected behavior

Both code examples work the same

Actual behavior

invalid metrics reported

Regression?

No response

Known Workarounds

The first example obviously works

Configuration

8.0

Other information

No response

@KalleOlaviNiemitalo
Copy link

For .NET Runtime, I think this is a duplicate of dotnet/runtime#93767, which was fixed in dotnet/runtime#104993 before v9.0.0-preview.6.24327.7.

For dotnet-counters, TraceEventExtensions will have to be changed to use the new instrumentId payload field. AFAICT, there's no pull request for that yet. I guess it'll be tracked in the issue #4564, even though that one talks about multiple meters rather than multiple instruments in the same meter.

@ericstj
Copy link
Member

ericstj commented Aug 9, 2024

@tarekgh can you confirm @KalleOlaviNiemitalo's analysis

@ericstj ericstj added the question Further information is requested label Aug 9, 2024
@tarekgh
Copy link
Member

tarekgh commented Aug 9, 2024

Yes @KalleOlaviNiemitalo is correct. We added a support to emit a unique Id for every instrument in the runtime. Tools like dotnet-counter and dotnet-monitor or any similar tools need to benefit from that and ensure distinguishing between instruments created with the same name. We need to transfer this issue to the https://github.com/dotnet/diagnostics repo.

@noahfalk @davmason @tommcdon @brianrob I don't have write permission on the diagnostics repo to transfer the issue there, can anyone of you help transferring it there?

@tarekgh tarekgh added this to the Future milestone Aug 9, 2024
@davmason davmason transferred this issue from dotnet/runtime Aug 9, 2024
@noahfalk
Copy link
Member

I guess it'll be tracked in the issue #4564, even though that one talks about multiple meters rather than multiple instruments in the same meter.

I'm happy to leave both this issue and #4564 open for tracking multiple instruments and multiple meters respectively. They technically could be fixed independently even though probably one PR is going to resolve both at the same time.

@tommcdon tommcdon added dotnet-counters and removed question Further information is requested labels Aug 12, 2024
@tommcdon tommcdon modified the milestones: Future, 9.0.0 Aug 12, 2024
noahfalk added a commit to noahfalk/diagnostics that referenced this issue Sep 17, 2024
In .NET 8.0 we added tags on Meters and Instruments but MetricsEventSource only included the names when emitting value publishing events. This made it ambiguous when there was more than one Meter or Instrument with the same name. In 9.0 MetricsEventSource started included InstrumentIDs in the BeginInstrumentReporting events and in the value publishing events that provides a stronger correlation identifier. This change consumes the IDs in those events within Microsoft.Diagnostics.Monitoring.EventPipe, allowing dotnet-counters (and presumably dotnet-monitor too) to track time series that differ only by the tags provided to the Meter/Instrument constructors.

I also re-enabled a disabled test that got broken .NET 9.0 added the System.Runtime Meter.

Fixes dotnet#4843, dotnet#4564
@github-actions github-actions bot locked and limited conversation to collaborators Oct 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants