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 metric No-Op to metric/noop #3893

Merged
merged 10 commits into from
Mar 21, 2023

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Mar 15, 2023

Fix #3886: the existing way for a user to create a no-op Meter without using the MeterProvider is removed.

  • Move metric No-Op to its own package
  • Export No-Op types

Move metric No-Op to its own package

The go.opentelemetry.io/otel/metric package is intended to primarily be used by instrumentation authors. Having the No-Op implementation in that package, something they will not interact with, only pollutes the API and clutters the documentation for these users.

This API pollution and documentation clutter is compounded when exporting No-Op types. It would mean there would be ~20 objects in go.opentelemetry.io/otel/metric that all some prefix of Noop. Instead of using a prefix to collect common objects, they are moved to their own package.

Export No-Op types

Part of #3865 includes providing No-Op implementations for all the API interfaces. These types can then be embedded in implementations that want to default to No-Op behavior instead of panicking.

What about the specification?

The No-Op MUST NOT provide a way for a user to create a Meter other than by a No-Op MeterProvider.

Counters are always created by a Meter, the No-Op MUST NOT provide a way for a user to create a Counter other than by a No-Op Meter.

UpDownCounters are always created by a Meter, the No-Op MUST NOT provide a way for a user to create a UpDownCounter other than by a No-Op Meter.

Histograms are always created by a Meter, the No-Op MUST NOT provide a way for a user to create a Histogram other than by a No-Op Meter.

Asynchronous Counters are always created by a Meter, the No-Op MUST NOT provide a way for a user to create a Asynchronous Counter other than by a No-Op Meter.

Asynchronous UpDownCounters are always created by a Meter, the No-Op MUST NOT provide a way for a user to create a Asynchronous UpDownCounter other than by a No-Op Meter.

Asynchronous Gauges are always created by a Meter, the No-Op MUST NOT provide a way for a user to create a Asynchronous Gauge other than by a No-Op Meter.

All of these requirements mean that the No-Op cannot provide a way for a user to create these types accept via the approved creation types.

I think, given the noop package does not provide creation functions for anything other than the MeterProvider, this approach still satisfies the specification. It true that a user could use the Go language features on their own to create one of these types, but I think the specification would say that the API "must not allow a user to create" these types outside of via the approved creation types.

@MrAlias MrAlias added pkg:API Related to an API package area:metrics Part of OpenTelemetry Metrics labels Mar 15, 2023
@MrAlias MrAlias added this to the v1.15.0 milestone Mar 15, 2023
@codecov
Copy link

codecov bot commented Mar 15, 2023

Codecov Report

Merging #3893 (084b24e) into main (90df525) will decrease coverage by 0.1%.
The diff coverage is 100.0%.

❗ Current head 084b24e differs from pull request most recent head bed06f3. Consider uploading reports for the commit bed06f3 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #3893     +/-   ##
=======================================
- Coverage   81.8%   81.8%   -0.1%     
=======================================
  Files        169     169             
  Lines      12855   12859      +4     
=======================================
+ Hits       10517   10520      +3     
- Misses      2119    2120      +1     
  Partials     219     219             
Impacted Files Coverage Δ
metric/noop/noop.go 100.0% <100.0%> (ø)

... and 1 file with indirect coverage changes

@MrAlias
Copy link
Contributor Author

MrAlias commented Mar 17, 2023

@MrAlias
Copy link
Contributor Author

MrAlias commented Mar 17, 2023

@Aneurysm9: thinking about #3865 last night, I think we don't have to add a private method to all the interfaces, but could still provide this No-Op implementation. That way, an SDK author could choose to:

  1. Leave their implementation without an embedded implementation (current default)
  2. Embed the interface itself so it would panic on uninitialized methods (not sure why they would do this, but 🤷)
  3. Embed this No-Op implementation in theirs if they want the default behavior to be No-Op

(This still wouldn't change the Observable interface being embedded in all observables)

Thoughts?

@Aneurysm9
Copy link
Member

That works for me as long as the spec stops prohibiting direct construction of no-op implementations. I still think we need giant flashing warning signs telling SDK creators and users that they risk having silently incorrect implementations if they choose to embed no-op implementations. It feels like a bad idea to me, but all the other options are also bad.

metric/noop/noop.go Outdated Show resolved Hide resolved
metric/noop/noop.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@MadVikingGod MadVikingGod merged commit 3c75a44 into open-telemetry:main Mar 21, 2023
MrAlias added a commit to MrAlias/opentelemetry-go that referenced this pull request Mar 22, 2023
@MrAlias MrAlias modified the milestones: v1.15.0, v1.15.0-RC2 Mar 22, 2023
MrAlias added a commit that referenced this pull request Mar 22, 2023
* Revert "Move metric No-Op to `metric/noop` (#3893)"

This reverts commit 3c75a44.

* Persist removal of NewNoopMeter
MrAlias added a commit to MrAlias/opentelemetry-go that referenced this pull request Mar 27, 2023
MrAlias added a commit that referenced this pull request Mar 29, 2023
* Revert "Revert "Move metric No-Op to metric/noop (#3893)" (#3921)"

This reverts commit 795ad97.

* Add PR number

* Move example_test back to `otel/metric`

* Update CHANGELOG.md

Co-authored-by: Robert Pająk <pellared@hotmail.com>

* Remove redundant panic tests

* Update noop pkg docs

---------

Co-authored-by: Robert Pająk <pellared@hotmail.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
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Metric No-Op API allows Meter to be created outside of MeterProvider
6 participants