Skip to content

Commit

Permalink
Always generate a new metric stream for each view an instrument match…
Browse files Browse the repository at this point in the history 
…es (#3148)
  • Loading branch information
@alanwest
alanwest committed Apr 12, 2022
1 parent 4cabb23 commit 879d3c9
Show file tree
Hide file tree
Showing 8 changed files with 559 additions and 449 deletions.
6 changes: 6 additions & 0 deletions src/OpenTelemetry/CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,12 @@
may be possible in the future.
([#3126](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3126))

* Conformed to the specification to ensure that each view that an instrument
matches results in a new metric stream. With this change it is possible for
views to introduce conflicting metric streams. Any conflicts encountered will
result in a diagnostic log.
([#3148](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3148))

## 1.2.0-rc4

Released 2022-Mar-30
Expand Down
11 changes: 10 additions & 1 deletion src/OpenTelemetry/Metrics/MeterProviderSdk.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -155,14 +155,23 @@ internal sealed class MeterProviderSdk : MeterProvider
// There may be excess space wasted, but it'll eligible for
// GC right after this method.
var metricStreamConfigs = new List<MetricStreamConfiguration>(viewConfigCount);
foreach (var viewConfig in this.viewConfigs)
for (var i = 0; i < viewConfigCount; ++i)
{
var viewConfig = this.viewConfigs[i];
MetricStreamConfiguration metricStreamConfig = null;
try
{
metricStreamConfig = viewConfig(instrument);
// The SDK provides some static MetricStreamConfigurations.
// For example, the Drop configuration. The static ViewId
// should not be changed for these configurations.
if (!metricStreamConfig.ViewId.HasValue)
{
metricStreamConfig.ViewId = i;
}
if (metricStreamConfig is ExplicitBucketHistogramConfiguration
&& instrument.GetType().GetGenericTypeDefinition() != typeof(Histogram<>))
{
Expand Down
56 changes: 19 additions & 37 deletions src/OpenTelemetry/Metrics/MetricReaderExt.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,31 +38,27 @@ public abstract partial class MetricReader

internal Metric AddMetricWithNoViews(Instrument instrument)
{
var meterName = instrument.Meter.Name;
var meterVersion = instrument.Meter.Version;
var metricName = instrument.Name;
var metricStreamName = $"{meterName}.{meterVersion}.{metricName}";
var metricStreamIdentity = new MetricStreamIdentity(instrument.Meter, metricName, instrument.Unit, instrument.Description, instrument.GetType(), null, null);
var metricStreamIdentity = new MetricStreamIdentity(instrument, metricStreamConfiguration: null);
lock (this.instrumentCreationLock)
{
if (this.instrumentIdentityToMetric.TryGetValue(metricStreamIdentity, out var existingMetric))
{
return existingMetric;
}

if (this.metricStreamNames.Contains(metricStreamName))
if (this.metricStreamNames.Contains(metricStreamIdentity.MetricStreamName))
{
OpenTelemetrySdkEventSource.Log.DuplicateMetricInstrument(
metricName,
meterName,
metricStreamIdentity.InstrumentName,
metricStreamIdentity.MeterName,
"Metric instrument has the same name as an existing one but differs by description, unit, or instrument type. Measurements from this instrument will still be exported but may result in conflicts.",
"Either change the name of the instrument or use MeterProviderBuilder.AddView to resolve the conflict.");
}

var index = ++this.metricIndex;
if (index >= this.maxMetricStreams)
{
OpenTelemetrySdkEventSource.Log.MetricInstrumentIgnored(metricName, instrument.Meter.Name, "Maximum allowed Metric streams for the provider exceeded.", "Use MeterProviderBuilder.AddView to drop unused instruments. Or use MeterProviderBuilder.SetMaxMetricStreams to configure MeterProvider to allow higher limit.");
OpenTelemetrySdkEventSource.Log.MetricInstrumentIgnored(metricStreamIdentity.InstrumentName, metricStreamIdentity.MeterName, "Maximum allowed Metric streams for the provider exceeded.", "Use MeterProviderBuilder.AddView to drop unused instruments. Or use MeterProviderBuilder.SetMaxMetricStreams to configure MeterProvider to allow higher limit.");
return null;
}
else
Expand All @@ -78,13 +74,13 @@ internal Metric AddMetricWithNoViews(Instrument instrument)
// Could be improved with separate Event.
// Also the message could call out what Instruments
// and types (eg: int, long etc) are supported.
OpenTelemetrySdkEventSource.Log.MetricInstrumentIgnored(metricName, instrument.Meter.Name, "Unsupported instrument. Details: " + nse.Message, "Switch to a supported instrument type.");
OpenTelemetrySdkEventSource.Log.MetricInstrumentIgnored(metricStreamIdentity.InstrumentName, metricStreamIdentity.MeterName, "Unsupported instrument. Details: " + nse.Message, "Switch to a supported instrument type.");
return null;
}

this.instrumentIdentityToMetric[metricStreamIdentity] = metric;
this.metrics[index] = metric;
this.metricStreamNames.Add(metricStreamName);
this.metricStreamNames.Add(metricStreamIdentity.MetricStreamName);
return metric;
}
}
Expand Down Expand Up @@ -114,21 +110,13 @@ internal List<Metric> AddMetricsListWithViews(Instrument instrument, List<Metric
for (int i = 0; i < maxCountMetricsToBeCreated; i++)
{
var metricStreamConfig = metricStreamConfigs[i];
var meterName = instrument.Meter.Name;
var meterVersion = instrument.Meter.Version;
var metricName = metricStreamConfig?.Name ?? instrument.Name;
var metricStreamName = $"{meterName}.{meterVersion}.{metricName}";
var metricDescription = metricStreamConfig?.Description ?? instrument.Description;
var tagKeysInteresting = metricStreamConfig?.CopiedTagKeys;
var histogramBucketBounds = (metricStreamConfig is ExplicitBucketHistogramConfiguration histogramConfig
&& histogramConfig.CopiedBoundaries != null) ? histogramConfig.CopiedBoundaries : null;
var metricStreamIdentity = new MetricStreamIdentity(instrument.Meter, metricName, instrument.Unit, metricDescription, instrument.GetType(), tagKeysInteresting, histogramBucketBounds);
var metricStreamIdentity = new MetricStreamIdentity(instrument, metricStreamConfig);

if (!MeterProviderBuilderSdk.IsValidInstrumentName(metricName))
if (!MeterProviderBuilderSdk.IsValidInstrumentName(metricStreamIdentity.InstrumentName))
{
OpenTelemetrySdkEventSource.Log.MetricInstrumentIgnored(
metricName,
instrument.Meter.Name,
metricStreamIdentity.InstrumentName,
metricStreamIdentity.MeterName,
"Metric name is invalid.",
"The name must comply with the OpenTelemetry specification.");

Expand All @@ -137,45 +125,39 @@ internal List<Metric> AddMetricsListWithViews(Instrument instrument, List<Metric

if (this.instrumentIdentityToMetric.TryGetValue(metricStreamIdentity, out var existingMetric))
{
// The list of metrics may already contain a matching metric with the same
// identity when a single instrument is selected by multiple views.
if (!metrics.Contains(existingMetric))
{
metrics.Add(existingMetric);
}

metrics.Add(existingMetric);
continue;
}

if (this.metricStreamNames.Contains(metricStreamName))
if (this.metricStreamNames.Contains(metricStreamIdentity.MetricStreamName))
{
OpenTelemetrySdkEventSource.Log.DuplicateMetricInstrument(
metricName,
meterName,
metricStreamIdentity.InstrumentName,
metricStreamIdentity.MeterName,
"Metric instrument has the same name as an existing one but differs by description, unit, or instrument type. Measurements from this instrument will still be exported but may result in conflicts.",
"Either change the name of the instrument or use MeterProviderBuilder.AddView to resolve the conflict.");
}

if (metricStreamConfig == MetricStreamConfiguration.Drop)
{
OpenTelemetrySdkEventSource.Log.MetricInstrumentIgnored(metricName, instrument.Meter.Name, "View configuration asks to drop this instrument.", "Modify view configuration to allow this instrument, if desired.");
OpenTelemetrySdkEventSource.Log.MetricInstrumentIgnored(metricStreamIdentity.InstrumentName, metricStreamIdentity.MeterName, "View configuration asks to drop this instrument.", "Modify view configuration to allow this instrument, if desired.");
continue;
}

var index = ++this.metricIndex;
if (index >= this.maxMetricStreams)
{
OpenTelemetrySdkEventSource.Log.MetricInstrumentIgnored(metricName, instrument.Meter.Name, "Maximum allowed Metric streams for the provider exceeded.", "Use MeterProviderBuilder.AddView to drop unused instruments. Or use MeterProviderBuilder.SetMaxMetricStreams to configure MeterProvider to allow higher limit.");
OpenTelemetrySdkEventSource.Log.MetricInstrumentIgnored(metricStreamIdentity.InstrumentName, metricStreamIdentity.MeterName, "Maximum allowed Metric streams for the provider exceeded.", "Use MeterProviderBuilder.AddView to drop unused instruments. Or use MeterProviderBuilder.SetMaxMetricStreams to configure MeterProvider to allow higher limit.");
}
else
{
Metric metric;
metric = new Metric(metricStreamIdentity, this.Temporality, this.maxMetricPointsPerMetricStream, histogramBucketBounds, tagKeysInteresting);
metric = new Metric(metricStreamIdentity, this.Temporality, this.maxMetricPointsPerMetricStream, metricStreamIdentity.HistogramBucketBounds, metricStreamIdentity.TagKeys);

this.instrumentIdentityToMetric[metricStreamIdentity] = metric;
this.metrics[index] = metric;
metrics.Add(metric);
this.metricStreamNames.Add(metricStreamName);
this.metricStreamNames.Add(metricStreamIdentity.MetricStreamName);
}
}

Expand Down
4 changes: 3 additions & 1 deletion src/OpenTelemetry/Metrics/MetricStreamConfiguration.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ public class MetricStreamConfiguration
/// Note: All metrics for the given instrument will be dropped (not
/// collected).
/// </remarks>
public static MetricStreamConfiguration Drop { get; } = new MetricStreamConfiguration();
public static MetricStreamConfiguration Drop { get; } = new MetricStreamConfiguration { ViewId = -1 };

/// <summary>
/// Gets or sets the optional name of the metric stream.
Expand Down Expand Up @@ -107,6 +107,8 @@ public string[] TagKeys

internal string[] CopiedTagKeys { get; private set; }

internal int? ViewId { get; set; }

// TODO: MetricPoints caps can be configured here on
// a per stream basis, when we add such a capability
// in the future.
Expand Down
44 changes: 17 additions & 27 deletions src/OpenTelemetry/Metrics/MetricStreamIdentity.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,34 +24,18 @@ namespace OpenTelemetry.Metrics
private static readonly StringArrayEqualityComparer StringArrayComparer = new StringArrayEqualityComparer();
private readonly int hashCode;

public MetricStreamIdentity(Meter meter, string instrumentName, string unit, string description, Type instrumentType, string[] tagKeys, double[] histogramBucketBounds)
public MetricStreamIdentity(Instrument instrument, MetricStreamConfiguration metricStreamConfiguration)
{
this.MeterName = meter.Name;
this.MeterVersion = meter.Version ?? string.Empty;
this.InstrumentName = instrumentName;
this.Unit = unit ?? string.Empty;
this.Description = description ?? string.Empty;
this.InstrumentType = instrumentType;

if (tagKeys != null && tagKeys.Length > 0)
{
this.TagKeys = new string[tagKeys.Length];
tagKeys.CopyTo(this.TagKeys, 0);
}
else
{
this.TagKeys = null;
}

if (histogramBucketBounds != null && histogramBucketBounds.Length > 0)
{
this.HistogramBucketBounds = new double[histogramBucketBounds.Length];
histogramBucketBounds.CopyTo(this.HistogramBucketBounds, 0);
}
else
{
this.HistogramBucketBounds = null;
}
this.MeterName = instrument.Meter.Name;
this.MeterVersion = instrument.Meter.Version ?? string.Empty;
this.InstrumentName = metricStreamConfiguration?.Name ?? instrument.Name;
this.Unit = instrument.Unit ?? string.Empty;
this.Description = metricStreamConfiguration?.Description ?? instrument.Description ?? string.Empty;
this.InstrumentType = instrument.GetType();
this.ViewId = metricStreamConfiguration?.ViewId;
this.MetricStreamName = $"{this.MeterName}.{this.MeterVersion}.{this.InstrumentName}";
this.TagKeys = metricStreamConfiguration?.CopiedTagKeys;
this.HistogramBucketBounds = (metricStreamConfiguration as ExplicitBucketHistogramConfiguration)?.CopiedBoundaries;

unchecked
{
Expand All @@ -62,6 +46,7 @@ public MetricStreamIdentity(Meter meter, string instrumentName, string unit, str
hash = (hash * 31) + this.InstrumentName.GetHashCode();
hash = this.Unit == null ? hash : (hash * 31) + this.Unit.GetHashCode();
hash = this.Description == null ? hash : (hash * 31) + this.Description.GetHashCode();
hash = !this.ViewId.HasValue ? hash : (hash * 31) + this.ViewId.Value;
hash = this.TagKeys == null ? hash : (hash * 31) + StringArrayComparer.GetHashCode(this.TagKeys);
if (this.HistogramBucketBounds != null)
{
Expand All @@ -88,6 +73,10 @@ public MetricStreamIdentity(Meter meter, string instrumentName, string unit, str

public Type InstrumentType { get; }

public int? ViewId { get; }

public string MetricStreamName { get; }

public string[] TagKeys { get; }

public double[] HistogramBucketBounds { get; }
Expand All @@ -109,6 +98,7 @@ public bool Equals(MetricStreamIdentity other)
&& this.InstrumentName == other.InstrumentName
&& this.Unit == other.Unit
&& this.Description == other.Description
&& this.ViewId == other.ViewId
&& StringArrayComparer.Equals(this.TagKeys, other.TagKeys)
&& HistogramBoundsEqual(this.HistogramBucketBounds, other.HistogramBucketBounds);
}
Expand Down
Loading

0 comments on commit 879d3c9

Please sign in to comment.