Add Support For Multiplexing System.Diagnostics.Metrics for Dotnet Monitor and Dotnet Counters

[kkeirstead]

This PR corresponds to dotnet/runtime#86504 from the runtime repo to support multiplexing for System.Diagnostics.Metrics. This change converts dotnet monitor and dotnet counters to use a shared session, assuming the MaxHistograms, MaxTimeSeries, and IntervalSeconds are all the same. The code is in a functional state for anyone who wants to interact with it locally; however, there may still be some bugs (I haven't done extensive testing yet).

I'll call attention to specific areas where I'm looking for feedback via comments.

@wiktork @davmason @noahfalk

[kkeirstead]

@davmason Any changes you'd like to see for this one?

[davmason]

With this update we will only ever use the SHARED session ID, is that a valid ID on previous runtimes that don't have shared session support? We can still run against 6 and 7

[kkeirstead]

With this update we will only ever use the SHARED session ID, is that a valid ID on previous runtimes that don't have shared session support? We can still run against 6 and 7

Good point - while it will work, it does create an issue where the MultipleSessionNotSupportedError is disregarded (assuming both sessions use "SHARED" as the session ID). Is there a pattern I should follow for having this behavior for 8+, but still create a GUID as the session ID for 6/7?

[davmason]

I don't think we have any existing code that checks the runtime version, generally either the command is available or it isn't. But the DiagnosticsClient has a GetRuntimeInfo that will return the runtime version that you could use to determine if it supports multiplexing or not

[wiktork]

Approved with a few comments.

[jander-msft]

This is assuming the runtime is coreclr; is there an affordance for mono? Not sure if there is a way to detect that from the perspective of an out-of-proc diagnostic tool.

[kkeirstead]

@noahfalk Would you be able to weigh in on this or point me in the right direction to investigate this further?

[davmason]

Mono uses the same System.Private.CoreLib as coreclr, unless we specifically did something to exclude mono from using the metrics stuff I would expect it to be on mono too

[noahfalk]

Is version refering to the assembly version of System.Diagnostics.DiagnosticSource.dll? Assuming it is that then there should be no difference between Mono, CLR, or any other runtime because they are all sharing the same S.D.DiagnosticSource assembly that has the same version.

[jander-msft]

Is version refering to the assembly version of System.Diagnostics.DiagnosticSource.dll? Assuming it is that then there should be no difference between Mono, CLR, or any other runtime because they are all sharing the same S.D.DiagnosticSource assembly that has the same version.

From an out-or-proc perspective, we can't get the assembly version, especially not in multi-container environments. This version seems to be the runtime version, which would be 8.0.0 for CoreCLR but that may not be true for Mono.

[jander-msft]

For dotnet-monitor does that mean the same version check is just happening in a different place in the code, or it actually changes how we'd do the check? I was more looking at would the check produce a useful result rather than where the check occurs. If dotnet-monitor doesn't get the new behavior for .NET 6.0 apps running 8.0 DiagnosticSource I'm not going to lose sleep over it.

I'm not sure we have much of an option here for .NET Monitor. We would need some way of interrogating the app folder in the file system in order to find that particular assembly and get its version. And consider that against the runtime version of in is less than 8. This would require a new diagnostic command that would get us the assembly information from the app folder.

[noahfalk]

Yeah, I'm not aware you have a good alternative. Long term we could of course add one, but also long term it won't matter because all the builds that had an assembly specific version check would also have DiagnosticSource 8.0.

[kkeirstead]

@jander-msft I made the change to remove the version logic from MetricSourceConfiguration - let me know if that's what you wanted.

It sounds like on the dotnet-monitor side of things we've settled on checking that the runtime version >= 8, but the dotnet counters logic is still up in the air. If there's concerns about the granularity/correctness of the current logic that will require more investigation, Justin and I were chatting yesterday about temporarily disabling the SHARED session in dotnet-counters as part of this PR. If we go that route, I could either pull all the new logic out of CounterMonitor to revert it to its existing state, or I could leave it in and we never toggle the SHARED switch (this would be leaving unused code behind, but if the expectation is that this would be enabled in the near future, everything would already be rigged up correctly). I've left it as-is for now, but just let me know if there's any action you want me to take and I'll push another update.

[noahfalk]

I'm happy to have @davmason make the call.

[davmason]

For the mono question, from my research it looks like the mono version will be 8.0 (match the coreclr version).

For the dotnet-counters logic, I think matching dotnet-monitor is fine and we don't need special logic.

[kkeirstead]

@noahfalk @davmason Let me know if you want any more changes - otherwise would one of you be able to merge this in?