Allow duplicate metric name when meter is different #2634
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2634 +/- ##
==========================================
- Coverage 79.84% 79.84% -0.01%
==========================================
Files 249 249
Lines 8663 8667 +4
==========================================
+ Hits 6917 6920 +3
- Misses 1746 1747 +1
|
Co-authored-by: Reiley Yang <reyang@microsoft.com>
| [InlineData(AggregationTemporality.Cumulative, false)] | ||
| [InlineData(AggregationTemporality.Delta, true)] | ||
| [InlineData(AggregationTemporality.Delta, false)] | ||
| public void StreamNamesDuplicatesAreNotAllowedTest(AggregationTemporality temporality, bool hasView) |
There was a problem hiding this comment.
Rename this to StreamNameDuplicatesAreAllowedWithDifferentMetersTest?
We should also add another unit test to confirm that we cannot add duplicate metric stream names when the instruments created from the same meter.
There was a problem hiding this comment.
This unit test currently tests both
- that duplicate names are not allowed in when the instruments are created from the same meter, and
- that duplicate names are allowed when instruments are from different meters
Are you suggesting breaking this into two tests for clarity? If so, I'm 👍 on that idea.
There was a problem hiding this comment.
We should also add another unit test to confirm that we cannot add duplicate metric stream names when the instruments created from the same meter.
Ohh I just realized that this was already covered before this PR. I guess we could have two different unit tests so that their names match what they are testing.
…hub.com:alanwest/opentelemetry-dotnet into alanwest/allow-same-metric-name-different-meter
|
|
||
| if (!MeterProviderBuilderSdk.IsValidInstrumentName(metricStreamName)) | ||
| if (!MeterProviderBuilderSdk.IsValidInstrumentName(metricName)) |
There was a problem hiding this comment.
Shouldn't this be metricStreamName still? As it is now, it again uses the instrument name, right?
There was a problem hiding this comment.
We do want to use the instrument name as the validation check is only for the instrument name. metricStreamName is now used to ensure the uniqueness of the combination of meter name and instrument name.
There was a problem hiding this comment.
👍, thanks for tackling this so quickly!
Fixes #2627.
Changes
Please provide a brief description of the changes here.
For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes