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
[prototype] feat(api): add wrapMeter() for experimental metrics API features #4622
base: main
Are you sure you want to change the base?
[prototype] feat(api): add wrapMeter() for experimental metrics API features #4622
Conversation
b5e13e8
to
9853047
Compare
9853047
to
b630553
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4622 +/- ##
==========================================
+ Coverage 90.77% 92.90% +2.12%
==========================================
Files 90 292 +202
Lines 1930 7919 +5989
Branches 417 1667 +1250
==========================================
+ Hits 1752 7357 +5605
- Misses 178 562 +384
|
I've been fighting webpack for a while now on this problem. I'm pretty sure we're doing something wrong with our entrypoint exports, but I just cannot figure out what it is. Edit: I wonder if that's because we're using karma-webpack and that's using webpack 4. Maybe it does not support it yet?
|
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.
I'm sure there must be a way that is simpler and has less duplication than copying and binding all methods
@@ -26,6 +26,16 @@ | |||
"default": "./build/src/experimental/index.js" | |||
} | |||
}, | |||
"typesVersions": { | |||
"<4.9": { |
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.
I think you want all TS versions.
"<4.9": { | |
"*": { |
In higher versions TS takes exports over this, but only if you have it set up in a specific esm-first way. You still want to use typesversions for commonjs.
constructor(meter: Meter) { | ||
this._meter = meter; | ||
this.addBatchObservableCallback = | ||
meter.addBatchObservableCallback.bind(meter); | ||
this.createCounter = meter.createCounter.bind(meter); | ||
this.createObservableGauge = meter.createObservableGauge.bind(meter); | ||
this.createHistogram = meter.createHistogram.bind(meter); | ||
this.createObservableCounter = meter.createObservableCounter.bind(meter); | ||
this.createObservableUpDownCounter = | ||
meter.createObservableUpDownCounter.bind(meter); | ||
this.createUpDownCounter = meter.createUpDownCounter.bind(meter); | ||
this.removeBatchObservableCallback = | ||
meter.removeBatchObservableCallback.bind(meter); | ||
} | ||
addBatchObservableCallback: Meter['addBatchObservableCallback']; | ||
createCounter: Meter['createCounter']; | ||
createObservableGauge: Meter['createObservableGauge']; | ||
createHistogram: Meter['createHistogram']; | ||
createObservableCounter: Meter['createObservableCounter']; | ||
createUpDownCounter: Meter['createUpDownCounter']; | ||
createObservableUpDownCounter: Meter['createObservableUpDownCounter']; | ||
removeBatchObservableCallback: Meter['removeBatchObservableCallback']; |
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.
This seems like a lot of duplication and large strings. I doubt this is very friendly to minification. Isn't there a way to keep the meter mostly the same and put new methods on it without a whole new class?
Which problem is this PR solving?
This is based on @clintonb's work on adding a
createGauge()
API.We did not have a way to introduce experimental API features for the metrics API. This PR implements a function
wrapMeter
available via@opentelemetry/api/experimental
that allows users to use experimental features on aMeter
.It falls back to a no-op should trying to create the Instrument on the underlying
Meter
fail (if the function is not present, for instance).Type of change
Please delete options that are not relevant.
How Has This Been Tested?