opentelemetry-collector icon indicating copy to clipboard operation
opentelemetry-collector copied to clipboard

Published 20 hours ago • verified publisher icon open-telemetry

[config/confighttp] `IncludeMetadata` is experimental

Open atoulme opened this issue 1 year ago • 4 comments

From confighttp.go:

// IncludeMetadata propagates the client metadata from the incoming requests to the downstream consumers
// Experimental: *NOTE* this option is subject to change or removal in the future.
IncludeMetadata bool `mapstructure:"include_metadata"`

atoulme avatar Jan 24 '24 17:01 atoulme

The origin of the Experiment designation is here.

On one hand it has been 2 years and requests to include only certain headers has not surfaced. The feature has been working as needed via the headerssetterextension since v0.58.0 (August 2022).

On the other hand, the API still doesn't let us add this feature in the future without either doing a breaking change or adding a new section, which spreads the feature out between 2 root configuration options.

Changing this now would end up being a user-facing configuration breaking change, not only an API breaking change. Seeing as we haven't had user requests for this feature I am against removing IncludeMetadata so close to stability. If, after a 1.0 release, a specific need to filter headers arises, we could add a new configuration interface and deprecate IncludeMetadata. Then we could remove IncludeMetadata in an eventual 2.0 release.

We could lean on the Experimental flag to deprecate IncludeMetadata now, add a new configuration interface, and remove IncludeMetadata after 1.0.

TylerHelmuth avatar Feb 26 '24 21:02 TylerHelmuth

Related: client lists client.Info.Metadata as experimental from the same PR. Whether or not we stick with the IncludeMetadata config interface I think client.Info.Metadata is here to stay. I think we could safely drop the Experimental warning from that struct.

TylerHelmuth avatar Feb 26 '24 21:02 TylerHelmuth

I would vote for stabilizing the client module before we decide on this

mx-psi avatar Feb 29 '24 11:02 mx-psi

We (observIQ) are using this and have a customer using it as well. It would be great if include_metadata could be kept around.

jsirianni avatar May 09 '24 17:05 jsirianni

Related: client lists client.Info.Metadata as experimental from the same PR. Whether or not we stick with the IncludeMetadata config interface I think client.Info.Metadata is here to stay. I think we could safely drop the Experimental warning from that struct.

I agree, since it's no longer experimental on client, it should no longer be experimental in confighttp and configgrpc.

atoulme avatar Jul 21 '24 07:07 atoulme