-
Notifications
You must be signed in to change notification settings - Fork 825
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
feat(sdk-metrics)!: drop deprecated type
field on MetricDescriptor
#5291
base: main
Are you sure you want to change the base?
Conversation
Move the field to the internal `InstrumentDescriptor` type and keep it for internal use. Fixes open-telemetry#3439
@@ -7,6 +7,8 @@ All notable changes to experimental packages in this project will be documented | |||
|
|||
### :boom: Breaking Change | |||
|
|||
* feat(exporter-prometheus)!, feat(shim-opencensus)!: drop support for the deprecated `type` field on `MetricDescriptor` [#5291](https://github.com/open-telemetry/opentelemetry-js/pull/5291) |
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.
Not exactly sure what to say here, there is probably more to say, since, as the test shown this change can have noticeable impact
!name.endsWith('_total') && | ||
data.dataPointType === DataPointType.SUM && | ||
data.isMonotonic | ||
) { |
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.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5291 +/- ##
=======================================
Coverage 94.62% 94.62%
=======================================
Files 323 323
Lines 8068 8071 +3
Branches 1637 1640 +3
=======================================
+ Hits 7634 7637 +3
Misses 434 434
|
'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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pichlermarc I think this is the natural conclusion of your proposed check right? (or– did I misunderstood your comment and flipped the check?)
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Which problem is this PR solving?
Drop deprecated
type
field onMetricDescriptor
Fixes #3439
Fixes #5266 (review) (@pichlermarc)
Short description of the changes
Move the field to the internal
InstrumentDescriptor
type 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 compile
npm lint
npm test:*
Checklist:
addedupdated