PR feedback - #3608
Conversation
noahfalk
left a comment
noahfalk
left a comment
There was a problem hiding this comment.
If you need to get a move on I'm fine as-is, but I did put one suggestion inline. Thanks!
| { | ||
| } | ||
|
|
||
| public MetricSourceConfiguration(float metricIntervalSeconds, IEnumerable<MetricEventPipeProvider> customProviderNames, int maxHistograms = 20, int maxTimeSeries = 1000) { |
There was a problem hiding this comment.
Do you care about having the flexibility to set different time intervals per-provider? This code path gets a bit confusing because we've got a top-level parameter but also every MetricEventPipeProvider is defining its own interval and then some of these input values appear to get ignored.
Could we simplify to MetricSourceConfiguration(float metricIntervalSeconds, IEnumerable<string> meterNames, IEnumerable<string> eventCounterProviderNames, ...)? If we did that we'd have one shared interval and perhaps no need for MetricEventPipeProvider class or MetricType enum?
There was a problem hiding this comment.
I wanted to lay the foundation for per provider intervals. I think the overload you're suggesting makes a lot of sense as a shortcut for simple configuration though with very reasonable defaults.
There was a problem hiding this comment.
Was there a particular motivation for the per-provider intervals? I wasn't aware there was any demand for that kind of functionality so I am anticipating it would go unused even if we did add it.
MetricsEventSource doesn't support per-provider intervals so for the API to accurately reflect the underlying capabilities we'd end up with something asymmetric like MetricSourceConfiguration(float metricsIntervalSecs, IEnumerable<string> meterNames, IEnumerable<MetricEventPipeProvider> eventCounterProviders, ...).
I don't want to hold you up over this. I am OK with the current code as-is or either of those alternative APIs. From what I know so far the option I like best is where we don't support per-provider intervals.
There was a problem hiding this comment.
The request on our side is tracked with dotnet/dotnet-monitor#2870. My intent is to allow a hierarhical interval specification. I.e. you have a global interval, and then can change it for each provider. For meter-based specification, per provider configuration would be ignored. I will tweak this a bit to be clearer.
There was a problem hiding this comment.
dotnet/dotnet-monitor#2870 - Is that request from customer feedback? The person who filed it is another VSDiag person so I can't tell who is being represented :) I might be totally wrong, but my understanding of the space so far is that the overwhelming majority of customers would not care about or use multiple intervals if you add support for it. Just for curiousity and learning sake I'd love to hear if you had feedback to the contrary, but as long as you are confident it will gain you some users go for it.
|
Not sure if this would be part of this PR, but were we planning to act on #3587 (comment)? |
I will make this change but after merging these initial changes with the feature branch. |
3 participants
Feedback for #3587