Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[component] TelemetrySettings.MetricsLevel is experimental #9510

Closed
Tracked by #9376
mx-psi opened this issue Feb 7, 2024 · 8 comments · Fixed by #10912
Closed
Tracked by #9376

[component] TelemetrySettings.MetricsLevel is experimental #9510

mx-psi opened this issue Feb 7, 2024 · 8 comments · Fixed by #10912
Assignees
Labels
area:component collector-telemetry healthchecker and other telemetry collection issues release:required-for-ga Must be resolved before GA release

Comments

@mx-psi
Copy link
Member

mx-psi commented Feb 7, 2024

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 mx-psi added release:required-for-ga Must be resolved before GA release collector-telemetry healthchecker and other telemetry collection issues area:component labels Feb 7, 2024
@codeboten
Copy link
Contributor

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

@mx-psi
Copy link
Member Author

mx-psi commented Feb 7, 2024

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)

@bogdandrutu
Copy link
Member

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
Copy link
Member

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.

@mx-psi
Copy link
Member Author

mx-psi commented Feb 8, 2024

@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
Copy link
Member Author

mx-psi commented Mar 6, 2024

@codeboten Could you take a look at #9510 (comment) ?

@codeboten
Copy link
Contributor

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.

@mx-psi
Copy link
Member Author

mx-psi commented Apr 5, 2024

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 mx-psi moved this to Todo in Collector: v1 Apr 18, 2024
@mx-psi mx-psi added the help wanted Good issue for contributors to OpenTelemetry Service to pick up label Jul 17, 2024
@codeboten codeboten self-assigned this Jul 17, 2024
@codeboten codeboten moved this from Todo to In Progress in Collector: v1 Jul 30, 2024
@mx-psi mx-psi removed the help wanted Good issue for contributors to OpenTelemetry Service to pick up label Aug 21, 2024
@github-project-automation github-project-automation bot moved this from In Progress to Done in Collector: v1 Aug 29, 2024
@github-project-automation github-project-automation bot moved this from In Progress to Done in Collector: v1 Aug 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:component collector-telemetry healthchecker and other telemetry collection issues release:required-for-ga Must be resolved before GA release
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants