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

Remove the metric/unit package #3775

Closed
MrAlias opened this issue Feb 24, 2023 · 2 comments
Closed

Remove the metric/unit package #3775

MrAlias opened this issue Feb 24, 2023 · 2 comments
Assignees
Labels
area:metrics Part of OpenTelemetry Metrics pkg:API Related to an API package
Milestone

Comments

@MrAlias
Copy link
Contributor

MrAlias commented Feb 24, 2023

The curent metric/unit package is extremely simple:

package unit // import "go.opentelemetry.io/otel/metric/unit"
// Unit is a determinate standard quantity of measurement.
type Unit string
// Units defined by OpenTelemetry.
const (
Dimensionless Unit = "1"
Bytes Unit = "By"
Milliseconds Unit = "ms"
)

This package, specifically the Unit type, is used throughout the metric API, SDK, and exporters. There are two issues with this: the OTel specification states a unit needs to be a string that is not validated by the API or SDK, and the API design of unit is brittle and will not evolve well.

It is proposed here to remove the unit package and replace all use of the Unit type with a string throughout the metric signal.

Specification context

Currently the metric API specification defines the instrument unit as follows.

Instrument unit

The unit is an optional string provided by the author of the Instrument. The API SHOULD treat it as an opaque string.

  • It MUST be case-sensitive (e.g. kb and kB are different units), ASCII string.
  • It can have a maximum length of 63 characters. The number 63 is chosen to allow the unit strings (including the \0 terminator on certain language runtimes) to be stored and compared as fixed size array/struct when performance is critical.

This definition implies that a unit type is based on a string type and should remain unvalidated. The unit package, knowing ultimately a unit needs to be the case-sensitive UCUM code, tries to provide a convenience API for users to create these units. This API has its own flaws (see below), but regardless of this, the unit.Unit is used throughout the API and SDK to represent a unit. This means that if the unit package is updated and it validates unit input the API and SDK would in spirit not be compliant with the specification.

Instead of using the unit.Unit type to represent units for the metric signal, a string should be used instead. This will decouple the unit package from the metric telemetry. Additionally, this will follow the same design of other language implementations 123.

unit API

The unit API couples all UCUM unit types (metric, special, arbitrary) into a single type: Unit. This type is declared over a string. This means if enhanced support for different units or functionality for units is wanted in the future it cannot be added in a backwards compatible manner.

Additionally, the concept of a prefix is tied to a Unit in the current API. This means that units need to be defined for each prefix/unit combination possible. Because of this the package is left with the choice of being comprehensive and declaring all these combinations or only arbitrarily choosing a subset. Both options are not ideal, especially since there are better choice in how to design an API for these two concepts (i.e. #3759).

If the unit package is decoupled from the metric pipeline (see above). The unit package will become a purely convenience API for unit definitions. This is not needed by the OTel specification, meaning it is not needed for a GA release of the metric API. Therefore, the unit package should be removed at this time.

If the unit package is wanted by users in the future it can be appropriately designed and added then.

Footnotes

  1. Java's setUnit accepts a string.

  2. Java's Unit is stored as a string.

  3. Python's optional unit as a string.

@MrAlias
Copy link
Contributor Author

MrAlias commented Mar 1, 2023

The unit package was deprecated in #3776. Moving this to the v1.15.0 milestone where the final removal of the package needs to take place.

@MrAlias MrAlias modified the milestones: v1.15.0-RC1, v1.15.0 Mar 1, 2023
@MrAlias
Copy link
Contributor Author

MrAlias commented Mar 2, 2023

Closed by #3814

@MrAlias MrAlias closed this as completed Mar 2, 2023
@MrAlias MrAlias modified the milestones: v1.15.0, v1.15.0-RC2 Mar 22, 2023
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
Projects
No open projects
Status: Done
Development

No branches or pull requests

1 participant