Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -13,70 +13,85 @@

namespace Microsoft.Diagnostics.Monitoring.EventPipe
{
[Flags]
public enum MetricType
{
EventCounter = 0x1,
Meter = 0x2
}

public sealed class MetricEventPipeProvider
{
public string Provider { get; set; }

public float IntervalSeconds { get; set; }

public MetricType Type { get; set; } = MetricType.EventCounter | MetricType.Meter;
}

public sealed class MetricSourceConfiguration : MonitoringSourceConfiguration
{
private readonly IList<EventPipeProvider> _eventPipeProviders;
public string SessionId { get; private set; }

public static readonly string[] DefaultProviders = new[] { SystemRuntimeEventSourceName, MicrosoftAspNetCoreHostingEventSourceName, GrpcAspNetCoreServer };

public MetricSourceConfiguration(float metricIntervalSeconds, IEnumerable<string> customProviderNames)
: this(metricIntervalSeconds, customProviderNames?.Any() == true ? CreateProviders(metricIntervalSeconds, customProviderNames) :
CreateProviders(metricIntervalSeconds, DefaultProviders))
{
RequestRundown = false;
if (customProviderNames == null)
{
}

public MetricSourceConfiguration(float metricIntervalSeconds, IEnumerable<MetricEventPipeProvider> customProviderNames, int maxHistograms = 20, int maxTimeSeries = 1000) {
Copy link
Member

@noahfalk noahfalk Jan 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@noahfalk noahfalk Jan 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (customProviderNames == null) {
throw new ArgumentNullException(nameof(customProviderNames));
}

RequestRundown = false;
MetricIntervalSeconds = metricIntervalSeconds.ToString(CultureInfo.InvariantCulture);

IEnumerable<string> providers = null;
if (customProviderNames.Any())
{
providers = customProviderNames;
}
else
{
providers = new[] { SystemRuntimeEventSourceName, MicrosoftAspNetCoreHostingEventSourceName, GrpcAspNetCoreServer };
}
_eventPipeProviders = customProviderNames.Where(provider => provider.Type.HasFlag(MetricType.EventCounter))
.Select((MetricEventPipeProvider provider) => new EventPipeProvider(provider.Provider,
EventLevel.Informational,
(long)ClrTraceEventParser.Keywords.None,
new Dictionary<string, string>()
{
{ "EventCounterIntervalSec", provider.IntervalSeconds.ToString(CultureInfo.InvariantCulture)}
})).ToList();

_eventPipeProviders = providers.Select((string provider) => new EventPipeProvider(provider,
EventLevel.Informational,
(long)ClrTraceEventParser.Keywords.None,
new Dictionary<string, string>()
{
{ "EventCounterIntervalSec", MetricIntervalSeconds }
})).ToList();
}
IEnumerable<MetricEventPipeProvider> meterProviders = customProviderNames.Where(provider => provider.Type.HasFlag(MetricType.Meter));

public MetricSourceConfiguration(float metricIntervalSeconds, IEnumerable<string> customProviderNames, int maxHistograms, int maxTimeSeries) : this(metricIntervalSeconds, customProviderNames)
{
const long TimeSeriesValues = 0x2;
StringBuilder metrics = new StringBuilder();
foreach (string provider in customProviderNames)
if (meterProviders.Any())
{
if (metrics.Length != 0)
{
metrics.Append(",");
}

metrics.Append(provider);
}
const long TimeSeriesValues = 0x2;
string metrics = string.Join(',', meterProviders.Select(p => p.Provider));

SessionId = Guid.NewGuid().ToString();
SessionId = Guid.NewGuid().ToString();

EventPipeProvider metricsEventSourceProvider =
new EventPipeProvider("System.Diagnostics.Metrics", EventLevel.Informational, TimeSeriesValues,
new Dictionary<string, string>()
{
EventPipeProvider metricsEventSourceProvider =
new EventPipeProvider("System.Diagnostics.Metrics", EventLevel.Informational, TimeSeriesValues,
new Dictionary<string, string>()
{
{ "SessionId", SessionId },
{ "Metrics", metrics.ToString() },
{ "Metrics", metrics },
{ "RefreshInterval", MetricIntervalSeconds.ToString() },
{ "MaxTimeSeries", maxTimeSeries.ToString() },
{ "MaxHistograms", maxHistograms.ToString() }
}
);
}
);

_eventPipeProviders = _eventPipeProviders.Append(metricsEventSourceProvider).ToArray();
_eventPipeProviders = _eventPipeProviders.Append(metricsEventSourceProvider).ToArray();
}
}

private static IEnumerable<MetricEventPipeProvider> CreateProviders(float metricIntervalSeconds, IEnumerable<string> customProviderNames) =>
customProviderNames.Select(provider => new MetricEventPipeProvider {
Provider = provider,
IntervalSeconds = metricIntervalSeconds,
Type = MetricType.EventCounter
});


private string MetricIntervalSeconds { get; }

public override IList<EventPipeProvider> GetProviders() => _eventPipeProviders;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,6 @@ public ErrorPayload(string errorMessage, DateTime timestamp) :
public string ErrorMessage { get; private set; }
}

// If keep this, should probably put it somewhere else
internal enum EventType : int
{
Rate,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using Microsoft.Diagnostics.Tracing;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;

Expand Down Expand Up @@ -39,7 +40,13 @@ public CounterPipeline(DiagnosticsClient client,

protected override MonitoringSourceConfiguration CreateConfiguration()
{
var config = new MetricSourceConfiguration(Settings.CounterIntervalSeconds, _filter.GetProviders(), Settings.MaxHistograms, Settings.MaxTimeSeries);
var config = new MetricSourceConfiguration(Settings.CounterIntervalSeconds, Settings.CounterGroups.Select((EventPipeCounterGroup counterGroup) => new MetricEventPipeProvider
{
Provider = counterGroup.ProviderName,
IntervalSeconds = counterGroup.IntervalSeconds,
Type = (MetricType)counterGroup.Type
}),
Settings.MaxHistograms, Settings.MaxTimeSeries);

_sessionId = config.SessionId;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,21 @@ internal class CounterPipelineSettings : EventSourcePipelineSettings
public int MaxTimeSeries { get; set; }
}

[Flags]
internal enum CounterGroupType
{
EventCounter = 0x1,
Meter = 0x2,
}

internal class EventPipeCounterGroup
{
public string ProviderName { get; set; }

public string[] CounterNames { get; set; }

public CounterGroupType Type { get; set; } = CounterGroupType.EventCounter | CounterGroupType.Meter;

public float IntervalSeconds { get; set; }
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -166,10 +166,7 @@ private static void HandleGauge(TraceEvent obj, CounterFilter filter, string ses
{
// for observable instruments we assume the lack of data is meaningful and remove it from the UI
// this happens when the Gauge callback function throws an exception.

//TODO Can this occur for other meter types?
payload = new CounterEndedPayload(meterName, instrumentName, null, obj.TimeStamp);

}
}

Expand Down Expand Up @@ -200,6 +197,12 @@ private static void HandleCounterRate(TraceEvent traceEvent, CounterFilter filte
{
payload = new RatePayload(meterName, instrumentName, null, unit, tags, rate, filter.IntervalSeconds, traceEvent.TimeStamp);
}
else
{
// for observable instruments we assume the lack of data is meaningful and remove it from the UI
// this happens when the ObservableCounter callback function throws an exception.
payload = new CounterEndedPayload(meterName, instrumentName, null, traceEvent.TimeStamp);
}
}

private static void HandleHistogram(TraceEvent obj, CounterFilter filter, string sessionId, out List<ICounterPayload> payload)
Expand Down
1 change: 0 additions & 1 deletion src/Tools/dotnet-counters/CounterMonitor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,6 @@ private void HandleCounterRate(TraceEvent obj)
CounterPayload payload = new RatePayload(meterName, instrumentName, null, unit, tags, rate, _interval, obj.TimeStamp);
_renderer.CounterPayloadReceived(payload, _pauseCmdSet);
}

}

private void HandleGauge(TraceEvent obj)
Expand Down