Diagnostics: dotnet-counter: invalid display name building for non Core CLR IncrementingPollingCounter

[chrisnas]

When displaying custom counters based on IncrementingPollingCounter, the display name is invalid

This does not happen for well known Core CLR counters such as gc count

This is due to the fact that a CounterProfile is created for each well known counter
https://github.com/dotnet/diagnostics/blob/master/src/Tools/dotnet-counters/KnownData.cs#L29.

The ConsoleWriter is using a cache based on these CounterProfile (https://github.com/dotnet/diagnostics/blob/master/src/Tools/dotnet-counters/ConsoleWriter.cs#L148) to compute the display name.

If there is no such hard-coded profile, the ConsoleWriter is calling GetDisplay on the counter payload (https://github.com/dotnet/diagnostics/blob/master/src/Tools/dotnet-counters/ConsoleWriter.cs#L152) .
In the case of IncrementingCounterPayload, GetDisplay builds a string with two elements of the payload:
return $"{m_DisplayName} / {m_DisplayRateTimeScale}";
(https://github.com/dotnet/diagnostics/blob/master/src/Tools/dotnet-counters/CounterPayload.cs#L73)
You can verify that the problem would be the same with Core CLR counters definition without the cache in the debugger by skipping the if test:

The display rate scale is built based on the _DisplayRateTimeScale_ payload field:
m_DisplayRateTimeScale = TimeSpan.Parse(payloadFields["DisplayRateTimeScale"].ToString()).ToString("%s' sec'");
(https://github.com/dotnet/diagnostics/blob/master/src/Tools/dotnet-counters/CounterPayload.cs#L58)

This code expects a string following the TimeSpan format such as hh:mm:ss. This is what is provided by the RuntimeEventSource class when the counters are defined (https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/src/System/Diagnostics/Eventing/RuntimeEventSource.cs#L59)
_gen0GCCounter = _gen0GCCounter ?? new IncrementingPollingCounter("gen-0-gc-count", this, () => GC.CollectionCount(0)) { DisplayName = "Gen 0 GC Count", DisplayRateTimeScale = new TimeSpan(0, 1, 0) };

In the case of a custom IncrementingPollingCounter counter, the same constructor is used and _DisplayRateTimeScale_ property must be set (if not, the code to compute m_DisplayRateTimeScale with throw an exception because payloadFields["DisplayRateTimeScale"] will return null). The postfix will then be "/ xx sec" with xx the number of second passed in the counter constructor.

This bug highlight a larger problem: what could be the meaning of the "/ xx sec" rate?
Since the dotnet-counters user decides what will be the refresh frequency, the current counters implementation in System.Private.CoreLib does not take it into account + it can't be defined at the time the counters are constructed. However, the CounterGroup receives the refresh frequency in OnEventSourceCommand so it could update its counters at that time.

[dorki]

Also, when creating counter, DisplayName is not mandatory, and this is why ConsoleWriter handle this by checking string.IsNullOrEmpty(displayName) and then using CounterPayload.GetName.

But in IncrementingCounterPayload this will never happen since CounterPayload.GetDisplayName will always concatenate the "/ rate":

This also creates invalid display name :(

[Expecho]

Also, when creating counter, DisplayName is not mandatory, and this is why ConsoleWriter handle this by checking string.IsNullOrEmpty(displayName) and then using CounterPayload.GetName.
But in IncrementingCounterPayload this will never happen since CounterPayload.GetDisplayName will always concatenate the "/ rate":

This also creates invalid display name :(

This is solved by #505

[noahfalk]

@sywhang / @Expecho / @chrisnas - There was a mention above that #505 resolved the behavior. Is there a consensus that the bug can be closed now or there are still portions of the issue that need to be addressed? Thanks!

[chrisnas]

I'll test it ASAP

[chrisnas]

Sounds good to me.
I've checked with custom counters (https://github.com/chrisnas/ClrEvents/blob/master/Counters/CountersWebApp/Counters/RequestCountersEventSource.cs) and if I'm not setting DisplayRateTimeScale, I get the expected output in dotnet-counters with a 3 seconds refresh interval
.

[noahfalk]

Excellent, thanks for verifying @chrisnas !