Skip to content

Allow duplicate instrument registration #2916

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

Merged
cijothomas merged 30 commits into from
Mar 5, 2022
Merged

Allow duplicate instrument registration #2916

Changes from 1 commit
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
ac0e766
Allow duplicate instrument registration
alanwest Feb 18, 2022
7fb51d0
Merge branch 'main' into alanwest/metric-identity
alanwest Feb 18, 2022
27b595a
Merge branch 'main' into alanwest/metric-identity
cijothomas Feb 23, 2022
ceacd5f
Merge branch 'main' into alanwest/metric-identity
alanwest Feb 23, 2022
d4a45d0
Rename MetricIdentity to InstrumentIdentity
alanwest Feb 23, 2022
0423b57
Handle duplicate instrument registration on views code path
alanwest Feb 25, 2022
56e5313
Test identical instruments with no views
alanwest Feb 25, 2022
024e947
Merge branch 'main' into alanwest/metric-identity
alanwest Feb 25, 2022
134fc1b
Merge branch 'alanwest/metric-identity' of github.com:alanwest/opente…
alanwest Feb 25, 2022
dd19e28
Allow distinct metric streams with same name
alanwest Mar 1, 2022
d399bee
Add more tests
alanwest Mar 1, 2022
445a3da
Merge branch 'main' into alanwest/metric-identity
alanwest Mar 1, 2022
8062437
Update warning message
alanwest Mar 1, 2022
37526c0
Merge branch 'main' into alanwest/metric-identity
alanwest Mar 1, 2022
16b9b63
Fixx spelling
alanwest Mar 1, 2022
1bfafc6
Update src/OpenTelemetry/Metrics/MetricReaderExt.cs
cijothomas Mar 2, 2022
3642a3a
Update changelog
alanwest Mar 3, 2022
c69ed26
Merge remote-tracking branch 'upstream/main' into alanwest/metric-ide…
alanwest Mar 3, 2022
8eec320
Hashtags: For more than just social media
alanwest Mar 3, 2022
2dd0058
Fix view use case
alanwest Mar 3, 2022
03a245e
Handle instruments with same name from meters with same name differen…
alanwest Mar 4, 2022
0ff1d22
Clear InstrumentIdenity map when meter is disposed
alanwest Mar 4, 2022
63bfe85
Merge remote-tracking branch 'upstream/main' into alanwest/metric-ide…
alanwest Mar 4, 2022
e52051d
Update public API
alanwest Mar 4, 2022
90ced8c
Update changelog
alanwest Mar 4, 2022
28aa3b3
Add additional clarification for duplicate instrument warning
alanwest Mar 4, 2022
247b0d7
Private set
alanwest Mar 4, 2022
683182e
Instrument name from InstrumentIdentity
alanwest Mar 4, 2022
ef836a7
Compute hash in constructor
alanwest Mar 5, 2022
012f080
Merge branch 'main' into alanwest/metric-identity
utpilla Mar 5, 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
Prev Previous commit
Next Next commit
Compute hash in constructor
  • Loading branch information
@alanwest
alanwest committed Mar 5, 2022
commit ef836a7f0cdbe6bf5a8c17cd9c0d99e0bd28e998
29 changes: 15 additions & 14 deletions src/OpenTelemetry/Metrics/InstrumentIdentity.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ namespace OpenTelemetry.Metrics
{
internal readonly struct InstrumentIdentity : IEquatable<InstrumentIdentity>
{
private readonly int hashCode;

public InstrumentIdentity(Meter meter, string instrumentName, string unit, string description, Type instrumentType)
{
this.MeterName = meter.Name;
Expand All @@ -29,6 +31,18 @@ public InstrumentIdentity(Meter meter, string instrumentName, string unit, strin
this.Unit = unit ?? string.Empty;
this.Description = description ?? string.Empty;
this.InstrumentType = instrumentType;

unchecked
{
var hash = 17;
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();
this.hashCode = hash;
}
}

public readonly string MeterName { get; }
Copy link
Member Author

@alanwest alanwest Mar 4, 2022

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...

Copy link
Member Author

@alanwest alanwest Mar 4, 2022

Choose a reason for hiding this comment

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

Fixed 03a245e

Copy link
Contributor

@utpilla utpilla Mar 4, 2022

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...

Wouldn't it be better to just have Meter as the only public property in that case? We would only need to update the Equals() check like below:

                ...
                && this.Meter.Name == other.Meter.Name
                && this.Meter.Version == other.Meter.Version
                && this.Meter.SchemaUrl == other.Meter.SchemaUrl 
                ...

This way if schema url gets added to Meter later on, we don't have to add a new public property dedicated just for that.

Copy link
Contributor

@utpilla utpilla Mar 5, 2022

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 use Meter here as it provides the best encapsulation for everything Meter related.

Copy link
Member Author

@alanwest alanwest Mar 5, 2022

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.

Copy link
Member Author

@alanwest alanwest Mar 5, 2022

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?

Copy link
Contributor

@utpilla utpilla Mar 5, 2022 •
edited
Loading

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 the InstrumentPublished callback so the Meter would not be disposed in this path. But I overlooked the fact that in case of dictionary lookup collisions, the Equals check might fail as the dictionary might still have entries to instruments whose Meters are disposed. This would only work if we can ensure that the dictionary would never have any instrument whose Meter is disposed.

Copy link
Member

@cijothomas cijothomas Mar 5, 2022

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.)

Expand Down Expand Up @@ -62,19 +76,6 @@ public bool Equals(InstrumentIdentity other)
&& this.Description == other.Description;
}

public readonly override int GetHashCode()
{
unchecked
{
int hash = 17;
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;
}
}
public readonly override int GetHashCode() => this.hashCode;
}
}