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

kkeirstead · 2023-05-19T15:18:18Z

This PR corresponds to dotnet/diagnostics#3889 from the diagnostics 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

…ic testing in place now. Needs refinement and more testing to guarantee old behavior is still preserved.

…tched params

… shutting down the shared session

… end, we finally disable, and then a new session can start when an enable is received. Also added in test for this.

…ead/Multiplexing_2

...ies/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/AggregationManager.cs

kkeirstead · 2023-05-19T15:26:38Z

...ies/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/MetricsEventSource.cs

@@ -45,6 +45,8 @@ internal sealed class MetricsEventSource : EventSource
    {
        public static readonly MetricsEventSource Log = new();

+        private const string SharedSessionId = "SHARED";


As mentioned in the diagnostics repo, using the previously discussed "SHARED" value to signify these special sessions

kkeirstead · 2023-05-19T15:31:13Z

...ies/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/MetricsEventSource.cs

+
+                    string commandSessionId = GetSessionId(command);
+
+                    if ((command.Command == EventCommand.Update


A lot of the actual code here needs to be cleaned up so don't bother too much with that, but here's the main idea:

If we don't currently have a shared session, proceed as we would before

If we have an active shared session, see if we can fuse with it (by checking that the args are consistent) -> if not, we give a MultipleSessionsConfiguredIncorrectlyError. -> if so, we call Update on the _aggregationManager.

We do ref counting for enables/disables - once all SHARED sessions have been disabled, this frees the _aggregationManager, allowing a new session to start.

Due to EventSource limitations I think you will struggle to do refcounting. I believe the only thing you can know for sure is that when IsEnabled() returns false then there are no sessions active. I think @davmason remembers these details better than I though.

(I ignored looking at the code in more detail, as you suggested)

...ies/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/MetricsEventSource.cs

kkeirstead · 2023-05-19T15:34:36Z

src/libraries/System.Diagnostics.DiagnosticSource/tests/MetricEventSourceTests.cs

@@ -16,7 +16,7 @@ namespace System.Diagnostics.Metrics.Tests
    public class MetricEventSourceTests
    {
        ITestOutputHelper _output;
-        const double IntervalSecs = 10;
+        const double IntervalSecs = 4;


I would recommend mostly ignoring the tests file for this draft - I have added a lot of tests around this (all of which pass), but there's a lot of cleanup that needs to happen here since I was adding new tests any time I found a bug (which means there's probably a lot of redundancy).

The tests are now in a more cleaned-up state for anyone who wants to take a look (I still need to change this variable back and remove the commenting for the OuterLoop attribute)

...ies/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/AggregationManager.cs

...ies/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/MetricsEventSource.cs

wiktork · 2023-05-23T16:36:11Z

...ies/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/MetricsEventSource.cs

+            private void IncrementRefCount(string uniqueIdentifier, EventCommandEventArgs command)
+            {
+                // Could be unsafe if UniqueIdentifier protocol isn't followed
+                if (command.Arguments!.TryGetValue("UniqueIdentifier", out string? uniqueIdentifierArg))


Should we just reject shared sessions that do not have UniqueIdentifiers?

I'm not sure that's possible...the way Shared sessions work, every consumer (e.g. dotnet-monitor) with a SHARED session id will listen to the events, unless the UniqueIdentifier (now renamed to ClientId) has a MultipleSessionsConfiguredIncorrectlyError. Without a ClientId, we have no way to communicate to that particular consumer that they shouldn't be allowed to listen to the SHARED session. In theory you could shut down the whole SHARED session, but that doesn't necessarily feel like a great solution either.

...ies/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/MetricsEventSource.cs

...ies/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/AggregationManager.cs

...ies/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/MetricsEventSource.cs

noahfalk · 2023-05-26T23:15:46Z

...ies/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/MetricsEventSource.cs

+                        if (command.Command == EventCommand.Update
+                            || command.Command == EventCommand.Enable)
+                        {
+                            IncrementRefCount(commandSessionId, command);


@davmason - EventSource sends Enable events even when sessions are being disabled right, except for the last one? That makes it impossible to do this kind of ref-counting and why we had to resort to testing IsEnabled() instead for EventCounters as I recall.

Is behavior documented anywhere? I always struggle to remember it, nobody guesses it because its unintuitive and pages where I'd expect to find it don't mention it:
https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.tracing.eventcommand?view=net-7.0
https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.tracing.eventsource.oneventcommand?view=net-7.0#system-diagnostics-tracing-eventsource-oneventcommand(system-diagnostics-tracing-eventcommandeventargs)

I'm leaving this as unresolved for now (I'm not sure if @davmason had a chance to take a look), but assuming the ClientId protocol is followed by all consumers, is there a reason why this can't/shouldn't work? In my own testing the ref counting seems to work properly (I tested with DM metrics, DM live metrics, and Dotnet Counters simultaneously), but I may be lacking some depth in understanding around this issue.

Sorry, I didn't see Noah's comment. The underlying providers (ETW and EventPipe) send an enable command for every disabled session except the one that turns the provider off completely, but EventPipe will translate those to Enables so you don't have to worry about it.

The problem with ref counting is that we can't guarantee an even number of Enables and Disables with EventListeners. An EventListener can send as many Enables as they want and will send at most one Disable.

For EventCounters we just check on every Disable command if the EventSource is active and shut everything down if it is not

noahfalk · 2023-05-26T23:20:14Z

...ies/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/MetricsEventSource.cs

+
+                    string commandSessionId = GetSessionId(command);
+
+                    if ((command.Command == EventCommand.Update


Due to EventSource limitations I think you will struggle to do refcounting. I believe the only thing you can know for sure is that when IsEnabled() returns false then there are no sessions active. I think @davmason remembers these details better than I though.

(I ignored looking at the code in more detail, as you suggested)

...ies/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/MetricsEventSource.cs

...ies/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/AggregationManager.cs

… as basic test for this). Also includes some cleanup and refactoring.

…ead/Multiplexing_2_backup

...ies/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/AggregationManager.cs

wiktork · 2023-06-15T16:24:08Z

...ies/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/AggregationManager.cs

@@ -21,17 +21,17 @@ internal sealed class AggregationManager
        // these fields are modified after construction and accessed on multiple threads, use lock(this) to ensure the data
        // is synchronized
        private readonly List<Predicate<Instrument>> _instrumentConfigFuncs = new();
-        private TimeSpan _collectionPeriod;
+        public TimeSpan CollectionPeriod { get; private set; }


Nit: consider scoping the lock to this property, rather than 'this' + _aggregationManager

…ead/Multiplexing_2_backup

…dded some comments.

kkeirstead · 2023-06-16T20:49:29Z

...ies/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/MetricsEventSource.cs

@@ -217,6 +225,16 @@ public void UpDownCounterRateValuePublished(string sessionId, string meterName,
            WriteEvent(16, sessionId, meterName, meterVersion ?? "", instrumentName, unit ?? "", tags, rate, value);
        }

+        [Event(17, Keywords = Keywords.TimeSeriesValues)]
+#if !NET8_0_OR_GREATER


I'm lacking some context here since this change happened midway through my change, but it appears that builds were failing due to this not being here - let me know if this isn't the correct resolution

This looks good, we made a change in EventSource to get rid of most of these suppression attributes, so after that change it would not be required except on below 8.

…rce being disabled.

kkeirstead · 2023-06-27T19:37:20Z

@davmason or @noahfalk let me know if you have any new feedback on this - just want to make sure this PR keeps moving forward

kkeirstead · 2023-06-27T19:38:32Z

src/libraries/System.Diagnostics.DiagnosticSource/tests/MetricEventSourceTests.cs

@@ -25,7 +25,638 @@ public MetricEventSourceTests(ITestOutputHelper output)
        }

        [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotBrowser))]
-        [OuterLoop("Slow and has lots of console spew")]
+        //[OuterLoop("Slow and has lots of console spew")]


For convenience I've been leaving these commenting as we iterate (and using a reduced IntervalSecs) - I'll revert all of this before merging.

noahfalk · 2023-06-27T20:24:51Z

@davmason or @noahfalk let me know if you have any new feedback on this - just want to make sure this PR keeps moving forward

Thanks for the heads up and don't block on me :) I'm relying on @davmason from our side and David can ask me to look again if he thinks that will be useful.

davmason

The shared session approach looks good to me. I didn't review it line by line. If you need me to look at a specific section of code let me know.

… Monitor` and `Dotnet Counters` (#3889) 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.

kkeirstead added 10 commits May 10, 2023 13:40

Basic multiplexing is working using the SHARED method - only have bas…

bb15261

…ic testing in place now. Needs refinement and more testing to guarantee old behavior is still preserved.

Improvements to testing, some refactoring, added event to report unma…

4e47265

…tched params

Tests still need fixing/updating - resolved issue where a Disable was…

cd76fc5

… shutting down the shared session

Some test cleanup

9d7528a

Added an extra test

c137c3a

Added in ref counting for proper disabling. When all shared listeners…

aa26df8

… end, we finally disable, and then a new session can start when an enable is received. Also added in test for this.

Cleanup

f4ba2f7

Merge branch 'main' of https://github.com/dotnet/runtime into kkeirst…

2619c99

…ead/Multiplexing_2

Fixes a small bug with ref counting

0a5a901

Updated some naming

d363465

ghost added the community-contribution Indicates that the PR has been added by a community member label May 19, 2023

dotnet-issue-labeler bot added the area-System.Diagnostics.Metric label May 19, 2023

kkeirstead mentioned this pull request May 19, 2023

Add Support For Multiplexing System.Diagnostics.Metrics for Dotnet Monitor and Dotnet Counters dotnet/diagnostics#3889

Merged

kkeirstead commented May 19, 2023

View reviewed changes

...ies/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/AggregationManager.cs Show resolved Hide resolved

kkeirstead commented May 19, 2023

View reviewed changes

...ies/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/MetricsEventSource.cs Outdated Show resolved Hide resolved

kkeirstead commented May 19, 2023

View reviewed changes

This was referenced May 19, 2023

Failed USB connection via port 54050, error 61, in tvOS arm64 Release AllSubsets_Mono #82637

Open

Build ios-arm64 Release AllSubsets_Mono failures dotnet/arcade#13625

Closed

wiktork reviewed May 23, 2023

View reviewed changes

...ies/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/AggregationManager.cs Outdated Show resolved Hide resolved

wiktork reviewed May 23, 2023

View reviewed changes

...ies/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/MetricsEventSource.cs Outdated Show resolved Hide resolved

wiktork reviewed May 23, 2023

View reviewed changes

...ies/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/MetricsEventSource.cs Outdated Show resolved Hide resolved

wiktork reviewed May 23, 2023

View reviewed changes

...ies/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/MetricsEventSource.cs Outdated Show resolved Hide resolved

wiktork reviewed May 23, 2023

View reviewed changes

...ies/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/MetricsEventSource.cs Show resolved Hide resolved

wiktork reviewed May 23, 2023

View reviewed changes

Addressing a few nits.

452f306

build-analysis bot mentioned this pull request May 24, 2023

Tracking issue for CI build timeouts #76454

Closed

danmoseley reviewed May 24, 2023

View reviewed changes

...ies/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/MetricsEventSource.cs Outdated Show resolved Hide resolved

noahfalk reviewed May 26, 2023

View reviewed changes

kkeirstead added 8 commits June 6, 2023 15:16

Added in disabling ref counting when ClientId isn't provided (as well…

58aedba

… as basic test for this). Also includes some cleanup and refactoring.

Some refactoring, bulking up testing.

8b6d433

Cleanup, especially for tests.

e6c26a9

Cleanup, especially for tests.

0021369

Small cleanup

c3f746d

Removed stray comment

15ae5bc

Small cleanup

93fa74a

Merge branch 'main' of https://github.com/dotnet/runtime into kkeirst…

516f4a2

…ead/Multiplexing_2_backup

wiktork reviewed Jun 14, 2023

View reviewed changes

...ies/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/AggregationManager.cs Outdated Show resolved Hide resolved

wiktork reviewed Jun 14, 2023

View reviewed changes

...ies/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/AggregationManager.cs Show resolved Hide resolved

wiktork reviewed Jun 15, 2023

View reviewed changes

kkeirstead added 3 commits June 15, 2023 14:24

Merge branch 'main' of https://github.com/dotnet/runtime into kkeirst…

ce8a923

…ead/Multiplexing_2_backup

Merge branch 'main' of https://github.com/dotnet/runtime into kkeirst…

108671f

…ead/Multiplexing_2_backup

Removing instruments after they've stopped collecting measurements, a…

8396b8e

…dded some comments.

kkeirstead marked this pull request as ready for review June 16, 2023 20:20

Added NET8 check to new event

74ff54a

kkeirstead commented Jun 16, 2023

View reviewed changes

kkeirstead added 2 commits June 19, 2023 09:51

Added description for ClientId protocol; added check for the EventSou…

f6c1e47

…rce being disabled.

Changed test IntervalSecs

409b2a8

kkeirstead commented Jun 27, 2023

View reviewed changes

davmason approved these changes Jun 28, 2023

View reviewed changes

kkeirstead added 3 commits June 30, 2023 11:43

Minor adjustments before checking in.

da49819

Re-enabled OuterLoop

0d5b140

Fixed typo.

cc17cb0

wiktork approved these changes Jul 6, 2023

View reviewed changes

davmason merged commit b99a4dc into dotnet:main Jul 10, 2023

ghost locked as resolved and limited conversation to collaborators Aug 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kkeirstead commented May 19, 2023 •

edited

Loading

kkeirstead May 19, 2023

kkeirstead May 19, 2023

noahfalk May 26, 2023

kkeirstead May 19, 2023

kkeirstead Jun 9, 2023

wiktork May 23, 2023

kkeirstead Jun 9, 2023

noahfalk May 26, 2023

kkeirstead Jun 9, 2023

davmason Jun 9, 2023

davmason Jun 9, 2023

noahfalk May 26, 2023

wiktork Jun 15, 2023

kkeirstead Jun 16, 2023

davmason Jun 28, 2023

kkeirstead commented Jun 27, 2023

kkeirstead Jun 27, 2023

noahfalk commented Jun 27, 2023

davmason left a comment


		string commandSessionId = GetSessionId(command);

		if ((command.Command == EventCommand.Update

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

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

Conversation

kkeirstead commented May 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkeirstead commented Jun 27, 2023

Choose a reason for hiding this comment

noahfalk commented Jun 27, 2023

davmason left a comment

Choose a reason for hiding this comment

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

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

kkeirstead commented May 19, 2023 •

edited

Loading