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

Unify metric API into the one otel/metric package #4018

Merged
merged 10 commits into from
Apr 27, 2023

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Apr 18, 2023

Resolves: #3995

Changes:

  • Flatten metric/instrument into metric
  • Deprecates all previously released types from metric/instrument and the package itself

These changes are proposed as they resolve the following:

  • The metric/instrument package only provides types and functionality called from the metric package
  • The metric API is fractured across two packages currently
    • Users need to import two packages for the single task of instrumenting code with metrics
    • Users need to reference documentation from metric/instrument to understand the full purpose and functionality of metric

This proposal has been discussed before12, why change it now?

Prior considerations were made before the Meter API was updated to be specification compliant. That update and the following cleanup removed the non-compliant instrument provider and the number scoped instrument packages. With those changes the instruments themselves are never meant to be created or used outside of the functionality provided by metric. It is a relic to keep the partition of the metric/instrument package when all of its use is now in metric.

Footnotes

  1. https://github.com/open-telemetry/opentelemetry-go/issues/2653

  2. https://github.com/open-telemetry/opentelemetry-go/issues/2526

@codecov
Copy link

codecov bot commented Apr 18, 2023

Codecov Report

Merging #4018 (4302d01) into main (94f6c4f) will decrease coverage by 0.1%.
The diff coverage is 100.0%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #4018     +/-   ##
=======================================
- Coverage   82.2%   82.2%   -0.1%     
=======================================
  Files        175     175             
  Lines      13065   13065             
=======================================
- Hits       10742   10740      -2     
- Misses      2102    2104      +2     
  Partials     221     221             
Impacted Files Coverage Δ
metric/asyncfloat64.go 100.0% <ø> (ø)
metric/asyncint64.go 100.0% <ø> (ø)
metric/syncfloat64.go 100.0% <ø> (ø)
metric/syncint64.go 100.0% <ø> (ø)
metric/instrument.go 100.0% <100.0%> (ø)
metric/internal/global/instruments.go 60.8% <100.0%> (ø)
metric/internal/global/meter.go 93.8% <100.0%> (ø)
metric/noop/noop.go 100.0% <100.0%> (ø)
sdk/metric/instrument.go 93.0% <100.0%> (ø)
sdk/metric/meter.go 90.2% <100.0%> (ø)

... and 1 file with indirect coverage changes

@MrAlias MrAlias changed the title Flatten sdk/metric/instrument into sdk/metric Unify metric API into the one otel/metric package Apr 18, 2023
@MrAlias MrAlias marked this pull request as ready for review April 18, 2023 20:36
@MrAlias MrAlias added this to the Metric v0.38.0 milestone Apr 20, 2023
@MrAlias MrAlias added pkg:API Related to an API package area:metrics Part of OpenTelemetry Metrics labels Apr 20, 2023
Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

I am aware that my comments are mostly concerns about the OTel Metrics API Spec. However, I do not think that the package users need to know/check exactly what is in the spec.

metric/doc.go Show resolved Hide resolved
metric/doc.go Show resolved Hide resolved
@pellared
Copy link
Member

@seh PTAL 😉

@seh
Copy link
Contributor

seh commented Apr 21, 2023

Do the types and functions that had lived in the sdk/metric/instrument package need to be exported? It's hard to tell from the PR how the files outside this package were relying on it. In some cases it was the options that we discussed during last week's SIG meeting, and continuing to export those is still necessary.

For example, does the specification mandate that we expose types like metric.Float64Observable? Can we shrink the exported surface of this Go module?

@MrAlias
Copy link
Contributor Author

MrAlias commented Apr 21, 2023

Do the types and functions that had lived in the sdk/metric/instrument package need to be exported? It's hard to tell from the PR how the files outside this package were relying on it. In some cases it was the options that we discussed during last week's SIG meeting, and continuing to export those is still necessary.

For example, does the specification mandate that we expose types like metric.Float64Observable? Can we shrink the exported surface of this Go module?

All of the instruments need to be exported as they are returned from their respective creation method of the Meter.

The Float64Observable itself is needed to generally reference all the observable instruments that can be passed to a Float64Callback.

@seh
Copy link
Contributor

seh commented Apr 21, 2023

All of the instruments need to be exported as they are returned from their respective creation method of the Meter.

Got it. This is not the opportunity I was hoping to find.

@MrAlias
Copy link
Contributor Author

MrAlias commented Apr 24, 2023

@Aneurysm9 thoughts?

@Aneurysm9

This comment was marked as resolved.

@MrAlias MrAlias merged commit 15d6ba2 into open-telemetry:main Apr 27, 2023
@MrAlias MrAlias deleted the mv-metric-inst-pkg branch April 27, 2023 18:25
@MrAlias MrAlias mentioned this pull request Apr 27, 2023
Kavindu-Dodan added a commit to open-feature/flagd that referenced this pull request May 10, 2023
## This PR

Upgrade OTEL dependencies. PR fix API deprecations & upgrade to latest
recommendations.

OTEL Go release notes -
https://github.com/open-telemetry/opentelemetry-go/releases/tag/v1.15.0

Note - Refer OTEL Proposal [1] & PR [2] for breaking changes

[1] - open-telemetry/opentelemetry-go#3995
[2] - open-telemetry/opentelemetry-go#4018

Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:metrics Part of OpenTelemetry Metrics pkg:API Related to an API package proposal
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[proposal] Move instrument creation configuration to otel/metric
5 participants