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

Deprecate the syncint64/syncfloat64/asyncint64/asyncfloat64 packages #3575

Merged
merged 6 commits into from
Jan 10, 2023

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Jan 6, 2023

Flatten the instruments from the go.opentelemetry.io/otel/metric/instrument/{syncint64,syncfloat64,asyncint64,asyncfloat64} packages into go.opentelemetry.io/otel/metric/instrument.

Follow up to #3507 (comment)

Why Flatten?

Our design choice to partition instrument names was made a while ago, why now flatten these packages?

The general reason is our recent API redesign work12 has made this partitioned package structure not fit with naming and code grouping design. It has become a relic of the prior, non-specification-compliant, design.

Match Specification Recommended Names

Fixes #3453

Our old design used the asynchronous and synchronous terms to identify instrument. The specification has specific naming recommendations for asynchronous instruments though. They should use the "Obervable" term instead. This was fixed in on the meter2, but by flattening the package structure the instrument names are also brought into compliance with the specification naming recommendation.

Unify instruments in instrument package

Each of the go.opentelemetry.io/otel/metric/instrument/{syncint64,syncfloat64,asyncint64,asyncfloat64} packages was a wrapper around 3 instrument types each. The go.opentelemetry.io/otel/metric/instrument package provided configuration and common embedded types for the other packages. This mean that the provided instrument functionality was spread across 5 different packages. By flattening into the instrument package all instrument functionality is provided by 1 unified package.

Configuration is flattened, instruments were not

Instrument configuration is already split in the instrument package1 for each instrument type. Having the configuration for an instrument live in a separate package and not having those instruments split the same way in the same package creates an unneeded package boundary. Users looking at documentation about how an instrument is configured or implementing the configuration of an instrument need to look or import separate packages. This goes against the common Go idea of keeping related things close.

Deprecate instead of remove

To prevent adding the go.opentelemetry.io/otel/metric/instrument/{syncint64,syncfloat64,asyncint64,asyncfloat64} packages to our abandoned packages pile, this deprecates the packages and expects them to be removed in the next release.

Footnotes

  1. https://github.com/open-telemetry/opentelemetry-go/pull/3507 2

  2. https://github.com/open-telemetry/opentelemetry-go/pull/3530 2

@MrAlias MrAlias added the area:metrics Part of OpenTelemetry Metrics label Jan 6, 2023
@MrAlias MrAlias added this to the Metric v0.35.0 milestone Jan 6, 2023
@codecov
Copy link

codecov bot commented Jan 6, 2023

Codecov Report

Merging #3575 (dce4998) into main (78a5582) will not change coverage.
The diff coverage is 74.2%.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #3575   +/-   ##
=====================================
  Coverage   78.8%   78.8%           
=====================================
  Files        169     169           
  Lines      12437   12437           
=====================================
  Hits        9811    9811           
  Misses      2417    2417           
  Partials     209     209           
Impacted Files Coverage Δ
metric/instrument/asyncfloat64.go 100.0% <ø> (ø)
metric/instrument/asyncint64.go 100.0% <ø> (ø)
metric/instrument/syncfloat64.go 100.0% <ø> (ø)
metric/instrument/syncint64.go 100.0% <ø> (ø)
sdk/metric/instrument.go 92.8% <ø> (ø)
metric/noop.go 62.1% <50.0%> (ø)
metric/internal/global/instruments.go 54.3% <72.2%> (ø)
metric/internal/global/meter.go 95.2% <100.0%> (ø)
sdk/metric/meter.go 79.3% <100.0%> (ø)
exporters/jaeger/jaeger.go 90.3% <0.0%> (-0.9%) ⬇️
... and 1 more

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
Projects
No open projects
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

Asynchronous Instrument API does not follow OTel Specification recommendation on naming
3 participants