Support dynamic instance counters in dotnet-counters monitoring

[thargy]

Background and Motivation

In a few of my projects I use counters that are 'instanced', that is a set of unique counters is created per-instance of an object (e.g. per connection, per session, etc). The lifetime of these instance counters is bound to the instance (being created on construction and disposed with the instance), and each counter has a unique name.

For example, I have an FPS counter for each video camera connection, which have names such as "1_FPS", "2_FPS", etc. These connections start and stop over time, with an ever-growing number of counters being seen, but only 2 or 3 active at any one time. The counters are disposed when the connections are disposed. An alternative would have been to have them as a static counter, with unique payloads identifying the instance, but the current tooling would not make it easy to visualise the separate instances.

The problem is that dotnet-counters assumes a static list of counters that only grows. Once a counter is 'seen' it is never removed. Eventually, this leads to an unmanageable list of counters on display, when monitoring, and currently (#1716), will cause a crash.

Proposed Feature

When a counter is disposed it should be removed from display. I can't see where an EventListener can detect the disposal of an EventSource (this seems like an oversight in the design), which is a shame as automatic detection of disposal would be preferable. However, as far as the monitor is concerned that isn't the most important as I propose we rely on a timeout method anyway. A timeout would also cover the scenario of short-lived event sources, ensuring they are displayed for long enough to be noticed.

I propose we add an eventSourceTimeout parameter (in seconds) to dotnet-counters which will remove an event source from display after not receiving any further updates for the specified period. This should probably default to something sensible like 60, with a value of 0 disabling the feature (current implementation).

Usage Examples

In most use cases the default value of 60s would not cause any noticeable impact, as most counters update every second. Where consumers have instance counters that are changing relatively quickly, they could reduce this value to 2s to ensure the display does not become cluttered, e.g:

dotnet-counters monitor -n MyApp --eventSourceTimeout 2 --providers System.Runtime MyInstanceCounters

[thargy]

@sywhang would you be able to point to where the most appropriate place would be to raise a query about detecting EventSource disposal in EventListeners? Although, not necessarily useful here, it seems like a strange omission to the design.

[sywhang]

@thargy we (the .NET diagnostics team) also own EventSource and EventListeners. The appropriate place to discuss them is dotnet/runtime repo though, so feel free to open an issue there and tag myself and @josalem. Thanks!

[sywhang]

@thargy also, thanks for submitting this feature request! I took a closer look at the proposed feature and thought about it for a bit. Here are some initial thoughts I have on your request:

When a counter is disposed it should be removed from display.

The recommended usage pattern for EventSource is a singleton pattern without having to dispose it unless needed. That being said I can see why it would be necessary in some cases.

I propose we add an eventSourceTimeout parameter (in seconds) to dotnet-counters which will remove an event source from display after not receiving any further updates for the specified period.

I am happy to add this to monitor command. A small modification I'd like to make is the name and the default value (see below on why). Rather than using eventSourceTimeout, I think --expire-timeout is a better flag name.

In most use cases the default value of 60s would not cause any noticeable impact, as most counters update every second.

The update interval is configurable and some customers use longer than a second update interval, especially when they are running it over a long period of time to minimize perf impact. If one already has a workflow set up with --refresh-interval being 60, it will break them. I think a safer default option is to just not have it expire at all.

[thargy]

Hi @sywhang,

I have raised dotnet/runtime#45247, to document a proposal for lifetime monitoring of sources and counters in EventListener. Even if that proposal (or something similar) were accepted and implemented, I would still suggest implementing a timeout as

• It allows for very short-lived counters/sources to be persisted long enough to be 'noticed'.
• It supports legacy code that doesn't yet update lifetime correctly.

The recommended usage pattern for EventSource is a singleton pattern without having to dispose it unless needed. That being said I can see why it would be necessary in some cases.

Agreed, however, and perhaps more importantly, the EventSource and EventCounter APIs support a non-singleton usage, and promote a temporary lifespan when they implement IDisposable as such it is possible for a bad-faith actor, or buggy implementation, to create and destroy counters and sources and, as a result, crash attached EventListeners through indefinite memory growth; and similarly, any long-running monitor.

I am happy to add this to monitor command. A small modification I'd like to make is the name and the default value (see below on why). Rather than using eventSourceTimeout, I think --expire-timeout is a better flag name.

Agreed. In fact, the other problem with eventSourceTimeout is technically it should apply to the expiry of individual EventCounters not EventSources, which are effectively a collection of EventCounters and may not be expired even as their counters are added and destroyed dynamically.

The update interval is configurable and some customers use longer than a second update interval, especially when they are running it over a long period of time to minimize perf impact. If one already has a workflow set up with --refresh-interval being 60, it will break them. I think a safer default option is to just not have it expire at all.

An alternative would be to default to the refresh interval, i.e. the counters will disappear on next refresh if not updated. (You may wish to set a minimum, when defaulting, of 1s, to prevent 'thrashing' under high refresh rates where counters are updated, on average, less quickly than the monitor refreshes, but it's not necessary). The disadvantage of defaulting to 0, is the danger of crashing on long-running sessions due to ever-expanding memory/output lines. Removing a counter when it has been disposed (or has 'expired') is not an unreasonable change in behaviour. We have to assume the 'average' user would not use/notice this option, particularly as it won't appear in many tutorials, so it's not unreasonable to default to the option that is most likely to prevent crashes occurring and give predictable behaviour (i.e. counters disappear when they expire).

[sywhang]

I have raised dotnet/runtime#45247, to document a proposal for lifetime monitoring of sources and counters in EventListener. Even if that proposal (or something similar) were accepted and implemented, I would still suggest implementing a timeout

I haven't taken a look at that yet, but will do soon : ) And yes, regardless of what that is, I think having the expire timeout would be a good addition to dotnet-counters.

the EventSource and EventCounter APIs support a non-singleton usage

and promote a temporary lifespan when they implement IDisposable as such it is possible for a bad-faith actor, or buggy implementation, to create and destroy counters and sources

I agree. These APIs way predates my time at Microsoft and my ownership of these APIs so while I am not a huge fan of this and don't quite understand some parts of it either, backcompat prevents me from making substantial changes to these. I'll take a closer look at your proposal regarding EventListeners notification of disposals and we can discuss more there.

We have to assume the 'average' user would not use/notice this option, particularly as it won't appear in many tutorials, so it's not unreasonable to default to the option that is most likely to prevent crashes occurring and give predictable behaviour

Yup - that aligns with my goal when introducing any new features to not cause headaches (or minimize them at the very least) for users not wanting to use them.

The disadvantage of defaulting to 0, is the danger of crashing on long-running sessions due to ever-expanding memory/output lines. Removing a counter when it has been disposed (or has 'expired') is not an unreasonable change in behaviour.

OK, that makes sense. In that case, how about I set the default to be --refresh-interval multiplied by some value (say, 10 * refreshInterval) so that it can change w.r.t the refresh interval being used by the user. If it's not specified, it will default to 10 since default refresh interval is 1.

[thargy]

OK, that makes sense. In that case, how about I set the default to be --refresh-interval multiplied by some value (say, 10 * refreshInterval) so that it can change w.r.t the refresh interval being used by the user. If it's not specified, it will default to 10 since default refresh interval is 1.

I guess that's mostly about preference at this point. I suggested it defaults to the refresh interval as there is no 'magic constant' involved (a multiplier), and in general that results in more expected/anticipated behaviour.

In my proposal, if the refresh interval is 1s (most common as default), then counters will 'disappear' as they are disposed, which is actually a benefit as it will draw attention to the functionality and is easily understandable (i.e. the counters are disposed so they disappear). If a user then wants the counters to persist for longer, so they can notice them, they would be driven to the help where they would find the expire-timeout parameter. The main issue with this proposal is that counters could disappear very quickly (e.g. for very short refresh intervals < 1s), that is why I suggested a lower bound of 1s when defaulting.

In your proposal, counters would not disappear for 10s, which isn't too bad, but is far less noticeable. The issue is when the next most common use case appears, and the refresh interval is set at a minute, now the counters won't disappear for 10 minutes, and so on (1hr->10hr, etc.). This is the primary issue of using a multiplier. Further, the longer a disposed counter continues to be displayed the less tolerant the system is to rapid instancing. The disappearing too quickly issue inherent in my proposal is still present, but only for refresh intervals <100ms, so it is less likely, I would still set a minimum of 1s when defaulting anyway.

An alternative would be to default to refresh interval + 1s (or to be similar to your proposal +9s), to prevent the scaling issue. However, that also suffers from being 'magic' and therefore represents less anticipatable behaviour.

Again, it is preference, but I would find defaulting to the refresh interval to be the most readily understood, and to offer the better resisliance in rapid instancing scenarios.