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

Move Metrics SDK implementation types into metric/sdkapi sub-package #2090

Closed
jmacd opened this issue Jul 15, 2021 · 7 comments · Fixed by #2271
Closed

Move Metrics SDK implementation types into metric/sdkapi sub-package #2090

jmacd opened this issue Jul 15, 2021 · 7 comments · Fixed by #2271
Labels
area:metrics Part of OpenTelemetry Metrics enhancement New feature or request
Milestone

Comments

@jmacd
Copy link
Contributor

jmacd commented Jul 15, 2021

Description

There are a number of types that exist in the metric package that are needed only for the SDK to support the API, that are not directly used by the user. These types are part of the SDK API, so to speak, so I propose moving them to the sdkapi sub-package. Most of the types in the metric_sdkapi.go file can move there. This has been verified by the experiment in #2044, which replaces the API but continues to use at least these metric types:

  1. MeterImpl
  2. Measurement
  3. InstrumentImpl
  4. SyncImpl
  5. AsyncImpl
  6. InstrumentKind
  7. Descriptor

Expected behavior

In order to create a sub-package for the SDK API types means having no cycles between them. The Measurement type is the one among these that belongs in the public API, but since it is part of the MeterImpl it will live in the sdkapi package. There will be an alias for Measurement from the top-level package so that it can be referred to in public APIs.

@jmacd jmacd added the bug Something isn't working label Jul 15, 2021
@MadVikingGod MadVikingGod added area:metrics Part of OpenTelemetry Metrics enhancement New feature or request and removed bug Something isn't working labels Jul 19, 2021
@MrAlias
Copy link
Contributor

MrAlias commented Jul 22, 2021

I'm weary of this change. If the types being moved to the sdkapi package are truly not needed by the user can we move them to an internal package?

@jmacd
Copy link
Contributor Author

jmacd commented Jul 22, 2021

If we moved the types into an internal package, all the internal SDK code would work just fine, but it would be impossible to implement an alternate SDK in an external repository. These types could move into a subdirectory of /sdk perhaps, but these are not part of the default SDK, they are the connection between any SDK and the API.

@MrAlias
Copy link
Contributor

MrAlias commented Jul 22, 2021

If we moved the types into an internal package, all the internal SDK code would work just fine, but it would be impossible to implement an alternate SDK in an external repository. These types could move into a subdirectory of /sdk perhaps, but these are not part of the default SDK, they are the connection between any SDK and the API.

Gotcha. I know we added these types to help deal with geometric scaling of places we needed to update when we originally were implementing this will a large parameter space, but I'm wondering if we should take this refactor as a chance to rethink these. Having other SDKs not only implement the API directly, but implement this sdkapi package seems like an additional level of complexity. Complexity that I'm not sure is warranted at this point given we don't have any (to my knowledge) other SDKs.

@jmacd
Copy link
Contributor Author

jmacd commented Jul 22, 2021

level of complexity

I see it as simplifying. There are two packages already acting as SDK alternatives here, the metric global package and the metric registry package. These packages are written to the generic synchronous or asynchronous SDK API, instead of having to directly support every instrument.

@jmacd
Copy link
Contributor Author

jmacd commented Jul 22, 2021

Another way to think about this sdkapi package is that it allows us both to upgrade the API w/o changing the SDK or to replace the SDK with an alternative. My motivation here is the first one, to replace the current API with a new one that I can drop in without modifying the sdkapi package or modifying the SDK.

@MrAlias
Copy link
Contributor

MrAlias commented Jul 22, 2021

There are two packages already acting as SDK alternatives here, the metric global package and the metric registry package. These packages are written to the generic synchronous or asynchronous SDK API, instead of having to directly support every instrument.

Hmm, I might not be following. With both of those examples being local packages could we not then just put this into an internal directory?

@jmacd
Copy link
Contributor Author

jmacd commented Jul 22, 2021

I see. To summarize, I think you're calling into question the entire relationship between the API and the default SDK. The existing API uses struct types for the API and an interface named MeterImpl with two methods for the two general kinds of instrument. If Meter were an interface, for example, then sdkapi could become an implementation detail of the default SDK. (At this point, sdkapi is unnecessary except to keep registry and Accumulator functions separated in the code.)

I do not support the Meter-as-interface pattern because it makes it difficult to implement an alternative SDK. Support for decoupling the SDK and API is an essential part of OTel, but you're right that it's not required to be easy. Also the Meter-as-interface pattern makes the global support significantly more difficult. The global package will have to implement deferred objects for every one of the associated interfaces, i.e., all the boilerplate needed for an alternative SDK will also be copied into the global package.

I prefer the Meter-as-struct pattern because it means that all SDKs can share the large amount of code necessary to turn the user-facing API with two calling conventions, six instruments, two number types and a bunch of requirements about unique naming into an SDK. In the Meter-as-struct pattern, I can provide a new SDK without re-implementing the registry package.

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 enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants