Implement/Convert existing EventCounters to Metrics

[samsp-msft]

The System.Diagnostics.Metrics api was added with support for dimensioned metrics, but it doesn't seem to have been utilized much by the framework. We should have an effort to go through the framework and update usage of EventCounter to Meter, adding dimensions where applicable.

The main areas that should be considered are:

• Http
• Crypto?
• Disk IO?

Tracking work in external repos:

• Modify EventCounter usage to support the new metric APIs as well aspnetcore#33387

[ghost]

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

---

The System.Diagnostics.Metrics api was added with support for dimensioned metrics, but it doesn't seem to have been utilized much by the framework. We should have an effort to go through the framework and update usage of EventCounter to Meter, adding dimensions where applicable.

The main areas that should be considered are:

• Http
• Crypto?
• Disk IO?

Author:	samsp-msft
Assignees:	-
Labels:	area-System.Net
Milestone:	-

[ghost]

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

Issue Details

---

The System.Diagnostics.Metrics api was added with support for dimensioned metrics, but it doesn't seem to have been utilized much by the framework. We should have an effort to go through the framework and update usage of EventCounter to Meter, adding dimensions where applicable.

The main areas that should be considered are:

• Http
• Crypto?
• Disk IO?

Author:	samsp-msft
Assignees:	-
Labels:	area-System.Diagnostics, area-System.Net
Milestone:	-

[samsp-msft]

Tagging @noahfalk, @davidfowl @karelz

[tommcdon]

@samsp-msft do we have issues in the appropriate repo's tracking the requested work?

[davidfowl]

We should open issues and tag the appropriate teams on the right repos. We'll need to flesh out each of the issues with what each counter definition looks like and what dimensions it should have.

[samsp-msft]

@samsp-msft do we have issues in the appropriate repo's tracking the requested work?

Not yet. We should figure out what the guidance is before we explode out the work. For example, what dimensions should we the metrics use. AFAIK both dimension key & value counts should be small - how small is the question.

[karelz]

Tagging @MihaZupan

[davidfowl]

@JamesNK did work here that's now checked into ASP.NET Core. We can use that to help drive some of the open questions here.

[karelz]

@samsp-msft @noahfalk (cc @tarekgh) ... how is it different from #84978. Is it a dupe?

[tarekgh]

We should have an effort to go through the framework and update usage of EventCounter to Meter, adding dimensions where applicable.

This issue needs to list the specific cases and as suggested before to open separate issues for each area.

[karelz]

@antonfirsov can you please check Networking how many EventCounters we didn't convert yet?
Do we plan to do it in 8.0 or should we move it to Future?

[antonfirsov]

I opened separate issues #88383 and #88384 to cover System.Net.*.

[tommcdon]

.NET 8 work is tracked on other linked issues, the remaining work will be targeting .NET 9

[tommcdon]

We believe we have ported most EventCounters. Please feel free to open new issues to track specific counters that need to be created/ported.

[stephentoub]

We believe we have ported most EventCounters. Please feel free to open new issues to track specific counters that need to be created/ported.

Do we have any plans to remove the EventCounters (e.g.

runtime/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/RuntimeEventSource.cs

Lines 25 to 52 in fcb9b18

	private static RuntimeEventSource? s_RuntimeEventSource;
	private PollingCounter? _gcHeapSizeCounter;
	private IncrementingPollingCounter? _gen0GCCounter;
	private IncrementingPollingCounter? _gen1GCCounter;
	private IncrementingPollingCounter? _gen2GCCounter;
	private PollingCounter? _gen0BudgetCounter;
	private PollingCounter? _cpuTimeCounter;
	private PollingCounter? _workingSetCounter;
	private PollingCounter? _threadPoolThreadCounter;
	private IncrementingPollingCounter? _monitorContentionCounter;
	private PollingCounter? _threadPoolQueueCounter;
	private IncrementingPollingCounter? _completedItemsCounter;
	private IncrementingPollingCounter? _allocRateCounter;
	private PollingCounter? _timerCounter;
	private PollingCounter? _fragmentationCounter;
	private PollingCounter? _committedCounter;
	private IncrementingPollingCounter? _exceptionCounter;
	private PollingCounter? _gcTimeCounter;
	private IncrementingPollingCounter? _totalGcPauseTimeCounter;
	private PollingCounter? _gen0SizeCounter;
	private PollingCounter? _gen1SizeCounter;
	private PollingCounter? _gen2SizeCounter;
	private PollingCounter? _lohSizeCounter;
	private PollingCounter? _pohSizeCounter;
	private PollingCounter? _assemblyCounter;
	private PollingCounter? _ilBytesJittedCounter;
	private PollingCounter? _methodsJittedCounter;
	private IncrementingPollingCounter? _jitTimeCounter;

) or is the plan to have both forever more?

[tarekgh]

Removing the event counters will be a breaking change which I don't think we need to make, at least at the current time. But I believe we can do it in the future after getting most of the users to migrate to the metrics.

CC @noahfalk @samsp-msft

[noahfalk]

Do we have any plans to remove the EventCounters

I feel similarly to Tarek. I hope to shift people away from using them over time and try simplify our messaging to focus on the Meter API, but no plan anytime soon on removing EventCounters from the codebase.

