feat(sdk-metrics)!: drop deprecated type field on MetricDescriptor
#5291
Conversation
| !name.endsWith('_total') && | ||
| data.dataPointType === DataPointType.SUM && | ||
| data.isMonotonic | ||
| ) { |
There was a problem hiding this comment.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5291 +/- ##
==========================================
+ Coverage 94.62% 94.63% +0.01%
==========================================
Files 323 323
Lines 8068 8071 +3
Branches 1637 1640 +3
==========================================
+ Hits 7634 7638 +4
+ Misses 434 433 -1
|
| 'metric_observable_counter{key1="attributeValue1"} 20', | ||
| '# HELP metric_observable_counter_total a test description', | ||
| '# TYPE metric_observable_counter_total counter', | ||
| 'metric_observable_counter_total{key1="attributeValue1"} 20', | ||
| '', | ||
| ]); | ||
| }); |
There was a problem hiding this comment.
@pichlermarc I think this is the natural conclusion of your proposed check right? (or– did I misunderstood your comment and flipped the check?)
There was a problem hiding this comment.
yes that's correct. 🙂
I think the previous behavior was actually incorrect and should've included the suffix.
| @@ -374,6 +380,9 @@ describe('Instruments', () => { | |||
| unit: '', | |||
| type: InstrumentType.HISTOGRAM, | |||
| valueType: ValueType.INT, | |||
| advice: { | |||
| explicitBucketBoundaries: [1, 9, 100], | |||
| }, | |||
There was a problem hiding this comment.
The previous test should have this but left it out due to it not being enforced by the type. Internally that went to assertPartialDeepStrictEqual and that was only checking everything appears here (expected) is present and === on the actual, but not the other way around.
chancancode
force-pushed
the
rm-metric-descriptor-type
branch
from
bf47aa2 to
cbfee05
Compare
|
@pichlermarc rebased, and friendly ping :) |
| 'metric_observable_counter{key1="attributeValue1"} 20', | ||
| '# HELP metric_observable_counter_total a test description', | ||
| '# TYPE metric_observable_counter_total counter', | ||
| 'metric_observable_counter_total{key1="attributeValue1"} 20', | ||
| '', | ||
| ]); | ||
| }); |
There was a problem hiding this comment.
yes that's correct. 🙂
I think the previous behavior was actually incorrect and should've included the suffix.
Move the field to the internal `InstrumentDescriptor` type and keep it for internal use. Fixes open-telemetry#3439
chancancode
force-pushed
the
rm-metric-descriptor-type
branch
from
7be3799 to
c297da7
Compare
|
@pichlermarc applied the suggestion and rebased away the conflict |
Which problem is this PR solving?
Drop deprecated
typefield onMetricDescriptorFixes #3439
Fixes #5266 (review) (@pichlermarc)
Short description of the changes
Move the field to the internal
InstrumentDescriptortype and keep it for internal use.See also: #5266 (comment)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
npm compilenpm lintnpm test:*Checklist:
addedupdated