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

Asynchronous Instrument API non-compliant with OTel Specification: callbacks cannot be unregistered #3452

Closed
MrAlias opened this issue Nov 8, 2022 · 5 comments · Fixed by #3522
Assignees
Labels
area:metrics Part of OpenTelemetry Metrics bug Something isn't working pkg:API Related to an API package

Comments

@MrAlias
Copy link
Contributor

MrAlias commented Nov 8, 2022

The OpenTelemetry specification states:

The API SHOULD support registration of callback functions associated with asynchronous instruments after they are created.

Where the API supports registration of callback functions after asynchronous instrumentation creation, the user MUST be able to undo registration of the specific callback after its registration by some means.

Currently, we allow callbacks to be registered with the Meter. We do not support an "un-register" mechanism.

@MrAlias MrAlias added bug Something isn't working pkg:API Related to an API package area:metrics Part of OpenTelemetry Metrics labels Nov 8, 2022
@MrAlias
Copy link
Contributor Author

MrAlias commented Nov 8, 2022

One proposal is to have the register call return the unregister mechanism:

type Meter interface {
	// ...
	RegisterCallback([]instrument.Asynchronous, func(context.Context)) (Unregisterer, error)
}

type Unregisterer interface {
	Unregister() error
}

@MrAlias
Copy link
Contributor Author

MrAlias commented Nov 9, 2022

Same structure different naming:

type Meter interface {
	// ...
	RegisterCallback([]instrument.Asynchronous, func(context.Context)) (Registration, error)
}

type Registration interface {
	Cancel() error
}

This would however go against common Go naming practice of naming the interface after the one method it performs. However, it would allow for the interface to be extended in the future without then having a naming mismatch. Also the return type name would better match what it represents.

@MrAlias
Copy link
Contributor Author

MrAlias commented Nov 9, 2022

I think we can also update the function signature to better communicate one or more instrument is needed and at the same time reduce the boilerplate needed to pass arguments:

type Meter interface {
	// ...
	RegisterCallback(f func(context.Context), instrument instrument.Asynchronous, additional ...instrument.Asynchronous) (Registration, error)
}

This was inspired from the Java implementation: https://javadoc.io/doc/io.opentelemetry/opentelemetry-api/latest/io/opentelemetry/api/metrics/Meter.html#batchCallback(java.lang.Runnable,io.opentelemetry.api.metrics.ObservableMeasurement,io.opentelemetry.api.metrics.ObservableMeasurement...)

This means a call like:

m.RegisterCallback([]instrument.Asynchronous{i1, i2}, cBack)

becomes

m.RegisterCallback(cBack, i1, i2)

@MrAlias
Copy link
Contributor Author

MrAlias commented Nov 10, 2022

I think we can also update the function signature to better communicate one or more instrument is needed and at the same time reduce the boilerplate needed to pass arguments:

type Meter interface {
	// ...
	RegisterCallback(f func(context.Context), instrument instrument.Asynchronous, additional ...instrument.Asynchronous) (Registration, error)
}

This was inspired from the Java implementation: https://javadoc.io/doc/io.opentelemetry/opentelemetry-api/latest/io/opentelemetry/api/metrics/Meter.html#batchCallback(java.lang.Runnable,io.opentelemetry.api.metrics.ObservableMeasurement,io.opentelemetry.api.metrics.ObservableMeasurement...)

This means a call like:

m.RegisterCallback([]instrument.Asynchronous{i1, i2}, cBack)

becomes

m.RegisterCallback(cBack, i1, i2)

This will not work well if you already have a slice of instrument. You cannot just use ... after the slice to directly pass it.

i.e. m.Register(cBack, s[0], s[1:]...)

@jmacd
Copy link
Contributor

jmacd commented Nov 10, 2022

I remember discussing this once with @MadVikingGod -- one idea we discussed was to admit a second form of callback constructor, one that returns an unregistration handle of some kind, e.g., NewUnregisterableCallback(func (context.Context)) (Unregister, error)

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 bug Something isn't working pkg:API Related to an API package
Projects
No open projects
Status: Done
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants