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

feat(sdk-metrics)!: export factory functions over classes #4932

Open
wants to merge 8 commits into
base: next
Choose a base branch
from

Conversation

pichlermarc
Copy link
Member

@pichlermarc pichlermarc commented Aug 20, 2024

Currently types of private properties end up being part of the public interface, which causes breaking changes left, right and center. Using a factory function we can only expose what we want the user to use, this enables us to make larger changes to SDK internals without breaking users.

Note: This is combined with #4931 as it also eliminates some classes that we previously exported.

Old:

const { MeterProvider, PeriodicExportingMetricReader } = require('@opentelemetry/sdk-metrics');
const { OTLPMetricExporter } =  require('@opentelemetry/exporter-metrics-otlp-proto');

const meterProvider = new MeterProvider({
  readers: [
    new PeriodicExportingMetricReader({
      exporter: new OTLPMetricExporter(),
      exportIntervalMillis: 1000,
    })
  ]
});

New:

const { createMeterProvider, createPeriodicExportingMetricReader } = require('@opentelemetry/sdk-metrics');
const { OTLPMetricExporter } =  require('@opentelemetry/exporter-metrics-otlp-proto');

const meterProvider = createMeterProvider({
  readers: [ 
    createPeriodicExportingMetricReader({
      exporter: new OTLPMetricExporter(),
      exportIntervalMillis: 1000,
    })
  ]
});

For a user it's actually a fairly minimal change but for us it means that we can freely modify the SDK's MeterProvider and all previously unintentionally public referenced and transitively referenced types.

@pichlermarc pichlermarc changed the title [draft] feat(sdk-metrics)!: use factory functions over classes feat(sdk-metrics)!: use factory functions over classes Nov 7, 2024
Copy link

codecov bot commented Nov 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.25%. Comparing base (b057c93) to head (a762455).
Report is 3 commits behind head on next.

Additional details and impacted files
@@           Coverage Diff           @@
##             next    #4932   +/-   ##
=======================================
  Coverage   93.24%   93.25%           
=======================================
  Files         318      318           
  Lines        8231     8239    +8     
  Branches     1651     1651           
=======================================
+ Hits         7675     7683    +8     
  Misses        556      556           
Files with missing lines Coverage Δ
...imental/packages/opentelemetry-sdk-node/src/sdk.ts 95.48% <100.00%> (ø)
...es/shim-opencensus/src/OpenCensusMetricProducer.ts 100.00% <ø> (ø)
packages/sdk-metrics/src/MeterProvider.ts 100.00% <100.00%> (ø)
...es/sdk-metrics/src/export/ConsoleMetricExporter.ts 84.61% <100.00%> (+1.28%) ⬆️
...s/sdk-metrics/src/export/InMemoryMetricExporter.ts 92.00% <100.00%> (+0.69%) ⬆️
...etrics/src/export/PeriodicExportingMetricReader.ts 93.22% <100.00%> (+0.23%) ⬆️
packages/sdk-metrics/src/view/View.ts 100.00% <ø> (ø)

@pichlermarc pichlermarc changed the title feat(sdk-metrics)!: use factory functions over classes feat(sdk-metrics)!: export factory functions over classes Nov 8, 2024
@pichlermarc pichlermarc added target:next-major-release This PR targets the next major release (`next` branch) pkg:sdk-metrics labels Nov 8, 2024
@pichlermarc pichlermarc marked this pull request as ready for review November 8, 2024 12:56
@pichlermarc pichlermarc requested a review from a team as a code owner November 8, 2024 12:56
Copy link
Contributor

@trentm trentm left a comment

Choose a reason for hiding this comment

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

Looking good.

Here are some more hits of the old usage in docs and examples to apply to your branch:
https://gist.github.com/trentm/889d78ad727c53aae1e10244768cd9a6

packages/sdk-metrics/src/MeterProvider.ts Outdated Show resolved Hide resolved
@pichlermarc pichlermarc force-pushed the feat/use-factory-function-sdk-metrics branch from cdd22fe to e2a36a9 Compare November 12, 2024 10:31
@pichlermarc
Copy link
Member Author

Here are some more hits of the old usage in docs and examples to apply to your branch: https://gist.github.com/trentm/889d78ad727c53aae1e10244768cd9a6

Thanks for pointing that out - there were actually a few more things that needed changing from the previous PRs I made for that package, I applied them all in e2a36a9 and a762455

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:sdk-metrics target:next-major-release This PR targets the next major release (`next` branch)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants