refactor: Default to Proxy Tracer in instrumentations

[arielvalentin]

While working on #677, I ran into an issue where the base instrumentation tracer was not being automatically upgraded after the SDK initialization was complete.

I am not sure why this was implemented this way but I figured other instrumentations would need a reference to a proxy tracer in the future.

open-telemetry/opentelemetry-ruby#671

[fbogsany]

The key point in @robertlaurin's PR was:

In the case where the instrumentation is never installed you would having to conditionally check for the presence of a tracer. This change allows those who are instrumenting their library to simply add the trace points using the Instrumention.instance.tracer handle indifferently.

Auto-instrumentation has a no-op tracer until it is properly installed. If it is never installed, (naive) instrumentation code can still use the tracer without creating spans and without repeatedly checking if the instrumentation is installed. The change in this PR will upgrade the tracer to create spans when the SDK is installed, even if the instrumentation is not installed, so naive instrumentation code would have to be less naive, checking if it has been installed before using the named tracer.

It's worth debating whether this is useful in practice. I believe all our instrumentation explicitly monkey-patches or registers hooks only when installed. I think the original PR was aimed at supporting first-party instrumentation with the same framework (for config, in particular).

[fbogsany]

Also, to be clear, this represents a change in behaviour, so not really a refactor. I'm not entirely sure how this should be treated in terms of versioning if we decide to go ahead with it.

[arielvalentin]

The key point in @robertlaurin's PR was:

In the case where the instrumentation is never installed you would having to conditionally check for the presence of a tracer. This change allows those who are instrumenting their library to simply add the trace points using the Instrumention.instance.tracer handle indifferently.

Auto-instrumentation has a no-op tracer until it is properly installed. If it is never installed, (naive) instrumentation code can still use the tracer without creating spans and without repeatedly checking if the instrumentation is installed. The change in this PR will upgrade the tracer to create spans when the SDK is installed, even if the instrumentation is not installed, so naive instrumentation code would have to be less naive, checking if it has been installed before using the named tracer.

It's worth debating whether this is useful in practice. I believe all our instrumentation explicitly monkey-patches or registers hooks only when installed. I think the original PR was aimed at supporting first-party instrumentation with the same framework (for config, in particular).

It would be helpful to me to see a use case where we saw this cause a problem. I feel like I am missing something here based on your explanation.

Assuming first-part instrumentation were to exists and they derived their instrumentation classes from Base, then their instrumentation would be automatically registered with the SDK¹ and would need to adhere to the Base classes auto-installation lifecycle methods to avoid running into issues.

If for some reason they reimplemented Base#install, then they would still have to override the tracer ² as part of their implementation.

First party implementers that decide to deviate from this pattern, e.g. override methods in Base or not use the SDK Configure installation lifecycle, would not be impacted by this change; since they are managing their own installation process and creating their own tracers.

If someone did use Base but naively implemented their instrumentations such that they would be "Active" without having been installed really did not understand that they should have provided an install block³ where they then do the work of lazily installing their instrumentations will likely run into other problems before running into this issue with the proxy-> upgraded tracer.

Is there something else or another use case that I have overlooked?

I chose refactor because I did not perceive this as having a change in externally visible behavior but it is a big internal change for sure so I can reclassify this as a feat! to force a minor bump assuming no objections.

Footnotes

1. https://github.com/open-telemetry/opentelemetry-ruby-contrib/blob/main/instrumentation/base/lib/opentelemetry/instrumentation/base.rb#L78 ↩

2. https://github.com/open-telemetry/opentelemetry-ruby-contrib/blob/main/instrumentation/base/lib/opentelemetry/instrumentation/base.rb#L223 ↩

3. https://github.com/open-telemetry/opentelemetry-ruby-contrib/blob/main/instrumentation/base/lib/opentelemetry/instrumentation/base.rb#L222C17-L222C17 ↩

[robertlaurin]

Sorry for the slow response, any reason why we can do a simple revert of https://github.com/open-telemetry/opentelemetry-ruby/pull/671/files ?

Expanding a bit here, it seems like a revert opens an issue where any consumers of base trying to access a tracer before the install hook fires will have a problem.

[arielvalentin]

Sorry for the slow response, any reason why we can do a simple revert of https://github.com/open-telemetry/opentelemetry-ruby/pull/671/files ?

Expanding a bit here, it seems like a revert opens an issue where any consumers of base trying to access a tracer before the install hook fires will have a problem.

Reverting a commit is a-ok with me. Would you mind sharing more context of how 1P implementers are accessing the tracer before the instrumentation instance is initialized?

[robertlaurin]

Reverting a commit is a-ok with me. Would you mind sharing more context of how 1P implementers are accessing the tracer before the instrumentation instance is initialized?

We discussed this at length on the sig, but there isn't any good example.

It might be better to frame the competing ideas here as:

• All tracers should be upgraded unconditionally when the SDK is configured
• An instrumentation instance that fails to install should have a noop tracer (or alternatively phrased it should not have an active tracer)

I'm in the camp that a tracer should not be upgraded in the event that the instrumentation install fails.

Commentary from Francis

but that comes with a caveat that instrumentation that fails to install should really not "partially install" a codepath that calls its tracer - I think that would effectively break any trace along that codepath (because we'd end up with a no-op span)

So lets assume we have some misbehaving instrumentation that is unconditionally using the instance tracer regardless of the install being successful or not. If we always upgrade all tracers, you'll get spans you did not expect. If you conditionally upgrade the trace, you won't get the spans you didn't expect, but may end up with the behaviour of a broken trace as there may be noop spans scattered in.

I think because we're dealing with hypothetical abuses of instrumentation instances here, it's hard to make a really strong argument for or against either behaviour. I continue to be in favour of the following because the behaviour feels more "predictable" or "expected".

An instrumentation instance that fails to install should have a noop tracer (or alternatively phrased it should not have an active tracer)