Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update metrics to support OpenTelemetry specification #5120

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Update metrics to support OpenTelemetry specification #5120

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
be0ce46
Update metrics to support OTel.
CodeBlanch Dec 18, 2024
d72b4d3
Tweaks.
CodeBlanch Jan 6, 2025
5b35732
Tweaks.
CodeBlanch Jan 6, 2025
afe280e
Code review.
CodeBlanch Jan 6, 2025
bd792d9
Code review.
CodeBlanch Jan 6, 2025
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
20 changes: 20 additions & 0 deletions src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/CounterMetadata.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,37 @@ namespace Microsoft.Diagnostics.Monitoring.EventPipe
public class CounterMetadata
{
public CounterMetadata(string providerName, string counterName, string meterTags, string instrumentTags, string scopeHash)
: this(
providerName,
providerVersion: null,
counterName,
counterUnit: null,
counterDescription: null,
meterTags,
instrumentTags,
scopeHash)
{
}

public CounterMetadata(string providerName, string providerVersion, string counterName, string counterUnit, string counterDescription, string meterTags, string instrumentTags, string scopeHash)
{
ProviderName = providerName;
ProviderVersion = providerVersion;
CounterName = counterName;
CounterUnit = counterUnit;
CounterDescription = counterDescription;
MeterTags = meterTags;
InstrumentTags = instrumentTags;
ScopeHash = scopeHash;
}

public CounterMetadata() { }

public string ProviderName { get; private set; }
public string ProviderVersion { get; private set; }
public string CounterName { get; private set; }
public string CounterUnit { get; private set; }
public string CounterDescription { get; private set; }
public string MeterTags { get; private set; }
public string InstrumentTags { get; private set; }
public string ScopeHash { get; private set; }
Expand Down
109 changes: 98 additions & 11 deletions src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/CounterPayload.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,9 @@ protected CounterPayload(DateTime timestamp,

public string Unit { get; }

public double Value { get; }
public double Value { get; protected set; }

public DateTime Timestamp { get; }
public DateTime Timestamp { get; protected set; }

public float Interval { get; }

Expand Down Expand Up @@ -86,7 +86,38 @@ protected MeterPayload(DateTime timestamp,
{
}

public override bool IsMeter => true;
public sealed override bool IsMeter => true;
}

internal abstract class MeterInstrumentDeltaMeasurementPayload : MeterPayload
{
protected MeterInstrumentDeltaMeasurementPayload(
DateTime timestamp,
CounterMetadata counterMetadata,
string displayName,
string unit,
double value,
CounterType counterType,
string valueTags,
EventType eventType)
: base(timestamp, counterMetadata, displayName, unit, value, counterType, valueTags, eventType)
{
}

public abstract bool SupportsDelta { get; }
Copy link
Member

Choose a reason for hiding this comment

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

This feels confusing that the type is named "MeterInstrumentDeltaMeasurementPayload" and yet it might have SupportsDelta return false. Does the "Delta" in the type name have a different meaning than the Delta in the property name?

Copy link
Author

Choose a reason for hiding this comment

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

I moved it on to MeterPayload with a default of false. For instruments where delta is supported they now return true.


public virtual void Combine(MeterInstrumentDeltaMeasurementPayload other)
Copy link
Member

Choose a reason for hiding this comment

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

Right now all these payload objects are simple immutable objects that represent data which came across the wire and it would be nice to preserve that immutability. Rather than modify the objects in place perhaps you could track the mutating state in a couple variables you define together with the for loop iterating over them? That seems simpler and clearer to me than trying to reuse the payload object also as an accumulator for different subsets of the properties.

Copy link
Author

Choose a reason for hiding this comment

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

I dropped the combine concept. I do like the immutability of these objects too. They can mutate DisplayName it seems but still 😄 Hard to know exactly how the aggregation will work. I took a look at my PoC. At the moment it passes a payload type. So it will have to allocate a new one with the final value. But that may not be the end of the world, or something else could be possible, or we could do more here. But figured OK to get the good pieces in so it can keep moving/evolving.

{
if (other.Timestamp > Timestamp)
{
Timestamp = other.Timestamp;
}
}
}

internal interface IRatePayload
{
double Rate { get; }
}

internal sealed class GaugePayload : MeterPayload
Expand All @@ -100,14 +131,29 @@ public GaugePayload(CounterMetadata counterMetadata, string displayName, string
}
}

internal class UpDownCounterPayload : MeterPayload
internal class UpDownCounterPayload : MeterInstrumentDeltaMeasurementPayload, IRatePayload
{
public UpDownCounterPayload(CounterMetadata counterMetadata, string displayName, string displayUnits, string valueTags, double value, DateTime timestamp) :
public UpDownCounterPayload(CounterMetadata counterMetadata, string displayName, string displayUnits, string valueTags, double rate, double value, DateTime timestamp) :
base(timestamp, counterMetadata, displayName, displayUnits, value, CounterType.Metric, valueTags, EventType.UpDownCounter)
{
// In case these properties are not provided, set them to appropriate values.
string counterName = string.IsNullOrEmpty(displayName) ? counterMetadata.CounterName : displayName;
DisplayName = !string.IsNullOrEmpty(displayUnits) ? $"{counterName} ({displayUnits})" : counterName;
Rate = rate;
}

public double Rate { get; private set; }

public override bool SupportsDelta => false;

public override void Combine(MeterInstrumentDeltaMeasurementPayload other)
{
if (other is UpDownCounterPayload upDownCounterPayload)
{
Rate += upDownCounterPayload.Rate;
}

base.Combine(other);
}
}

Expand All @@ -131,9 +177,8 @@ public CounterEndedPayload(CounterMetadata counterMetadata, DateTime timestamp)
/// This gets generated for Counter instruments from Meters. This is used for pre-.NET 8 versions of MetricsEventSource that only reported rate and not absolute value,
/// or for any tools that haven't opted into using RateAndValuePayload in the CounterConfiguration settings.
/// </summary>
internal sealed class RatePayload : MeterPayload
internal sealed class RatePayload : MeterInstrumentDeltaMeasurementPayload, IRatePayload
{

public RatePayload(CounterMetadata counterMetadata, string displayName, string displayUnits, string valueTags, double rate, double intervalSecs, DateTime timestamp) :
base(timestamp, counterMetadata, displayName, displayUnits, rate, CounterType.Rate, valueTags, EventType.Rate)
{
Expand All @@ -143,13 +188,24 @@ public RatePayload(CounterMetadata counterMetadata, string displayName, string d
string intervalName = intervalSecs.ToString() + " sec";
DisplayName = $"{counterName} ({unitsName} / {intervalName})";
}

public double Rate => Value;

public override bool SupportsDelta => true;

public override void Combine(MeterInstrumentDeltaMeasurementPayload other)
{
Value += other.Value;

base.Combine(other);
}
}

/// <summary>
/// Starting in .NET 8, MetricsEventSource reports counters with both absolute value and rate. If enabled in the CounterConfiguration and the new value field is present
/// then this payload will be created rather than the older RatePayload. Unlike RatePayload, this one treats the absolute value as the primary statistic.
/// </summary>
internal sealed class CounterRateAndValuePayload : MeterPayload
internal sealed class CounterRateAndValuePayload : MeterInstrumentDeltaMeasurementPayload, IRatePayload
{
public CounterRateAndValuePayload(CounterMetadata counterMetadata, string displayName, string displayUnits, string valueTags, double rate, double value, DateTime timestamp) :
base(timestamp, counterMetadata, displayName, displayUnits, value, CounterType.Metric, valueTags, EventType.Rate)
Expand All @@ -162,6 +218,18 @@ public CounterRateAndValuePayload(CounterMetadata counterMetadata, string displa
}

public double Rate { get; private set; }

public override bool SupportsDelta => true;

public override void Combine(MeterInstrumentDeltaMeasurementPayload other)
{
if (other is CounterRateAndValuePayload counterRateAndValuePayload)
{
Rate += counterRateAndValuePayload.Rate;
}

base.Combine(other);
}
}

internal record struct Quantile(double Percentage, double Value);
Expand All @@ -182,17 +250,36 @@ public PercentilePayload(CounterMetadata counterMetadata, string displayName, st
// dotnet-counters created a separate payload for each quantile (multiple payloads per TraceEvent).
// AggregatePercentilePayload allows dotnet-monitor to construct a PercentilePayload for individual quantiles
// like dotnet-counters, while still keeping the quantiles together as a unit.
internal sealed class AggregatePercentilePayload : MeterPayload
internal sealed class AggregatePercentilePayload : MeterInstrumentDeltaMeasurementPayload
{
public AggregatePercentilePayload(CounterMetadata counterMetadata, string displayName, string displayUnits, string valueTags, IEnumerable<Quantile> quantiles, DateTime timestamp) :
public AggregatePercentilePayload(CounterMetadata counterMetadata, string displayName, string displayUnits, string valueTags, int count, double sum, IEnumerable<Quantile> quantiles, DateTime timestamp) :
base(timestamp, counterMetadata, displayName, displayUnits, 0.0, CounterType.Metric, valueTags, EventType.Histogram)
{
Count = count;
Sum = sum;
//string counterName = string.IsNullOrEmpty(displayName) ? name : displayName;
//DisplayName = !string.IsNullOrEmpty(displayUnits) ? $"{counterName} ({displayUnits})" : counterName;
Quantiles = quantiles.ToArray();
}

public Quantile[] Quantiles { get; }
public int Count { get; private set; }

public double Sum { get; private set; }

public Quantile[] Quantiles { get; private set; }

public override bool SupportsDelta => true;
Copy link
Member

Choose a reason for hiding this comment

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

Does doing delta aggregation make sense given that the quantiles can't be combined? I see that you are only doing it for Count and Sum and those do work functionally, so this is more of a broader scenario question if it makes sense to offer this partial information for Histograms vs leaving them out for now and telling people they have to update to a newer version of .NET to make that part of the scenario work.

Copy link
Author

Choose a reason for hiding this comment

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

The whole combine concept is gone for now. I left count/sum on here though. Talking with @alanwest backends like NR can do nice things with just sum/count so I felt it was still worth exposing the data to interested listeners 🤷


public override void Combine(MeterInstrumentDeltaMeasurementPayload other)
{
if (other is AggregatePercentilePayload aggregatePercentilePayload)
{
Count += aggregatePercentilePayload.Count;
Sum += aggregatePercentilePayload.Sum;
}

base.Combine(other);
}
}

internal sealed class ErrorPayload : MeterPayload
Expand Down
47 changes: 36 additions & 11 deletions src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/TraceEventExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,16 @@ internal static class TraceEventExtensions
private static Dictionary<int, CounterMetadata> counterMetadataById = new();
private static HashSet<string> inactiveSharedSessions = new(StringComparer.OrdinalIgnoreCase);

private static CounterMetadata AddCounterMetadata(string providerName, string counterName, int? id, string meterTags, string instrumentTags, string scopeHash)
private static CounterMetadata AddCounterMetadata(
string providerName,
string counterName,
int? id,
string meterTags,
string instrumentTags,
string scopeHash,
string providerVersion = null,
string counterUnit = null,
string counterDescription = null)
{
CounterMetadata metadata;
if (id.HasValue && counterMetadataById.TryGetValue(id.Value, out metadata))
Expand All @@ -67,7 +76,7 @@ private static CounterMetadata AddCounterMetadata(string providerName, string co
}

// no pre-existing counter metadata was found, create a new one
metadata = new CounterMetadata(providerName, counterName, meterTags, instrumentTags, scopeHash);
metadata = new CounterMetadata(providerName, providerVersion, counterName, counterUnit, counterDescription, meterTags, instrumentTags, scopeHash);
if (id.HasValue)
{
counterMetadataById.TryAdd(id.Value, metadata);
Expand Down Expand Up @@ -266,21 +275,26 @@ private static void HandleBeginInstrumentReporting(TraceEvent traceEvent, Counte
}

string meterName = (string)traceEvent.PayloadValue(1);
//string meterVersion = (string)obj.PayloadValue(2);
string meterVersion = (string)traceEvent.PayloadValue(2);
string instrumentName = (string)traceEvent.PayloadValue(3);

if (!counterConfiguration.CounterFilter.IsIncluded(meterName, instrumentName))
{
return;
}

string instrumentUnit = null;
string instrumentDescription = null;
string instrumentTags = null;
string meterTags = null;
string meterScopeHash = null;
int? instrumentID = null;

if (traceEvent.Version >= 1)
{
// string instrumentType = (string)traceEvent.PayloadValue(4);
instrumentUnit = (string)traceEvent.PayloadValue(5);
instrumentDescription = (string)traceEvent.PayloadValue(6);
instrumentTags = (string)traceEvent.PayloadValue(7);
meterTags = (string)traceEvent.PayloadValue(8);
meterScopeHash = (string)traceEvent.PayloadValue(9);
Expand All @@ -292,7 +306,18 @@ private static void HandleBeginInstrumentReporting(TraceEvent traceEvent, Counte
// Many different instruments may all share ID zero we don't want to index them by that ID.
instrumentID = (id != 0) ? id : null;
}
payload = new BeginInstrumentReportingPayload(AddCounterMetadata(meterName, instrumentName, instrumentID, meterTags, instrumentTags, meterScopeHash), traceEvent.TimeStamp);
payload = new BeginInstrumentReportingPayload(
AddCounterMetadata(
meterName,
instrumentName,
instrumentID,
meterTags,
instrumentTags,
meterScopeHash,
providerVersion: meterVersion,
counterUnit: instrumentUnit,
counterDescription: instrumentDescription),
traceEvent.TimeStamp);
}

private static void HandleCounterRate(TraceEvent traceEvent, CounterConfiguration counterConfiguration, out ICounterPayload payload)
Expand Down Expand Up @@ -367,7 +392,7 @@ private static void HandleUpDownCounterValue(TraceEvent traceEvent, CounterConfi
string instrumentName = (string)traceEvent.PayloadValue(3);
string unit = (string)traceEvent.PayloadValue(4);
string tags = (string)traceEvent.PayloadValue(5);
//string rateText = (string)traceEvent.PayloadValue(6); // Not currently using rate for UpDownCounters.
string rateText = (string)traceEvent.PayloadValue(6);
string valueText = (string)traceEvent.PayloadValue(7);
int? id = null;
if (traceEvent.Version >= 2)
Expand All @@ -381,11 +406,11 @@ private static void HandleUpDownCounterValue(TraceEvent traceEvent, CounterConfi
}

CounterMetadata metadata = GetCounterMetadata(meterName, instrumentName, id);
if (double.TryParse(valueText, NumberStyles.Number | NumberStyles.Float, CultureInfo.InvariantCulture, out double value))
if (double.TryParse(rateText, NumberStyles.Number | NumberStyles.Float, CultureInfo.InvariantCulture, out double rate)
Copy link
Member

Choose a reason for hiding this comment

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

Is the rate value not being present also tied to counter ending events?

&& double.TryParse(valueText, NumberStyles.Number | NumberStyles.Float, CultureInfo.InvariantCulture, out double value))
{
// UpDownCounter reports the value, not the rate - this is different than how Counter behaves.
payload = new UpDownCounterPayload(metadata, null, unit, tags, value, traceEvent.TimeStamp);

payload = new UpDownCounterPayload(metadata, null, unit, tags, rate, value, traceEvent.TimeStamp);
}
else
{
Expand Down Expand Up @@ -413,8 +438,8 @@ private static void HandleHistogram(TraceEvent obj, CounterConfiguration configu
string unit = (string)obj.PayloadValue(4);
string tags = (string)obj.PayloadValue(5);
string quantilesText = (string)obj.PayloadValue(6);
//int count - unused arg 7
//double sum - unused arg 8
int count = (int)obj.PayloadValue(7);
double sum = (double)obj.PayloadValue(8);
int? id = null;
if (obj.Version >= 2)
{
Expand All @@ -429,7 +454,7 @@ private static void HandleHistogram(TraceEvent obj, CounterConfiguration configu
//Note quantiles can be empty.
IList<Quantile> quantiles = ParseQuantiles(quantilesText);
CounterMetadata metadata = GetCounterMetadata(meterName, instrumentName, id);
payload = new AggregatePercentilePayload(metadata, null, unit, tags, quantiles, obj.TimeStamp);
payload = new AggregatePercentilePayload(metadata, null, unit, tags, count, sum, quantiles, obj.TimeStamp);
}

private static void HandleHistogramLimitReached(TraceEvent obj, CounterConfiguration configuration, out ICounterPayload payload)
Expand Down