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

[component] TelemetrySettings.MetricsLevel is experimental

Open mx-psi opened this issue 1 year ago • 8 comments
trafficstars

The MetricsLevel field is marked as experimental https://pkg.go.dev/go.opentelemetry.io/collector/component#TelemetrySettings. We should decide what to do with it (stabilize it, remove it, change it in some way)

mx-psi avatar Feb 07 '24 09:02 mx-psi

I expected TelemetrySettings to eventually include a LoggerProvider once the logging bridge API is implemented in otel-go. This makes me wonder if TelemetrySettings should be moved out of component

codeboten avatar Feb 07 '24 16:02 codeboten

I expected TelemetrySettings to eventually include a LoggerProvider once the logging bridge API is implemented in otel-go.

I had assumed we would keep on using *zap.Logger with a zapcore.Core that uses the LoggerProvider behind the scenes.

This makes me wonder if TelemetrySettings should be moved out of component

But I am in favor of this regardless, I am just not sure where it should go (service/telemetry implies a dependency of the components on service)

mx-psi avatar Feb 07 '24 17:02 mx-psi

I expected TelemetrySettings to eventually include a LoggerProvider once the logging bridge API is implemented in otel-go. This makes me wonder if TelemetrySettings should be moved out of component

Since we pass the TelemetrySettings in the start/creation of the components even if you move it, you depend on it so you have the same problem

bogdandrutu avatar Feb 07 '24 17:02 bogdandrutu

We should decide what to do with it (stabilize it, remove it, change it in some way)

I want to remove it, since it was used to configure "high cardinality" metrics in the past and we should have views and other configs for that.

bogdandrutu avatar Feb 07 '24 17:02 bogdandrutu

@codeboten Does the useOtelWithSDKConfigurationForInternalTelemetry feature gate still allow setting configtelemetry.Level? If so, I think we should remove its usage (at least once we add support for views).

It would also be interesting to tie ignoring this level in components that use it today with the useOtelWithSDKConfigurationForInternalTelemetry flag

mx-psi avatar Feb 08 '24 12:02 mx-psi

@codeboten Could you take a look at https://github.com/open-telemetry/opentelemetry-collector/issues/9510#issuecomment-1934046671 ?

mx-psi avatar Mar 06 '24 15:03 mx-psi

After discussing this at the 3-Apr-2024 SIG call, it was suggested that the use of a level is easier for users so we should keep it and remove the message about it being experimental.

More advanced use-cases will be able to enable all metrics and use views for maximum flexibility/control over what metrics are emitted.

codeboten avatar Apr 04 '24 16:04 codeboten

I think we should distinguish service.Telemetry.Config.MetricsConfig.Level from component.TelemetrySettings.MetricsLevel. We agreed to keep service.Telemetry.Config.MetricsConfig.Level to keep user configuration easy, but we discussed several options for component.TelemetrySettings.MetricsLevel:

  1. Keep as is
  2. Have several meter providers for the different metrics levels
  3. Allow components to define views in addition to metrics, and tie said views to levels

(2) could look something like:

type LeveledMeterProvider interface {
     MeterProvider(level configtelemetry.Level) metric.MeterProvider // noop/SDK provider depending on MetricsConfig.Level
}

or be at a lower level (when getting the meter or the instrument).

mx-psi avatar Apr 05 '24 09:04 mx-psi