-
Notifications
You must be signed in to change notification settings - Fork 790
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 26 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,80 @@ | ||
// <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; | ||
using System.Diagnostics.Metrics; | ||
|
||
namespace OpenTelemetry.Metrics | ||
{ | ||
internal readonly struct InstrumentIdentity : IEquatable<InstrumentIdentity> | ||
{ | ||
public InstrumentIdentity(Meter meter, string instrumentName, string unit, string description, Type instrumentType) | ||
{ | ||
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; | ||
} | ||
|
||
public readonly string MeterName { get; } | ||
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. 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be better to just have
This way if schema url gets added to 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. Since this is an 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. Do you mean keep a handle on the 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. Maybe it'd be less of a concern to keep a handle on the meter since 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. Yeah, I was suggesting to have a handle to 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. Okay to park this into an issue and come back to this ? (so that we can merge and do a release.) |
||
|
||
public readonly string MeterVersion { 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.MeterVersion == other.MeterVersion | ||
&& 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.MeterVersion.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; | ||
} | ||
} | ||
} | ||
} |
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.
What do folks think about this change? In part I was thinking since Meter can be disposed, maybe it's safer to not keep a handle on it from the metric. Another option would be to create a new struct to encapsulate these fields.
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 a good improvement. Don't know if we need to introduce new Struct for 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.
Need to add this to changelog as this is breaking any existing exporters.