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(api-metrics): remove bind/unbind and bound instruments #2559

Merged
merged 5 commits into from
Nov 2, 2021

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Oct 22, 2021

Which problem is this PR solving?

Removes metric.bind/unbind and bound instrument types from api-metrics. This PR didn't change the data flow or functionality of metric base SDK, as it would result in a large refactor, however, the SDK spec is not frozen yet.

Fixes #2545

Short description of the changes

  • Removes Metric.bind, Metric.unbind, Metric.clear in api-metrics.
  • Removes bound instrument types (BoundCounter, BoundHistogram, BoundObservableBase) in api-metrics.
  • Functionality in metric-base-sdk have not been changed, instead:
    • Classes like CounterMetric, BoundCounterMetric both implement api.Counter interface.
    • ObservableMetric types implements api.ObservableBase types. BoundObservableMetric no longer implements any interface exposed by api-metrics.

Type of change

  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

  • exporters like prometheus-exporter have been updated to latest api.
  • metrics-base-sdk tests have been updated to use SDK metric types instead of api-metrics types to not change functionality.

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated

@codecov
Copy link

codecov bot commented Oct 22, 2021

Codecov Report

Merging #2559 (f056409) into main (c307949) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2559   +/-   ##
=======================================
  Coverage   93.07%   93.08%           
=======================================
  Files         140      140           
  Lines        5168     5145   -23     
  Branches     1101     1101           
=======================================
- Hits         4810     4789   -21     
+ Misses        358      356    -2     
Impacted Files Coverage Δ
...ackages/opentelemetry-api-metrics/src/NoopMeter.ts 77.27% <0.00%> (-5.09%) ⬇️
...kages/opentelemetry-sdk-metrics-base/src/Metric.ts 90.24% <0.00%> (ø)
...ages/opentelemetry-api-metrics/src/types/Metric.ts 100.00% <0.00%> (ø)
...ntelemetry-sdk-metrics-base/src/BoundInstrument.ts 100.00% <0.00%> (ø)
...emetry-sdk-metrics-base/src/BatchObserverResult.ts 100.00% <0.00%> (ø)

@legendecas legendecas marked this pull request as ready for review October 25, 2021 07:39
@legendecas legendecas requested a review from a team October 25, 2021 07:39
Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

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

in general looks fine, just wondering why do you need to cast instruments in all places now. This is perhaps something that could / should be fixed

@legendecas
Copy link
Member Author

@obecny because Meter in sdk-metrics returns api.Counter, api.UpDownCounter etc for createCounter, createUpDownCounter respectively. Now since api.Counter doesn't have method bind/unbind (and clear) anymore, we have to case them to implementation type CounterMetric in sdk-metrics to get access to these methods. Or maybe Meter in sdk-metrics can return implementation type CounterMetric and so on, WDYT?

@dyladan
Copy link
Member

dyladan commented Oct 26, 2021

If bind is removed from the API then it should be removed from the SDK too. I don't see a reason to do the casting to retain a method that is being removed. Unless I misunderstood your comment.

@legendecas
Copy link
Member Author

legendecas commented Oct 26, 2021

@dyladan The main goal of this PR is to remove bind/unbind from api-metrics. As stated in the OP, this PR didn't change the data flow or functionality of metric base SDK, as it would result in a large refactor. And spec for SDK is not frozen yet.

@vmarchaud
Copy link
Member

As stated in the OP, this PR didn't change the data flow or functionality of metric base SDK, as it would result in a large refactor. And spec for SDK is not frozen yet.

Isn't the SDK freeze near, does it make sense to just wait for it to do everything in one PR ?

@dyladan
Copy link
Member

dyladan commented Oct 26, 2021

@dyladan The main goal of this PR is to remove bind/unbind from api-metrics. As stated in the OP, this PR didn't change the data flow or functionality of metric base SDK, as it would result in a large refactor. And spec for SDK is not frozen yet.

If the SDK refactor is really that large then it's fine to do just the API for now. The casting in that case is only temporary until we can do a proper refactor of the SDK?

As stated in the OP, this PR didn't change the data flow or functionality of metric base SDK, as it would result in a large refactor. And spec for SDK is not frozen yet.

Isn't the SDK freeze near, does it make sense to just wait for it to do everything in one PR ?

Yes the freeze is very near if it isn't already frozen. I would consider the SDK metrics spec to be essentially done aside from small tweaks based on the last few metrics and spec meetings.

@legendecas
Copy link
Member Author

Isn't the SDK freeze near, does it make sense to just wait for it to do everything in one PR ?

Even SDK is frozen, I would not put the SDK refactor with API change in a single PR. They can be changed separately. And even SDK is not refactored, it is compliant with the latest API (we just won't be able to access the bind/unbind from api-metrics). After api-metrics breaking changes settled down, users of api-metrics won't be affected when we land the SDK refactors. WDYT?

The casting in that case is only temporary until we can do a proper refactor of the SDK?

Yes, when we get rid of bind/unbind in SDK tests, we can remove these type casts.

@vmarchaud
Copy link
Member

vmarchaud commented Oct 27, 2021

Even SDK is frozen, I would not put the SDK refactor with API change in a single PR

Not something blocking on my end, so lgtm

Copy link
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

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

Yes, when we get rid of bind/unbind in SDK tests, we can remove these type casts.

Should we make an issue to track this before merging? Otherwise LGTM

@legendecas
Copy link
Member Author

Should we make an issue to track this before merging

I believe we need to create issues for feature-freeze SDK spec compliance. For now, we only have feature-freeze API spec tracking issues.

@vmarchaud vmarchaud requested a review from obecny November 2, 2021 08:14
@vmarchaud
Copy link
Member

@legendecas They is one conflict to fix before merging it

# Conflicts:
#	experimental/packages/opentelemetry-sdk-metrics-base/test/Meter.test.ts
@dyladan dyladan merged commit 5bb8322 into open-telemetry:main Nov 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove metric instrument bind/unbind
5 participants