Wrap TelemetrySettings providers to add component-identifying attributes

[jade-guiton-dd]

Description

Following the discussions in #12217, this PR adds (internal for now) code which wraps the TracerProvider and MeterProvider in component.TelemetrySettings to automatically add the component-identifying attributes defined in the Pipeline Component Telemetry RFC to all telemetry created by components.

This PR has one addition to the user-visible API: component.TelemetrySettings now has a InstanceAttributes attribute.Set field, which is used as the source of truth for the TracerProvider, MeterProvider, and Logger. If this field is updated manually, the providers can be updated with componentattribute.UpdateInstanceAttributes.

The common use case of components opting out of certain attributes can be achieved by calling componentattribute.RemoveInstanceAttributes.

Link to tracking issue

Fixes #12217

Also resolves #12213 and resolves #12117, as receiverhelper and exporterhelper use the TelemetrySettings of the component that uses them

Testing

For now, I've only done manual testing to check that the attributes are indeed added for the two signals.

Documentation

None for now.

[djaglowski]

This PR has one addition to the user-visible API: component.TelemetrySettings now has a InstanceAttributes attribute.Set field, which is used as the source of truth for the TracerProvider, MeterProvider, and Logger. If this field is updated manually, the providers can be updated with componentattribute.UpdateInstanceAttributes.

Do we have a real need for this? Seems unnecessary as far as the RFC goes. Can we leave it out for now and it can be proposed separately if necessary?

The common use case of components opting out of certain attributes can be achieved by calling componentattribute.RemoveInstanceAttributes.

One thing I notice about this implementation is it treats TelemetrySettings in such a way that there is only one per component instance, as opposed to allowing derivatives to be made as necessary. The logger was implemented to follow traditional logger patterns, where one part of the codebase may want its own logger. For example, #12203 shows how the OTLP receiver could be refactored to have separate instances that share only the server. In this case, we'd want one set of attributes for each instance, and a different set for the shared server.

In my opinion it would make sense that this same pattern be applied across the entire TelemetrySettings, so that share parts of the codebase can have their own set of attributes independently of other parts.

[jade-guiton-dd]

Do we have a real need for this [TelemetrySettings.InstanceAttributes]? Seems unnecessary as far as the RFC goes. Can we leave it out for now and it can be proposed separately if necessary?

To be able to remove attributes from a TelemetrySettings instance, we need to store the current list of attributes somewhere. In the original Logger implementation this was done internally, but since now the attribute set will be shared by the Logger, TracerProvider, and MetricProvider, I'm not sure where else to put it but the parent TelemetrySettings.

I've considered making it an unexported field to avoid expanding the API, but that would require putting the new functions as methods in the component package, which would make them user-visible, and I feel like their API is probably more subject to change than attribute.Set.

Do you have any ideas on how to leave it out?

One thing I notice about this implementation is it treats TelemetrySettings in such a way that there is only one per component instance, as opposed to allowing derivatives to be made as necessary. The logger was implemented to follow traditional logger patterns, where one part of the codebase may want its own logger.

Component authors can easily make a copy of the TelemetrySettings manually and call RemoveInstanceAttributes on it. Do you think it would be better if RemoveInstanceAttributes returned a modified copy of the original TelemetrySettings instead of modifying the original?

[jade-guiton-dd]

By the way, this may be a minor detail, but I noticed that the otelcol.component.kind attribute is capitalized (Exporter, Processor etc.), because the names are taken from here. This doesn't match the RFC, which specifies lowercase names. Are we okay with that?

[djaglowski]

Do we have a real need for this [TelemetrySettings.InstanceAttributes]? Seems unnecessary as far as the RFC goes. Can we leave it out for now and it can be proposed separately if necessary?

To be able to remove attributes from a TelemetrySettings instance, we need to store the current list of attributes somewhere. In the original Logger implementation this was done internally, but since now the attribute set will be shared by the Logger, TracerProvider, and MetricProvider, I'm not sure where else to put it but the parent TelemetrySettings.

I've considered making it an unexported field to avoid expanding the API, but that would require putting the new functions as methods in the component package, which would make them user-visible, and I feel like their API is probably more subject to change than attribute.Set.

Do you have any ideas on how to leave it out?

One thing I notice about this implementation is it treats TelemetrySettings in such a way that there is only one per component instance, as opposed to allowing derivatives to be made as necessary. The logger was implemented to follow traditional logger patterns, where one part of the codebase may want its own logger.

Component authors can easily make a copy of the TelemetrySettings manually and call RemoveInstanceAttributes on it. Do you think it would be better if RemoveInstanceAttributes returned a modified copy of the original TelemetrySettings instead of modifying the original?

I believe the solution to both problems is the same - to have the "Without" method (whatever it is called) return a new instance. This way, the attributes do not need to be exported.

On the topic of naming, I'd prefer we not use "Instance" in the name since TelemetrySettings is not meant exclusively for instances. We may find that there are other situations where we preload some attributes and allow them to be removed. Personally I think Without or WithoutAttributes conveys that we're returning a copy, while RemoveAttributes conveys that we're editing in place.

[jade-guiton-dd]

Ok, I'll move the RemoveInstanceAttributes function to component and change it to a TelemetrySettings.WithoutAttributes method then

[mx-psi]

Ok, I'll move the RemoveInstanceAttributes function to component and change it to a TelemetrySettings.WithoutAttributes method then

Can we keep this as a function in internal/telemetry (same as LoggerWithout) instead of a method in component? I would suggest we keep this internal until we are done fully implementing the RFC in a way that works with the components in this repository, so that we can change our mind in the process if we want to.

[jade-guiton-dd]

If we do that, we can't access an unexported field on TelemetrySettings.

[mx-psi]

If we do that, we can't access an unexported field on TelemetrySettings.

If we need to, we can define TelemetrySettings in internal/telemetry and re-export it in component

[jade-guiton-dd]

That's a good point, I hadn't thought of that.

[jade-guiton-dd]

I updated the API, tell me if this is not what you had in mind @djaglowski.

It looks like the remaining CI failure is related to the fact that zpagesextension relies on SDK-only methods for TracerProvider. I added a tracerProviderWithAttributesSdk variant of the tracerProviderWithAttributes wrapper, which is used when the wrapped TracerProvider is from the official SDK, and exposes all its methods through the embedding.

This has the side effect of adding an indirect dependency on the SDK to a lot of modules, including component, configauth, connector, extension... Is that okay? If not I can define a new TracerProviderSdkExtensions interface, add methods from the SDK to it as needed, and use that instead of the full struct (this is what zpagesextension does when checking for the existence of those methods).

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.61%. Comparing base (eb6fe00) to head (5d7a01a).

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #12384      +/-   ##
==========================================
+ Coverage   91.55%   91.61%   +0.06%     
==========================================
  Files         467      472       +5     
  Lines       25652    25857     +205     
==========================================
+ Hits        23485    23690     +205     
  Misses       1768     1768              
  Partials      399      399              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[dmitryax]

This approach looks good to me. Thank you for putting this together. Are there any visible changes for end-users at this point?

[jade-guiton-dd]

I would say yes, since this adds otelcol.component.id and friends to basically all internal metrics/logs.

[dmitryax]

Could you please add this to the subtext or create another user-facing changelog item?

[jade-guiton-dd]

The description of the item is "Internal metrics and spans now include component attributes", and the subtext already says "[The providers] automatically insert metric/span attributes which identify the originating component instance, as defined in the Pipeline Component Telemetry RFC". Do you think I should list the attributes explicitly?

[jade-guiton-dd]

I reworded the changelog and added more details, tell me whether you think it's sufficient.

[mx-psi]

This has the side effect of adding an indirect dependency on the SDK to a lot of modules, including component, configauth, connector, extension... Is that okay? If not I can define a new TracerProviderSdkExtensions interface, add methods from the SDK to it as needed, and use that instead of the full struct (this is what zpagesextension does when checking for the existence of those methods).

I like the new interface option better

[jade-guiton-dd]

I like the new interface option better

Looking into it more, it looks like it's not actually doable without importing the SDK unfortunately, since the interface relies on types from the SDK. The zpagesextension defines an extension with the two methods it needs, but imports the SDK anyway for the types in the method signatures. This is fine for its purpose (in case another API implementation has those methods but not others I guess), but in our case, we probably want to expose as much of the SDK as possible.

[jade-guiton-dd]

This method of wrapping the callbacks (using NewXConfig to parse the options, wrapping the callbacks, then manually "reserializing" the config) will be somewhat problematic if the API adds a new kind of option for asynchronous instruments, as we will be silently dropping them until we update this code. Does anyone have a better way to do this?

[jade-guiton-dd]

An alternative method of exposing SDK-specific TracerProvider methods would be to explicitly declare which methods we're forwarding. This would not allow us to drop the dependency on the SDK (since the relevant method signatures make use of SDK-only types), and it would require updating this code whenever new SDK-specific method are added. However, it would mean that alternate API implementations could hypothetically provide those same methods and have them forwarded.

Should we consider this an issue? Are there currently any alternate SDKs that provides methods like TracerProvider.RegisterSpanProcessor? If yes, do we expect such cross-SDK methods to be added to the API eventually?