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: observers should not expose bind/unbind method #1001

Merged
merged 1 commit into from
May 6, 2020

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Apr 30, 2020

Which problem is this PR solving?

Short description of the changes

  • @opentelemetry/api: Add new interface UnboundMetric, with bind/unbind method, which extends Metric.
  • @opentelemetry/metrics: Removed problematic setCallback method from BoundObserver. BoundObsever now acts as merely an instrument hold values, should not be exposed as public API.

@codecov-io
Copy link

codecov-io commented Apr 30, 2020

Codecov Report

Merging #1001 into master will increase coverage by 0.04%.
The diff coverage is 91.66%.

@@            Coverage Diff             @@
##           master    #1001      +/-   ##
==========================================
+ Coverage   95.00%   95.04%   +0.04%     
==========================================
  Files         212      212              
  Lines        8813     8806       -7     
  Branches      796      796              
==========================================
- Hits         8373     8370       -3     
+ Misses        440      436       -4     
Impacted Files Coverage Δ
packages/opentelemetry-api/src/metrics/Metric.ts 100.00% <ø> (ø)
...ackages/opentelemetry-metrics/test/Batcher.test.ts 92.59% <ø> (ø)
...ackages/opentelemetry-api/src/metrics/NoopMeter.ts 73.17% <83.33%> (+0.44%) ⬆️
...kages/opentelemetry-metrics/src/BoundInstrument.ts 100.00% <100.00%> (+7.14%) ⬆️
packages/opentelemetry-metrics/src/Meter.ts 96.29% <100.00%> (ø)
packages/opentelemetry-metrics/src/Metric.ts 96.87% <100.00%> (ø)

@legendecas legendecas force-pushed the remove-observer-bind branch 3 times, most recently from 0b4c2c6 to 317db95 Compare April 30, 2020 09:42
@dyladan dyladan added breaking bug Something isn't working labels Apr 30, 2020
clear(): void;
}

export interface UnboundMetric<T> extends Metric {
Copy link
Member

Choose a reason for hiding this comment

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

minor: pls add JSDoc comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated, PTAL :)

packages/opentelemetry-metrics/test/Batcher.test.ts Outdated Show resolved Hide resolved
Copy link
Member

@vmarchaud vmarchaud left a comment

Choose a reason for hiding this comment

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

lgtm

@dyladan dyladan added the Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) label May 6, 2020
@dyladan dyladan merged commit c1bcce8 into open-telemetry:master May 6, 2020
@legendecas legendecas deleted the remove-observer-bind branch May 6, 2020 16:14
dyladan pushed a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove Bound Observer
5 participants