Skip to content
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

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

pichlermarc
Copy link
Member

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

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.

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

  • Unit tests

Copy link

codecov bot commented Apr 18, 2024

Codecov Report

Attention: Patch coverage is 96.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 92.90%. Comparing base (2610122) to head (b630553).
Report is 16 commits behind head on main.

❗ Current head b630553 differs from pull request most recent head 017a5d5. Consider uploading reports for the commit 017a5d5 to get more accurate results

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     
Files Coverage Δ
api/src/experimental/index.ts 100.00% <100.00%> (ø)
api/src/experimental/metrics/ExperimentalMeter.ts 100.00% <100.00%> (ø)
api/src/metrics/NoopMeter.ts 97.36% <ø> (ø)
api/src/experimental/metrics/Gauge.ts 75.00% <75.00%> (ø)

... and 201 files with indirect coverage changes

@pichlermarc
Copy link
Member Author

pichlermarc commented Apr 18, 2024

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?
Edit 2: No can't be it because we only have webpack 5 installed, I wonder why npm does not compiain as the peer-dependency range of ^4.0.0 for webpack is not matched when installing karma-webpack.
Edit 3: It's the old version of karma-webpack that's the problem, now we have a few choices, all of which are kind of bad/not-easy to do:

  • update to karma-webpack 5.0.1, break tests for all macOS users (not sure if that's actually what would happen, will try to find someone to run the changes from chore: update karma-webpack to v5.0.1, adapt tests #4648 on macOS)
  • switch to @web/test-runner and rollup:
    • con: it's a lot of work
    • con: does not work well in WebStorm as there's no plugin for it, but I'll survive that, no biggie.
  • contribute a fix to karma-webpack

Copy link
Member

@dyladan dyladan left a 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": {
Copy link
Member

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.

Suggested change
"<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.

Comment on lines +81 to +102
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'];
Copy link
Member

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants