Allow multiple EventCounters sessions and make sure EventListeners always send a Disable command #82970
Conversation
davmason
force-pushed
the
eventcounters_refcount
branch
from
39c3a36
to
15a55df
Compare
davmason
force-pushed
the
eventcounters_refcount
branch
from
15a55df
to
a6fc6ec
Compare
|
Rebasing against main latest to trigger a new run, hopefully without all the timeouts |
…all to disableevents out of the main loop Update EventSource.cs
davmason
force-pushed
the
eventcounters_refcount
branch
from
a6fc6ec
to
c945d04
Compare
davmason
changed the title
davmason
force-pushed
the
eventcounters_refcount
branch
from
664ae9c
to
5b39139
Compare
…runtime into eventcounters_refcount
| if (e.Arguments.TryGetValue("EventCounterIntervalSec", out string? valueStr) && float.TryParse(valueStr, out float value)) | ||
| lock (s_counterGroupLock) // Lock the CounterGroup | ||
| { | ||
| float intervalValue = 1.0f; |
There was a problem hiding this comment.
I might be misunderstanding... because this has a non-zero value by default, isn't this going to automatically initialize counters at 1000ms intervals for the very first enabled event source if it doesn't have EventCounterIntervalSec in its spec? Won't that prevent any real request for counters to ever be observed?
There was a problem hiding this comment.
It's definitely wrong, good catch. As it is we will always enable counters for any Enable command. We would process future Enable commands though so it probably wouldn't impact anyone other than some additional overhead in the process
davmason
force-pushed
the
eventcounters_refcount
branch
from
b68c18a
to
5249e7f
Compare
|
@noahfalk this is ready to review again with the new approach |
| { | ||
| EnableTimer(value); | ||
| // Command is Enable but no EventCounterIntervalSec arg so ignore |
There was a problem hiding this comment.
I think the behavior here is probably the best we can do within the current limits of EventSource, but it still seems a little odd and merits comments. In particular I am comparing these two scenarios:
Scenario A - first the user enables a session with IntervalCounterIntervalSec=1, then they do an ETW update with new filter args "Foo=6" (which presumably gets translated into an Enable)
Scenario B - user enables a session with filter args Foo=6
Both scenarios end with ETW believing the current filter args are "Foo=6", but in A the counters are on whereas in B they are off. We are effectively saying not only does the current state matter, but also the sequence of steps you took to get there matters. In an ideal world where EventSource could enumerate the current filter args of each session we would know that scenario A shouldn't have the counters enabled anymore, but we don't have that info. Given the ambiguity I agree its probably better to guess that they are still on rather than guessing that they are off now.
| { | ||
| lock (s_counterGroupLock) | ||
|
|
||
| if ((e.Command == EventCommand.Disable && !_eventSource.IsEnabled()) || intervalValue <= 0) |
There was a problem hiding this comment.
-
I think a comment here describing why we do it this way and not by ref counting Enable/Disable calls would be very handy knowledge to preserve.
-
The logic with intervalValue still looks fishy to me. Imagine we get the sequence of events:
Enable session 1, Enable session 2, Disable session 2 and none of them specify IntervalCounterSec. It appears that when we process the disable command at that time we would enable the timer at 1sec intervals.
| @@ -2663,6 +2660,9 @@ internal void DoCommand(EventCommandEventArgs commandArgs) | |||
| m_eventSourceEnabled = false; | |||
| } | |||
| } | |||
|
|
|||
| this.OnEventCommand(commandArgs); | |||
There was a problem hiding this comment.
The previous asymmetry in when the callback ran relative to when the IsEnabled() bit updated puzzled me and this certainly seems more intuitive. However its possible the previous behavior was designed to let people send events during the disable callback and now they would be unable to do so. You should probably test it. If we are lucky sending events during the Disable callback already didn't work in which case this re-ordering is probably safe.
There was a problem hiding this comment.
Verified that in the existing code you cannot send events during a Disable command, the event is already turned off
ghost
locked as resolved and limited conversation to collaborators
This PR does two things
Fixes #61353