-
Notifications
You must be signed in to change notification settings - Fork 784
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.
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
Allow duplicate instrument registration #2916
Changes from 15 commits
ac0e766
7fb51d0
27b595a
ceacd5f
d4a45d0
0423b57
56e5313
024e947
134fc1b
dd19e28
d399bee
445a3da
8062437
37526c0
16b9b63
1bfafc6
3642a3a
c69ed26
8eec320
2dd0058
03a245e
0ff1d22
63bfe85
e52051d
90ced8c
28aa3b3
247b0d7
683182e
ef836a7
012f080
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,74 @@ | ||
// <copyright file="InstrumentIdentity.cs" company="OpenTelemetry Authors"> | ||
// Copyright The OpenTelemetry Authors | ||
// | ||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||
// you may not use this file except in compliance with the License. | ||
// You may obtain a copy of the License at | ||
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, | ||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
// </copyright> | ||
|
||
using System; | ||
|
||
namespace OpenTelemetry.Metrics | ||
{ | ||
internal readonly struct InstrumentIdentity : IEquatable<InstrumentIdentity> | ||
{ | ||
public InstrumentIdentity(string meterName, string instrumentName, string unit, string description, Type instrumentType) | ||
{ | ||
this.MeterName = meterName; | ||
this.InstrumentName = instrumentName; | ||
this.Unit = unit; | ||
this.Description = description; | ||
this.InstrumentType = instrumentType; | ||
} | ||
|
||
public readonly string MeterName { get; } | ||
|
||
public readonly string InstrumentName { get; } | ||
|
||
public readonly string Unit { get; } | ||
|
||
public readonly string Description { get; } | ||
|
||
public readonly Type InstrumentType { get; } | ||
|
||
public static bool operator ==(InstrumentIdentity metricIdentity1, InstrumentIdentity metricIdentity2) => metricIdentity1.Equals(metricIdentity2); | ||
|
||
public static bool operator !=(InstrumentIdentity metricIdentity1, InstrumentIdentity metricIdentity2) => !metricIdentity1.Equals(metricIdentity2); | ||
|
||
public readonly override bool Equals(object obj) | ||
{ | ||
return obj is InstrumentIdentity other && this.Equals(other); | ||
} | ||
|
||
public bool Equals(InstrumentIdentity other) | ||
{ | ||
return this.InstrumentType == other.InstrumentType | ||
&& this.MeterName == other.MeterName | ||
&& this.InstrumentName == other.InstrumentName | ||
&& this.Unit == other.Unit | ||
&& this.Description == other.Description; | ||
} | ||
|
||
public readonly override int GetHashCode() | ||
{ | ||
unchecked | ||
{ | ||
int hash = 17; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: would it help if the hash is calculated and stored during ctor? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting thought. Done. |
||
hash = (hash * 31) + this.InstrumentType.GetHashCode(); | ||
hash = (hash * 31) + this.MeterName.GetHashCode(); | ||
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(); | ||
return hash; | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,6 +27,7 @@ namespace OpenTelemetry.Metrics | |
public abstract partial class MetricReader | ||
{ | ||
private readonly HashSet<string> metricStreamNames = new HashSet<string>(StringComparer.OrdinalIgnoreCase); | ||
private readonly Dictionary<InstrumentIdentity, Metric> instrumentIdentityToMetric = new Dictionary<InstrumentIdentity, Metric>(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to clear an entry, upon instrument dispose as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Turns out there's a bit more to just freeing up an entry in this dictionary. I hadn't fully followed the dispose path, so I didn't notice that when an instrument is disposed the metric is freed up. Now that multiple instruments can modify a single metric we need to free up the metric only when no more instruments reference it. Working on this... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah ok, my initial understanding was incorrect. Instruments don't get disposed, meters do, so I think is less complicated than I had thought... 0ff1d22 |
||
private readonly object instrumentCreationLock = new object(); | ||
private int maxMetricStreams; | ||
private int maxMetricPointsPerMetricStream; | ||
|
@@ -39,12 +40,17 @@ internal Metric AddMetricWithNoViews(Instrument instrument) | |
var meterName = instrument.Meter.Name; | ||
var metricName = instrument.Name; | ||
var metricStreamName = $"{meterName}.{metricName}"; | ||
var instrumentIdentity = new InstrumentIdentity(meterName, metricName, instrument.Unit, instrument.Description, instrument.GetType()); | ||
lock (this.instrumentCreationLock) | ||
{ | ||
if (this.instrumentIdentityToMetric.TryGetValue(instrumentIdentity, out var existingMetric)) | ||
{ | ||
return existingMetric; | ||
} | ||
|
||
if (this.metricStreamNames.Contains(metricStreamName)) | ||
{ | ||
OpenTelemetrySdkEventSource.Log.MetricInstrumentIgnored(metricName, instrument.Meter.Name, "Metric name conflicting with existing name.", "Either change the name of the instrument or change name using View."); | ||
return null; | ||
OpenTelemetrySdkEventSource.Log.DuplicateMetricInstrument(metricName, instrument.Meter.Name, "Metric instrument has the same name as an existing one but differs by description, unit, or instrument type.", "Either change the name of the instrument or use MeterProviderBuilder.AddView to resolve the conflict."); | ||
cijothomas marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
var index = ++this.metricIndex; | ||
|
@@ -56,6 +62,7 @@ internal Metric AddMetricWithNoViews(Instrument instrument) | |
else | ||
{ | ||
var metric = new Metric(instrument, this.Temporality, metricName, instrument.Description, this.maxMetricPointsPerMetricStream); | ||
this.instrumentIdentityToMetric[instrumentIdentity] = metric; | ||
this.metrics[index] = metric; | ||
this.metricStreamNames.Add(metricStreamName); | ||
return metric; | ||
|
@@ -90,6 +97,8 @@ internal List<Metric> AddMetricsListWithViews(Instrument instrument, List<Metric | |
var meterName = instrument.Meter.Name; | ||
var metricName = metricStreamConfig?.Name ?? instrument.Name; | ||
var metricStreamName = $"{meterName}.{metricName}"; | ||
var metricDescription = metricStreamConfig?.Description ?? instrument.Description; | ||
var instrumentIdentity = new InstrumentIdentity(meterName, metricName, instrument.Unit, metricDescription, instrument.GetType()); | ||
|
||
if (!MeterProviderBuilderSdk.IsValidInstrumentName(metricName)) | ||
{ | ||
|
@@ -102,12 +111,16 @@ internal List<Metric> AddMetricsListWithViews(Instrument instrument, List<Metric | |
continue; | ||
} | ||
|
||
if (this.metricStreamNames.Contains(metricStreamName)) | ||
if (this.instrumentIdentityToMetric.TryGetValue(instrumentIdentity, out var existingMetric)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we need to add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. consider example: instr1 creation will return List{M1,M2}. ^ we should add unittest to cover this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are correct. Views hurt my mind 🤕. Fixed 2dd0058 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And yet, everyone loves a room with a view! 😄 |
||
{ | ||
OpenTelemetrySdkEventSource.Log.MetricInstrumentIgnored(metricName, instrument.Meter.Name, "Metric name conflicting with existing name.", "Either change the name of the instrument or change name using MeterProviderBuilder.AddView."); | ||
continue; | ||
} | ||
|
||
if (this.metricStreamNames.Contains(metricStreamName)) | ||
{ | ||
OpenTelemetrySdkEventSource.Log.DuplicateMetricInstrument(metricName, instrument.Meter.Name, "Metric instrument has the same name as an existing one but differs by description, unit, or instrument type.", "Either change the name of the instrument or use MeterProviderBuilder.AddView to resolve the conflict."); | ||
} | ||
|
||
if (metricStreamConfig?.Aggregation == Aggregation.Drop) | ||
{ | ||
OpenTelemetrySdkEventSource.Log.MetricInstrumentIgnored(metricName, instrument.Meter.Name, "View configuration asks to drop this instrument.", "Modify view configuration to allow this instrument, if desired."); | ||
|
@@ -122,12 +135,12 @@ internal List<Metric> AddMetricsListWithViews(Instrument instrument, List<Metric | |
else | ||
{ | ||
Metric metric; | ||
var metricDescription = metricStreamConfig?.Description ?? instrument.Description; | ||
string[] tagKeysInteresting = metricStreamConfig?.TagKeys; | ||
double[] histogramBucketBounds = (metricStreamConfig is ExplicitBucketHistogramConfiguration histogramConfig | ||
&& histogramConfig.Boundaries != null) ? histogramConfig.Boundaries : null; | ||
metric = new Metric(instrument, this.Temporality, metricName, metricDescription, this.maxMetricPointsPerMetricStream, histogramBucketBounds, tagKeysInteresting); | ||
|
||
this.instrumentIdentityToMetric[instrumentIdentity] = metric; | ||
this.metrics[index] = metric; | ||
metrics.Add(metric); | ||
this.metricStreamNames.Add(metricStreamName); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not quite right. The entire meter - that is, name, version, and schema url (when we eventually support it) should be a component of the identity. Fixing this...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed 03a245e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be better to just have
Meter
as the onlypublic
property in that case? We would only need to update theEquals()
check like below:This way if schema url gets added to
Meter
later on, we don't have to add a newpublic
property dedicated just for that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is an
internal
struct
, it wouldn't matter as much if we have to add more properties later on, but I think we could just useMeter
here as it provides the best encapsulation for everythingMeter
related.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean keep a handle on the
Meter
itself? Same concern here #2916 (comment) would apply.Meter
might be disposed so safer to not keep a handle on it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it'd be less of a concern to keep a handle on the meter since
InstrumentIdentity
is internal?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was suggesting to have a handle to
Meter
itself as all of this is happening in theInstrumentPublished
callback so theMeter
would not be disposed in this path. But I overlooked the fact that in case of dictionary lookup collisions, theEquals
check might fail as the dictionary might still have entries to instruments whoseMeter
s are disposed. This would only work if we can ensure that the dictionary would never have any instrument whoseMeter
is disposed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay to park this into an issue and come back to this ? (so that we can merge and do a release.)