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.

Sign up for GitHub

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

Jump to bottom

feat(sdk-metrics)!: remove MeterProvider.addMetricReader() in favor of constructor option #4419

Merged
merged 4 commits into from
Jan 24, 2024
Merged

feat(sdk-metrics)!: remove MeterProvider.addMetricReader() in favor of constructor option #4419

merged 4 commits into from
Jan 24, 2024

Conversation

pichlermarc
Copy link
Member

@pichlermarc pichlermarc commented Jan 16, 2024 •
edited
Loading

Which problem is this PR solving?

Currently a MetricReader is added to a MeterProvider via addMetricReader(). As Meters (and through it instruments) can be created before adding a MetricReader. This gives the false impression that the order of calls does not matter. This PR removes the addMetricReader() method in favor of a constructor option as it is the case for most SDKs:

  • see Java
    • does not allow adding a MetricReader after building the MeterProvider
  • see Python
    • does not allow adding a MetricReader after building the MeterProvider
  • see C++
    • the comment there seems to indicate that they are in a similar situation as we are (only meter data created after adding the MetricReader is respected.
  • see .NET [1] [2]
    • similar to Java, they have a builder that allows it, but adding it after building the MeterProvider is not possible

An alternative would be to allow adding a MetricReader like in the current interface, but re-configuring all instruments, their aggregations, assocaited views and their temporality. This however, is not required by the spec and would drastically increase complexity of an already very complex Metrics SDK.

Fixes #4112
References #open-telemetry/opentelemetry-js-contrib#1900

Short description of the changes

Type of change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

  • Adapted existing tests

@codecov Codecov
Copy link

codecov bot commented Jan 16, 2024 •
edited
Loading

Codecov Report

Merging #4419 (0be7c90) into next (543f0b4) will increase coverage by 0.03%.
Report is 8 commits behind head on next.
The diff coverage is 96.96%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             next    #4419      +/-   ##
==========================================
+ Coverage   92.24%   92.27%   +0.03%     
==========================================
  Files         332      332              
  Lines        9437     9436       -1     
  Branches     1999     1996       -3     
==========================================
+ Hits         8705     8707       +2     
+ Misses        732      729       -3
Files Coverage Δ
...erimental/packages/api-logs/src/types/LogRecord.ts 100.00% <ø> (ø)
...porter-metrics-otlp-grpc/src/OTLPMetricExporter.ts 93.75% <100.00%> (+0.20%) ⬆️
...-otlp-http/src/platform/node/OTLPMetricExporter.ts 100.00% <100.00%> (ø)
...orter-metrics-otlp-proto/src/OTLPMetricExporter.ts 100.00% <100.00%> (ø)
...ges/opentelemetry-instrumentation-http/src/http.ts 94.75% <100.00%> (+0.83%) ⬆️
...imental/packages/opentelemetry-sdk-node/src/sdk.ts 92.80% <100.00%> (+0.05%) ⬆️
...ackages/otlp-exporter-base/src/OTLPExporterBase.ts 95.23% <100.00%> (ø)
...mental/packages/otlp-transformer/src/logs/index.ts 100.00% <ø> (ø)
experimental/packages/sdk-logs/src/LogRecord.ts 98.05% <100.00%> (+0.07%) ⬆️
...ckages/opentelemetry-core/src/common/attributes.ts 93.18% <100.00%> (ø)
... and 6 more

... and 1 file with indirect coverage changes

@pichlermarc pichlermarc marked this pull request as ready for review January 16, 2024 12:18
@pichlermarc pichlermarc requested a review from a team January 16, 2024 12:18
legendecas
legendecas reviewed Jan 18, 2024
View reviewed changes
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.

Can we deprecate MeterProvider.addMetricReader instead while adding the new constructor option? In this way, we can avoid breaking all users of the sdk-metrics and allow them to migrate to the new option.

@pichlermarc
Copy link
Member Author

pichlermarc commented Jan 18, 2024 •
edited
Loading

Can we deprecate MeterProvider.addMetricReader instead while adding the new constructor option? In this way, we can avoid breaking all users of the sdk-metrics and allow them to migrate to the new option.

Yes. This PR is targeting the next branch so it's already intended for 2.0 (that's why it's breaking).
Once this is merged, I'll open a PR to backport the constructor option and tests to main and deprecate MeterProvider.addMetricReader there 🙂

@pichlermarc pichlermarc mentioned this pull request Jan 18, 2024
Merged
2 tasks
@pichlermarc
Copy link
Member Author

@legendecas, I opened another PR that targets main just now: #4427

@legendecas
Copy link
Member

Oh, I see. Thank you for clarifying. I didn't notice the target branch 😅

@pichlermarc pichlermarc merged commit a9af247 into open-telemetry:next Jan 24, 2024
19 checks passed
@pichlermarc pichlermarc deleted the feat/remove-addmetricreader branch January 24, 2024 13:38
@david-luna david-luna mentioned this pull request May 13, 2024
Open
@pichlermarc pichlermarc mentioned this pull request Nov 11, 2024
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@legendecas legendecas legendecas approved these changes

@david-luna david-luna david-luna approved these changes

Assignees
No one assigned
Labels
pkg:sdk-metrics
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pichlermarc @legendecas @david-luna