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

Always generate a new metric stream for each view an instrument matches #3148

Merged
merged 16 commits into from
Apr 12, 2022
Merged

Always generate a new metric stream for each view an instrument matches #3148

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
dd040d7
Add ViewId to MetricStreamConfiguration
alanwest Apr 6, 2022
6497ef3
Refactor MetricStreamIdentity
alanwest Apr 6, 2022
12e16fd
Add ViewId to MetricStreamIdentity
alanwest Apr 6, 2022
3042f1c
Fix tag keys
alanwest Apr 7, 2022
0fc2cef
Modify tests to match new expectations
alanwest Apr 7, 2022
af66134
Remove gratuitous check. ViewId makes it unnecessary.
alanwest Apr 7, 2022
8cc2971
Move helper test methods to base class
alanwest Apr 7, 2022
276a360
Move view tests to where all the other view tests are
alanwest Apr 7, 2022
da2fe60
Rename some tests
alanwest Apr 7, 2022
8876f20
Add additional tests from #3129
alanwest Apr 7, 2022
274a47f
Fix Drop configuration ViewId from getting overridden
alanwest Apr 7, 2022
7adb711
Merge branch 'main' into alanwest/view-identifier
alanwest Apr 8, 2022
88f7d64
Merge branch 'main' into alanwest/view-identifier
alanwest Apr 11, 2022
3ec7f6e
Do not copy tag keys and histogram bounds. It's already been done.
alanwest Apr 12, 2022
a948465
Update changelog
alanwest Apr 12, 2022
d5b5612
Merge branch 'main' into alanwest/view-identifier
cijothomas Apr 12, 2022
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
6 changes: 6 additions & 0 deletions src/OpenTelemetry/CHANGELOG.md
View file
Open in desktop
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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,14 +155,23 @@ internal MeterProviderSdk(
// 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
Open in desktop
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);
Comment on lines -142 to +128
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 conditional here is no longer required. Its purpose was to "merge" together identical streams resulting from different views. This primary purpose of this PR is to not do this merging.

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
Open in desktop
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
Open in desktop
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