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

fix(host-metrics): widen MeterProvider type restriction in BaseMetrics class #2428

Merged

Conversation

brianphillips
Copy link
Contributor

Instead of requiring the MeterProvider class implementation as exported by @opentelemetry/sdk-metrics, change to simply require the MeterProvider interface as exported by @opentelemtry/api. This makes HostMetrics easier to initialize with the meter provider returned by metrics.getMeterProvider()

Which problem is this PR solving?

Initializing the HostMetrics object with a meterProvider as returned by metrics.getMeterProvider() files a type check:

import { metrics } from "@opentelemetry/api";
import { HostMetrics } from "@opentelemetry/host-metrics";

const hostMetrics = new HostMetrics({ meterProvider: metrics.getMeterProvider() }); // <-- fails with TS2739

The compile-time error is:

Type 'MeterProvider' is missing the following properties from type 'MeterProvider': _sharedState, _shutdown, addMetricReader, shutdown, forceFlush [2739]

Short description of the changes

Instead of requiring the MeterProvider class implementation as exported by @opentelemetry/sdk-metrics, I've changed the BaseMetrics abstract class to simply require the MeterProvider interface as exported by @opentelemtry/api. This matches the existing behavior where, if no meterProvider is specified, it uses metrics.getMeterProvider() as the default (but emits a warning).

Instead of requiring the `MeterProvider` class implementation as
exported by `@opentelemetry/sdk-metrics`, change to simply require the
`MeterProvider` interface as exported by `@opentelemtry/api`. This makes
`HostMetrics` easier to initialize with the meter provider returned by
`metrics.getMeterProvider()`
Copy link

codecov bot commented Sep 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.71%. Comparing base (dfb2dff) to head (0407afb).
Report is 239 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2428      +/-   ##
==========================================
- Coverage   90.97%   90.71%   -0.26%     
==========================================
  Files         146      156      +10     
  Lines        7492     7671     +179     
  Branches     1502     1578      +76     
==========================================
+ Hits         6816     6959     +143     
- Misses        676      712      +36     
Files with missing lines Coverage Δ
...ages/opentelemetry-host-metrics/src/BaseMetrics.ts 100.00% <100.00%> (ø)

... and 76 files with indirect coverage changes

Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep that's better - thanks 👍
Note: waiting for @legendecas approval before merging (component owner) 🙂

@pichlermarc
Copy link
Member

yep that's better - thanks 👍 Note: waiting for @legendecas approval before merging (component owner) 🙂

(actually @legendecas is out of office for a bit so I'll just merge this - the change seems straight-forward enough) 🙂

@pichlermarc pichlermarc enabled auto-merge (squash) September 12, 2024 12:57
Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@legendecas
Copy link
Member

legendecas commented Sep 12, 2024

Does the CLA need to be signed, or is the CLA bot not working?

@brianphillips
Copy link
Contributor Author

brianphillips commented Sep 12, 2024

Does the CLA need to be signed, or is the CLA bot not working?

@legendecas - I (my company) has already signed the CLA and the commit that I originally pushed shows that the status was successful. I'm not sure why merging main into this PR isn't showing a ✔️

@pichlermarc pichlermarc merged commit cb89486 into open-telemetry:main Sep 12, 2024
4 checks passed
@brianphillips brianphillips deleted the host-metrics-meter-provider branch September 12, 2024 18:49
@dyladan dyladan mentioned this pull request Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants