Reduce transitive dependencies of components regarding internal telemetry providers

[atoulme]

Component(s)

No response

Describe the issue you're reporting

internal/telemetry is an internal module used to configure how a component can expose telemetry about itself.

This module aims to eventually become public - See #12405

2 issues arise from this module:

• The module bundles together reading the telemetry configuration of the component and the creation of the metric/trace/log providers, therefore adding indirect dependencies to all modules downstream related to those.
  This becomes a problem when those dependencies bring CVEs, such as CVE-2023-45142 for the Go SDK.
• The lifecycle by which the module allows to set their own telemetry attributes resets the existing traces/metric/log providers. It is better to set those late in the process of creating the component to avoid any resource allocation or potential leaks by initializing providers (sorry for the fear mongering, just want this as clean as possible).

Tip

_{React with 👍 to help prioritize this issue. Please use comments to provide useful context, avoiding +1 or me too, to help us triage it. Learn more here.}

[atoulme]

#13827 is open to perform the first change, moving the code that is responsible for setting the telemetry providers to a separate module to avoid adding indirect dependencies to all the modules. You can see the effect of this PR is a reduction of indirect dependencies across the board.

#13832 builds on top of it and adds a set of Factory option in the unstable x* modules to set it. This option allows the factory to dictate which telemetry attributes to set, based on a set offered by default. This approach allows to set the providers as part of component initialization in the service module. This also allows to move the module created in #13827 into the service module to reduce the complexity.

[jade-guiton-dd]

(Sorry for the long comment, I had a lot of thoughts about this.)

First, could you clarify a few things?

• What do you mean by "reading the telemetry configuration of the component"? Do you mean the calls to telemetry.WithoutAttributes to specify which scope attributes to exclude?

• What do you mean by "the creation of the metric/trace/log providers"? Are you talking about tracerProviderWithAttributes and the other structs in componentattribute?

  If so, those are just wrappers around existing providers, from any SDK. The actual providers are instantiated in service/telemetry/otelconftelemetry, not in internal/telemetry.

  This means that calling the internal/telemetry API to change the set of auto-injected attributes should not "reset the existing providers" or risk "potential leaks by initializing providers". It creates a new wrapper around the same underlying provider, only changing the extra arguments passed to the next TracerProvider.Tracer call.

---

Correct me if I'm wrong, but to reformulate, it sounds like the goal of this issue is to move the implementation of telemetry.WithAttributeSet so that its dependencies are no longer the dependencies of every single component. Notably, the dependencies of concern seem to be the otelzap bridge, and the official Go OTel SDK.

However, since the logic in internal/telemetry is a core part of the way the service sets up the internal telemetry pipeline, it sounds like the only way a full build would not have these dependencies (and thus, not be impacted by the CVEs associated with them) is if it doesn't use the OCB, otelcol, or service, and if it reimplements the bridge between OTel and Zap necessary to make the TelemetrySettings API work without using otelzap.

This seems to me like a niche enough case that accomodating it should not come at the cost of the flexibility of the telemetry.WithAttributeSet API, especially since the feature is still under a feature gate and we haven't yet had feedback on the potential use cases across the ecosystem.

In #13827, that cost is making the API more awkward to use, requiring calls to two different packages. And more importantly, because it only moves around the implementation without separating it from its interface, components that use this API will still have the same transitive dependencies. This is notably the case of the OTLP receiver, meaning that only a full build not featuring this crucial receiver would benefit. For these reasons, I don't think it's the right way to go about this.

In #13832, the API is changed completely, going through a Factory option instead, which allows decoupling the interface from the implementation, successfully removing the dependency from the OTLP receiver. However, doing it this way prevents components from creating multiple TelemetrySettings with different sets of component attributes.

I agree the use case isn't obvious, but I believe there are several that may pop up once the API starts being used:

• For example, a component may include both "singleton" parts that reused across all signals, and parts which are specific to a signal. It may then want to remove the "signal" attribute, but only for telemetry from the former half. This could be the case of the OTLP receiver; while the receiver as a whole is a singleton, once a signal-specific API has been called, it may be useful to reintroduce the "signal" attribute.
• Another example: components like the loadbalancingexporter, which instantiate other (sub)components inside of them, may want to set appropriate component-identifying attributes for the TelemetrySettings it passes to them. This would make it easier to differentiate telemetry from the loadbalancingexporter itself and from the OTLP exporters it instantiates.

