Fix CounterGroup timer to use Stopwatch instead of DateTime.UtcNow#127303
Fix CounterGroup timer to use Stopwatch instead of DateTime.UtcNow#127303unsafePtr wants to merge 2 commits intodotnet:mainfrom
Conversation
dotnet-policy-service
Bot
added
the
community-contribution
| _nextPollingTimeStamp = DateTime.UtcNow + new TimeSpan(0, 0, (int)pollingIntervalInSeconds); | ||
| long now = Stopwatch.GetTimestamp(); | ||
| _timeStampSinceCollectionStarted = now; | ||
| _nextPollingTimeStamp = now + (long)(Stopwatch.Frequency * pollingIntervalInSeconds); |
There was a problem hiding this comment.
This is notably a change in behavior, previously it treated 0.9f as 0 (float to integer conversions truncate). Now it will instead track the difference more precisely to the amount represented. Is this change correct/expected?
There was a problem hiding this comment.
A bit above we have following lines passed to EnableTimer
(int)0.9f = 0, so for EventCounterIntervalSec=0.9 we would fire immediately. As it will prdocue garbage on first data point using sub-second intervals.
The existing test was failing before this change
https://github.com/dotnet/runtime/blob/main/src/libraries/System.Diagnostics.Tracing/tests/BasicEventSourceTest/TestEventCounter.cs#L127
There was a problem hiding this comment.
Re-checked the code again. Probably there won't be values less than 1s, but if let's say 1.5 is submitted it will be truncated to 1 with old behaviour. All subsequnt intervals maintain initial interval of 1.5. One data point is not statistically significant, but if we can fix it, why not?
There was a problem hiding this comment.
I'm not specifying here whether the change is right or wrong, just that it's something that was likely overlooked and so should be carefully considered and potentially documented as part of the change here.
Even if the first data point was somewhat faulty, changing from "fire immediately" to "wait 900ms to fire" is a substantial change and likely needs weigh in from those on the dotnet/area-system-diagnostics team.
| _timeStampSinceCollectionStarted = now; | ||
| TimeSpan delta = now - _nextPollingTimeStamp; | ||
| delta = _pollingIntervalInMilliseconds > delta.TotalMilliseconds ? TimeSpan.FromMilliseconds(_pollingIntervalInMilliseconds) : delta; | ||
| long intervalTicks = (long)((double)Stopwatch.Frequency * _pollingIntervalInMilliseconds / 1000); |
There was a problem hiding this comment.
This can potentially have quirks for high frequency rates (although such frequencies are unexpected/unlikely) and the conversion to double in particular seems unnecessary here since double->long just truncates, which is how integer division already works.
I'm not sure if you're rather wanting to insert an explicit Round operation (0.6 -> 1 instead of 0.6 -> 0) or if this rather should just be Frequency * (_pollingIntervalInMilliseconds / 1000) which avoids the extra cost/complexity.
There was a problem hiding this comment.
I've propbably overlooked, since at other places I am not castign Stopwatch.Frequency to double
| DateTime now = DateTime.UtcNow; | ||
| if (counterGroup._nextPollingTimeStamp < now + new TimeSpan(0, 0, 0, 0, 1)) | ||
| long now = Stopwatch.GetTimestamp(); | ||
| if (counterGroup._nextPollingTimeStamp < now + Stopwatch.Frequency / 1000) |
There was a problem hiding this comment.
It feels like a lot of this complexity could be simplified if kept using TimeSpan instead, which normalizes to 100ns units and allows easier working with concepts like seconds.
Then you really only need to get the start/now timestamps from Stopwatch and use Stopwatch.GetElapsedTime` when comparing that to the tracked intervals.
2 participants
Background
DateTime.UtcNowcan jump due to NTP sync, causing elapsed time reported to EventCounter subscribers to be incorrect for that interval — affecting rate calculations like requests/sec in monitoring dashboards. Stopwatch is monotonic and not subject to clock adjustments.CounterGroupis only directly referenced fromDiagnosticCounter.DiagnosticCounteris the base class ofEventCounter,PollingCounter,IncrementingPollingCounter,IncrementingEventCounter, so they all are affected.