Allow duplicate instrument registration by alanwest · Pull Request #2916 · open-telemetry/opentelemetry-dotnet

alanwest · 2022-02-18T07:15:19Z

See https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#instrument

When more than one Instrument of the same name is created for identical Meters, denoted duplicate instrument registration, the implementation MUST create a valid Instrument in every case.

Duplicate instrument registration

Identical instruments

Instantiating multiple metric instruments with the same name and also identical in all other respects - same type, description, and unit - result in a single metric stream aggregating measurements from all the identical instruments.

For example, measurements from the following instruments will be aggregated together. No warning is produced by the SDK.

var instrument = meter.CreateCounter<long>(name: "my.counter", unit: "seconds", description: "counts seconds");
var duplicateInstrument = meter.CreateCounter<long>("my.counter", "seconds", "counts seconds");

Distinct instruments

Instantiating multiple metric instruments with the same name but differ in some respect - different type, description, or unit - will result in a separate metric stream for each distinct instrument.

For example, measurements from the following instruments will result in three distinct metric streams. The second one differs by unit/description and the third one differs by type. A warning is logged by the SDK in the event of distinct duplicate instrument registrations.

var instrument = meter.CreateCounter<long>(name: "my.counter", unit: "seconds", description: "counts seconds");
var duplicateInstrument = meter.CreateCounter<long>("my.counter", "milliseconds", "counts milliseconds");
var anotherDuplicateInstrument = meter.CreateCounter<double>("my.counter", "seconds", "counts seconds");

…lemetry-dotnet into alanwest/metric-identity

codecov · 2022-02-25T01:50:00Z

Codecov Report

Merging #2916 (012f080) into main (af0c9b4) will increase coverage by 0.02%.
The diff coverage is 95.87%.

@@            Coverage Diff             @@
##             main    #2916      +/-   ##
==========================================
+ Coverage   84.01%   84.03%   +0.02%     
==========================================
  Files         254      255       +1     
  Lines        8950     8996      +46     
==========================================
+ Hits         7519     7560      +41     
- Misses       1431     1436       +5

Impacted Files	Coverage Δ
src/OpenTelemetry/Metrics/InstrumentIdentity.cs	`90.32% <90.32%> (ø)`
src/OpenTelemetry/Metrics/Metric.cs	`94.20% <97.05%> (-0.25%)`	⬇️
...tryProtocol/Implementation/MetricItemExtensions.cs	`93.52% <100.00%> (ø)`
...nTelemetry/Internal/OpenTelemetrySdkEventSource.cs	`72.72% <100.00%> (+0.50%)`	⬆️
src/OpenTelemetry/Metrics/MetricReaderExt.cs	`90.51% <100.00%> (+1.62%)`	⬆️
...ZPages/Implementation/ZPagesExporterEventSource.cs	`56.25% <0.00%> (-6.25%)`	⬇️
...ter.ZPages/Implementation/ZPagesActivityTracker.cs	`97.14% <0.00%> (-2.86%)`	⬇️
src/OpenTelemetry/Metrics/MetricReader.cs	`85.07% <0.00%> (-0.22%)`	⬇️

reyang

LGTM.

src/OpenTelemetry/Metrics/MetricReaderExt.cs

…ntity

cijothomas · 2022-03-03T01:59:58Z

src/OpenTelemetry/Metrics/MetricReaderExt.cs

                    }

-                    if (this.metricStreamNames.Contains(metricStreamName))
+                    if (this.instrumentIdentityToMetric.TryGetValue(instrumentIdentity, out var existingMetric))


we need to add existingMetric to the metrics.Add(existingMetric) as well.

consider example:
instr1 has view, which says produce 2 streams named inst1_a, inst1_b.
instr2 has view, which says produce 2 streams named inst1_a, inst1_c.

instr1 creation will return List{M1,M2}.
instr2 creation should return List {M1,M3} --> this PR currently only returns {M3}.