---

Given all this, here is one alternative proposal for tackling this issue while addressing the above concerns (although I have doubts on whether the impact is worth the effort):

Since the goal is to make these dependencies conditional on using service, I would suggest we go all the way and move the code responsible for creating and updating the provider wrappers (ie. the InitializeWithAttributes function from #13832) into a method on service's implementation of component.Host.

Then, we add a host parameter to telemetry.WithAttributeSet, and make it call this method, if it exists. This would cleanly separate the interface from the implementation, allowing custom Hosts with different dependencies.

---

If we want to go further, I would also suggest removing the dependency on the trace SDK in the way I suggested in #13832, since it's fairly superfluous. The only reason we have this dependency is to allow components like zpagesextension to call SDK-specific methods on the trace provider.

I suggest that, following the pattern used in the errors package, we add an Unwrap method on the wrapped providers, and an accompanying As utility function, which would allow components to access SDK-specific methods despite the presence of wrappers around the providers (which a custom telemetry provider factory may want to add). Then we can modify zpagesextension and the hopefully few other components that make use of SDK-specific methods to use this new API.

---

What are your thoughts?

[atoulme]

(Sorry for the long comment, I had a lot of thoughts about this.)

First, could you clarify a few things?

• What do you mean by "reading the telemetry configuration of the component"? Do you mean the calls to telemetry.WithoutAttributes to specify which scope attributes to exclude?

correct.

• What do you mean by "the creation of the metric/trace/log providers"? Are you talking about tracerProviderWithAttributes and the other structs in componentattribute?
  If so, those are just wrappers around existing providers, from any SDK. The actual providers are instantiated in service/telemetry/otelconftelemetry, not in internal/telemetry.
  This means that calling the internal/telemetry API to change the set of auto-injected attributes should not "reset the existing providers" or risk "potential leaks by initializing providers". It creates a new wrapper around the same underlying provider, only changing the extra arguments passed to the next TracerProvider.Tracer call.

understood.

Correct me if I'm wrong, but to reformulate, it sounds like the goal of this issue is to move the implementation of telemetry.WithAttributeSet so that its dependencies are no longer the dependencies of every single component. Notably, the dependencies of concern seem to be the otelzap bridge, and the official Go OTel SDK.

correct.

However, since the logic in internal/telemetry is a core part of the way the service sets up the internal telemetry pipeline, it sounds like the only way a full build would not have these dependencies (and thus, not be impacted by the CVEs associated with them) is if it doesn't use the OCB, otelcol, or service, and if it reimplements the bridge between OTel and Zap necessary to make the TelemetrySettings API work without using otelzap.

correct.

This seems to me like a niche enough case that accomodating it should not come at the cost of the flexibility of the telemetry.WithAttributeSet API, especially since the feature is still under a feature gate and we haven't yet had feedback on the potential use cases across the ecosystem.

I am not sure that's niche. In any case, you can see that I have multiple programs that do not use service such as:

• https://github.com/otel-warez/stdinotel
• https://github.com/splunk/splunk_connect_for_otlp

In #13827, that cost is making the API more awkward to use, requiring calls to two different packages. And more importantly, because it only moves around the implementation without separating it from its interface, components that use this API will still have the same transitive dependencies.

No, the components will not list otelzap in their dependencies. You can see the diff of the PR as proof.

This is notably the case of the OTLP receiver, meaning that only a full build not featuring this crucial receiver would benefit. For these reasons, I don't think it's the right way to go about this.

I respectfully disagree. For one, if you ever want the telemetry API to become public, you must separate the implementation libs used for trace/log/metric providers elsewhere, or we cannot have an easy way to manage a component dependencies. See how this brings CVEs to the source tree of the code (my second example above fails Fossa because of the inclusion of the otelzap dependency).

In #13832, the API is changed completely, going through a Factory option instead, which allows decoupling the interface from the implementation, successfully removing the dependency from the OTLP receiver. However, doing it this way prevents components from creating multiple TelemetrySettings with different sets of component attributes.

I agree the use case isn't obvious, but I believe there are several that may pop up once the API starts being used:

• For example, a component may include both "singleton" parts that reused across all signals, and parts which are specific to a signal. It may then want to remove the "signal" attribute, but only for telemetry from the former half. This could be the case of the OTLP receiver; while the receiver as a whole is a singleton, once a signal-specific API has been called, it may be useful to reintroduce the "signal" attribute.
• Another example: components like the loadbalancingexporter, which instantiate other (sub)components inside of them, may want to set appropriate component-identifying attributes for the TelemetrySettings it passes to them. This would make it easier to differentiate telemetry from the loadbalancingexporter itself and from the OTLP exporters it instantiates.

OK. We can make this better, and this factory option can handle this I suppose.

Given all this, here is one alternative proposal for tackling this issue while addressing the above concerns (although I have doubts on whether the impact is worth the effort):

Since the goal is to make these dependencies conditional on using service, I would suggest we go all the way and move the code responsible for creating and updating the provider wrappers (ie. the InitializeWithAttributes function from #13832) into a method on service's implementation of component.Host.

Then, we add a host parameter to telemetry.WithAttributeSet, and make it call this method, if it exists. This would cleanly separate the interface from the implementation, allowing custom Hosts with different dependencies.

That seems about the same or better than a factory option, maybe a bit harder to discover. Sure?

If we want to go further, I would also suggest removing the dependency on the trace SDK in the way I suggested in #13832, since it's fairly superfluous. The only reason we have this dependency is to allow components like zpagesextension to call SDK-specific methods on the trace provider.

I suggest that, following the pattern used in the errors package, we add an Unwrap method on the wrapped providers, and an accompanying As utility function, which would allow components to access SDK-specific methods despite the presence of wrappers around the providers (which a custom telemetry provider factory may want to add). Then we can modify zpagesextension and the hopefully few other components that make use of SDK-specific methods to use this new API.

What are your thoughts?

Looks like that zpagesextension is trouble. Worth looking into on its own, maybe applying your section here as a separate issue.

[axw]

Looks like that zpagesextension is trouble. Worth looking into on its own, maybe applying your section here as a separate issue.

I spiked on this, one option is to invert the dependency and make it the responsibility of the telemetry implementation to register itself with the zpages mux: https://github.com/open-telemetry/opentelemetry-collector/compare/main...axw:opentelemetry-collector:hostedcapabilities-zpages?expand=1

If it looks like a sensible direction, I can clean it up for proper review.

[atoulme]

Looks like that zpagesextension is trouble. Worth looking into on its own, maybe applying your section here as a separate issue.

I spiked on this, one option is to invert the dependency and make it the responsibility of the telemetry implementation to register itself with the zpages mux: https://github.com/open-telemetry/opentelemetry-collector/compare/main...axw:opentelemetry-collector:hostedcapabilities-zpages?expand=1

If it looks like a sensible direction, I can clean it up for proper review.

sgtm!

[jade-guiton-dd]

No, the components will not list otelzap in their dependencies. You can see the diff of the PR as proof.

I said this precisely because I looked at the diff of #13827. If you look at receiver/otlpreceiver/go.mod, you will see that it does not remove otelzap from the indirect dependencies. (In fact it upgrades it for some reason.)

[jade-guiton-dd]

Looks like that zpagesextension is trouble. Worth looking into on its own, maybe applying your section here as a separate issue.

I don't really think the zpagesextension is doing anything strange, it's just relying on important features of the OTel SDK which have not percolated into the OTel API yet (in a way which doesn't break things if they're not available, it just turns into a no-op).

If we intend to support zpagesextension as a core part of the service, I guess it makes sense to require telemetry provider factories to provide support for it. But that solution will not extend to other hypothetical extensions which want to set up span/log processors to capture/enhance the Collector's internal telemetry. Would this imply going the route of requiring components to only depend on the core OTel API, with the zpagesextension as a sole exception?

[axw]

Would this imply going the route of requiring components to only depend on the core OTel API, with the zpagesextension as a sole exception?

@jade-guiton-dd I think there is another option: add another layer of indirection. Instead of having components rely directly on the OTel SDK, or only on the OTel API, the collector could introduce its own cut-down SpanProcessor API until the OTel API grows one. Similar to what you mention above, this hypothetical collector SpanProcessor API would be effective only when using the OTel Go SDK, and would be a no-op otherwise.

(IMO we should still make the change I mentioned above first, and then only do this later as needed.)