open-telemetry / opentelemetry-go

OpenTelemetry Go API and SDK
https://opentelemetry.io/docs/languages/go
Apache License 2.0
5.24k stars 1.06k forks source link

Confusion about duplicate register callback API and when to use one or the other #3634

Open bogdandrutu opened 1 year ago

bogdandrutu commented 1 year ago

Right now there are 2 ways/APIs to register a callback associated with a Observer:

Questions:

MrAlias commented 1 year ago
  • With the RegisterCallback you can stop collection, which you cannot do with the first version. Is there a reason not to allow "stopping" an Observer instrument entirely?

This comes from the API specification, it says these callbacks need to be permanently registered.

The API MUST support creation of asynchronous instruments by passing zero or more callback functions to be permanently registered to the newly created instrument.

MrAlias commented 1 year ago
  • When it is recommended to use one or the other?

You touched on some of the important distinctions already, but for completeness, with the RegisterCallback method you can

I think the recommendation is that if you need any of these features you should use the RegisterCallback method and if not, just register during creation. There is a slight performance improvement for the single instrument registrations, but it isn't a large enough difference to choose one over the other IMO.

bogdandrutu commented 1 year ago

Java has a nice way to "close" an observe instrument, still specification compliant because it is permanently associated with the instrument. See https://javadoc.io/doc/io.opentelemetry/opentelemetry-api/latest/io/opentelemetry/api/metrics/ObservableLongCounter.html

Update: By offering this, it allows me to stop collection for a specific instrument and not having to do the "RegisterCallback" pattern unless I need to register the callback for more than one instrument.

MrAlias commented 1 year ago

Hmm, that's an interesting idea. I'm interested to hear what others think about adding it in the future.

bogdandrutu commented 1 year ago

I'm interested to hear what others think about adding it in the future.

Wouldn't be backwards incompatible? And if you make the dev checking if implementing a Closer interface is as inefficient for the code as going via the RegisterCallback since I need to do one more check anyway.

MrAlias commented 1 year ago

Yeah, I think how we add it will depend on if it is before or after the stable release.

MrAlias commented 1 year ago

@bogdandrutu does close also stop the instrument from being observed in a multi-instrument callback?

MrAlias commented 1 year ago

@bogdandrutu does close also stop the instrument from being observed in a multi-instrument callback?

Maybe that is what this note is describing, and it is not?

20230131_094657

Aneurysm9 commented 1 year ago

This comes from the API specification, it says these callbacks need to be permanently registered.

The API MUST support creation of asynchronous instruments by passing zero or more callback functions to be permanently registered to the newly created instrument.

I don't think that we can enable deregistration of these callbacks without violating the spec. If you need to deregister a callback then the instrument and callback can be associated after instrument creation.

bogdandrutu commented 1 year ago

@Aneurysm9

The API MUST support creation of asynchronous instruments by passing zero or more callback functions to be permanently registered to the newly created instrument.

To the "newly" created instrument. But if you "close" the instrument (remove it) then they will still be permanently registered to that instrument, but the instrument is no-op, hence you removed the callbacks.

Aneurysm9 commented 1 year ago

To the "newly" created instrument. But if you "close" the instrument (remove it) then they will still be permanently registered to that instrument, but the instrument is no-op, hence you removed the callbacks.

Where is this in the spec?

bogdandrutu commented 1 year ago

What do you look for the spec? Here is the link.

Aneurysm9 commented 1 year ago

There is no close method defined for any API type in the spec.