^ we should add unittest to cover this.

You are correct. Views hurt my mind 🤕. Fixed 2dd0058

And yet, everyone loves a room with a view! 😄

cijothomas

See this comment : https://github.com/open-telemetry/opentelemetry-dotnet/pull/2916/files#r818252181

I think we are missing a case, when Views are involved.

utpilla · 2022-03-03T02:17:50Z

src/OpenTelemetry/Metrics/MetricReaderExt.cs

                {
-                    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, meterName, "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.");


Should we update the message? The user doesn't really need to resolve any conflict here, right?

The reason for this warning is to highlight a duplicate instrument registration by name that is not an identical match - i.e., differs by description, unit, or instrument type.

For example:

var instrument = meter.CreateCounter<double>("MyCounter", unit: "seconds"); var duplicateButNotIdenticalInstrument = meter.CreateCounter<int>("MyCounter", unit: "milliseconds");

Per the spec, SDKs should no longer prevent this, but in practice, the differences in this example imply semantic differences between the two instruments which are likely not intended. Furthermore, the spec indicates that consumers of these two metric streams may do what they see fit including simply dropping one of them. So this is why the spec still requires SDKs to emit diagnostics that can help folks identify potential problems.

I think we can also mention in the log that "SDK will still exporter duplicates". Else it is not clear if SDK is going to drop it or pass it to exporters.

cijothomas · 2022-03-03T15:25:50Z

src/OpenTelemetry/Metrics/MetricReaderExt.cs

    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>();


We need to clear an entry, upon instrument dispose as well.

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

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

alanwest · 2022-03-04T01:46:04Z

src/OpenTelemetry/Metrics/InstrumentIdentity.cs

+            this.InstrumentType = instrumentType;
+        }
+
+        public readonly string MeterName { get; }


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

Fixed 03a245e

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.

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.

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.

Maybe it'd be less of a concern to keep a handle on the meter since InstrumentIdentity is internal?

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.

Okay to park this into an issue and come back to this ? (so that we can merge and do a release.)

…t version

…ntity

alanwest · 2022-03-04T04:00:52Z

src/OpenTelemetry/.publicApi/net461/PublicAPI.Unshipped.txt

-OpenTelemetry.Metrics.Metric.Meter.get -> System.Diagnostics.Metrics.Meter
+OpenTelemetry.Metrics.Metric.MeterName.get -> string
+OpenTelemetry.Metrics.Metric.MeterVersion.get -> string


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.

namespace OpenTelemetry.Api; public struct InstrumentationScope { string Name { get; } string Version { get; } // Eventually string SchemaUrl { get; } }

This is a good improvement. Don't know if we need to introduce new Struct for this.

Need to add this to changelog as this is breaking any existing exporters.

cijothomas

LGTM.
Please modify changelog to warn export authors about breaking change from previous version.
Also a improved log message to indicate SDK is not dropping the metric.

cijothomas · 2022-03-04T18:06:59Z

@reyang @utpilla Could use one more eyes to this PR. (It has changed since previous review.)

(Goal is to include this and do a rc3 release today)

reyang · 2022-03-04T20:46:19Z

src/OpenTelemetry/Metrics/InstrumentIdentity.cs

+        {
+            unchecked
+            {
+                int hash = 17;


Minor: would it help if the hash is calculated and stored during ctor?

Interesting thought. Done.

src/OpenTelemetry/Metrics/Metric.cs

alanwest and others added 9 commits February 17, 2022 23:13

Allow duplicate instrument registration

ac0e766

Merge branch 'main' into alanwest/metric-identity

7fb51d0

Merge branch 'main' into alanwest/metric-identity

27b595a

Merge branch 'main' into alanwest/metric-identity

ceacd5f

Rename MetricIdentity to InstrumentIdentity

d4a45d0

Handle duplicate instrument registration on views code path

0423b57

Test identical instruments with no views

56e5313

Merge branch 'main' into alanwest/metric-identity

024e947

Merge branch 'alanwest/metric-identity' of github.com:alanwest/opente…

134fc1b

…lemetry-dotnet into alanwest/metric-identity

alanwest added 6 commits February 28, 2022 17:26

Allow distinct metric streams with same name

dd19e28

Add more tests

d399bee

Merge branch 'main' into alanwest/metric-identity

445a3da

Update warning message

8062437

Merge branch 'main' into alanwest/metric-identity

37526c0

Fixx spelling

16b9b63

alanwest marked this pull request as ready for review March 1, 2022 20:25

alanwest requested a review from a team March 1, 2022 20:25

reyang approved these changes Mar 2, 2022

View reviewed changes

cijothomas reviewed Mar 2, 2022

View reviewed changes

src/OpenTelemetry/Metrics/MetricReaderExt.cs Outdated Show resolved Hide resolved

cijothomas and others added 4 commits March 1, 2022 20:29

Update src/OpenTelemetry/Metrics/MetricReaderExt.cs

1bfafc6

Update changelog

3642a3a

Merge remote-tracking branch 'upstream/main' into alanwest/metric-ide…

c69ed26

…ntity

Hashtags: For more than just social media

8eec320

cijothomas reviewed Mar 3, 2022

View reviewed changes

cijothomas requested changes Mar 3, 2022

View reviewed changes

utpilla reviewed Mar 3, 2022

View reviewed changes

cijothomas reviewed Mar 3, 2022

View reviewed changes

Fix view use case

2dd0058

alanwest commented Mar 4, 2022

View reviewed changes

alanwest added 4 commits March 3, 2022 18:33

Handle instruments with same name from meters with same name differen…

03a245e

…t version

Clear InstrumentIdenity map when meter is disposed

0ff1d22

Merge remote-tracking branch 'upstream/main' into alanwest/metric-ide…

63bfe85

…ntity

Update public API

e52051d

alanwest commented Mar 4, 2022

View reviewed changes

cijothomas approved these changes Mar 4, 2022

View reviewed changes

alanwest added 2 commits March 4, 2022 11:09

Update changelog

90ced8c

Add additional clarification for duplicate instrument warning

28aa3b3

reyang approved these changes Mar 4, 2022

View reviewed changes

reyang reviewed Mar 4, 2022

View reviewed changes

utpilla reviewed Mar 4, 2022

View reviewed changes

src/OpenTelemetry/Metrics/Metric.cs Outdated Show resolved Hide resolved

utpilla reviewed Mar 4, 2022

View reviewed changes

src/OpenTelemetry/Metrics/Metric.cs Outdated Show resolved Hide resolved

alanwest added 3 commits March 4, 2022 15:52

Private set

247b0d7

Instrument name from InstrumentIdentity

683182e

Compute hash in constructor

ef836a7

utpilla approved these changes Mar 5, 2022

View reviewed changes

Merge branch 'main' into alanwest/metric-identity

012f080

cijothomas merged commit ffffe5c into open-telemetry:main Mar 5, 2022

alanwest deleted the alanwest/metric-identity branch March 5, 2022 01:57

alanwest mentioned this pull request Mar 17, 2022

Fix bug with multiple views mapping to a single metric stream #3006

Merged

Conversation

alanwest commented Feb 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Duplicate instrument registration

Identical instruments

Distinct instruments

Uh oh!

codecov bot commented Feb 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

reyang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cijothomas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

utpilla Mar 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alanwest Mar 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cijothomas left a comment

Choose a reason for hiding this comment

Uh oh!

cijothomas commented Mar 4, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

alanwest commented Feb 18, 2022 •

edited

Loading

codecov bot commented Feb 25, 2022 •

edited

Loading

utpilla Mar 5, 2022 •

edited

Loading

alanwest Mar 4, 2022 •

edited

Loading