In terms of the cost-benefit of removing them from the product, for now the costs seem high - it would be a breaking change for many customers. As folks move away from them that cost should decrease over time. The main benefits I would see are shrinking the size of S.P.C. a little, perhaps a very tiny change in startup perf? Reducing a bit of support costs could be another. If other folks have expectations of greater benefits I'm all ears, but so far the upside seemed pretty small relative to breaking change pain.

[jkotas]

I hope to shift people away from using them over time

Using dotnet-counters for monitoring a console app that does something boring like reading and writing files works out of the box. Do we have a guideline for how to achieve the equivalent with the Meter APIs?

[noahfalk]

Do we have a guideline for how to achieve the equivalent with the Meter APIs?

They can continue doing the same thing, dotnet-counters has supported the Meter API since it was introduced in .NET 6. In a command like 'dotnet-counters --counters NAME` the NAME value can either refer to a Meter name or it can refer to an EventSource name.

A link to our docs about it: https://learn.microsoft.com/en-us/dotnet/core/diagnostics/metrics-collection#view-metrics-with-dotnet-counters

[jkotas]

I meant what one needs to do in the app itself. E.g. if I want to see the newly added runtime meters in dotnet-counters, I assume that I have to modify the code of my boring app to make that work. Is that correct?

The meters are not built into the runtime, so they are not registered by default unlike the event source runtime counters that Stephen linked to above.

[noahfalk]

For most Meters, yes, the user is intended to invoke something that will causes the Meter constructor to run. For something like ASP.NET it is indirect behind other general purpose ASP.NET startup APIs, for a custom Meter the user might create and invoke the constructor directly in their own code. Once the Meter is constructed that is sufficient to use it with dotnet-counters.

The new runtime Meter was intended to be a special case requiring no code changes at all but thinking it through I'm pretty sure there is a hole in the current implementation. The runtime Meter is automatically created when anyone creates a MeterListener, and MetricsEventSource creates a MeterListener if any external tool starts a session. MetricsEventSource itself is created the first time any Meter object is created however if a trivial app never created a Meter than nothing would kick off the entire chain of dependencies. I feel a little embarrassed not to have spotted that earlier and I assume all testing so far has always happened to have at least one other Meter/MeterListener in the process so it went unnoticed. Regardless I think that is the current state. My initial thought just now would be to add something to our startup path that always creates an instance of MetricsEventSource similar to how we create the RuntimeEventSource but happy to get other opinions.

[jkotas]

Yes, the current accidental initialization pattern for the runtime meters is non-unintuitive and it is likely going to produce user feedback.

add something to our startup path that always creates an instance of MetricsEventSource similar to how we create the RuntimeEventSource but happy to get other opinions.

It means loading System.Diagnostics.DiagnosticSource and whatever it happens to depend on even in a trivial hello world app. That is not going to be good for the footprint and startup perf of the minimal .NET app.

I guess it may be ok if this can be disabled via a feature flag. We would want to disable it by default for app models and form factors that care about footprint or where the meters are not really useful, similar to how we disable event source for those: mobile, nativeaot hello world, etc.

Also, if we are going to initialize System.Diagnostics.DiagnosticSource runtime meters by default, should we consider taking the breaking change to delete the initialization of the equivalent runtime counters in CoreLib at the same time to reduce the impact?

[noahfalk]

It means loading System.Diagnostics.DiagnosticSource and whatever it happens to depend on even in a trivial hello world app. That is not going to be good for the footprint and startup perf of the minimal .NET app.

It would mean loading the DiagnosticSource assembly and creating an instance of MetricsEventSource. MetricsEventSource itself doesn't have any explicit dependency on Meter, MeterListener etc. Those types ideally shouldn't load or JIT until dotnet-counters runs and establishes a connection which invokes OnEventCommand and causes MetricsEventSource to create its handler: https://source.dot.net/#System.Diagnostics.DiagnosticSource/System/Diagnostics/Metrics/MetricsEventSource.cs,81.
I'm guessing right now the CommandHandler being a field of MetricsEventSource would trigger more type loading to occur, but that dependency should be breakable using a trivial interface.

If the assembly load cost was still too impactful on its own we could investigate creating a deferred activation mechanism for well-known EventSources. I'm imagining something where we preregister the EventSource name with the OS/EventPipe + some kind of callback that would create the EventSource object on-demand if the external tools request to listen to it. Its definitely more engineering effort so it wouldn't be my first choice, but I'm not seeing anything obviously problematic so far if we had to.

I guess it may be ok if this can be disabled via a feature flag.

I was expecting this would respect the EventSource feature flag (ie we'd check the flag before attempting the assembly load). I assume that would cover scenarios where the startup/size-on-disk requirements are most stringent.

Also, if we are going to initialize System.Diagnostics.DiagnosticSource runtime meters by default, should we consider taking the breaking change to delete the initialization of the equivalent runtime counters in CoreLib at the same time to reduce the impact?

I think if we remove RuntimeEventSource we are going to cause customers a lot of aggravation and add friction to the .NET 8 -> .NET 9 upgrade process. I don't rule it out but it feels like an option of last resort to me.