-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
remove MetricsLevel
from TelemetrySettings
#11061
Comments
@bogdandrutu Are you going to submit any changes to the spec related to this? (Maybe related to open-telemetry/opentelemetry-specification/issues/4200?) |
Given that stabilization of |
Summary of current state: open-telemetry/opentelemetry-specification/issues/4256 and open-telemetry/opentelemetry-specification/issues/4207 are blocking stabilization in the spec. There is a PR for 4256: open-telemetry/opentelemetry-specification/pull/4262 that is blocked by adding a prototype. Nobody is working on a prototype. For 4207, things seem to be moving forward. This is one of the last issues blocking component 1.0. I have been focusing on #11499 and related issues in case it changes the roadmap and this becomes less important, but given the state above, we may want to go with the approach of "remove MetricsLevel and use feature gates for current use cases"/ |
There is also now an OTEP related to one of the issues above: open-telemetry/opentelemetry-specification/pull/4290 |
We discussed open-telemetry/opentelemetry-specification/issues/4256 at the 2024-11-19 spec meeting. We will close it on 2024-12-03 unless there are use case brought up based on open-telemetry/opentelemetry-specification#4256 (comment). This also would mean that open-telemetry/opentelemetry-specification#4207 is not blocking stabilization, so we may (fingers crossed) be able to merge the stabilization PR. |
This will be unblocked by open-telemetry/opentelemetry-go/issues/6002 |
Filed the following PRs for this:
I am working on other PRs, including:
|
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description <!-- Issue number if applicable --> Removes `telemetry::level`, which is unused. If this was intended to serve as the 'minimum level' for a component, I can handle that on #12143. This is a breaking change, but since the field was effectively dead I think that's fine. #### Link to tracking issue Updates #11061
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description <!-- Issue number if applicable --> Drops metrics that depend on the metrics level: - Batch processor metric - otelarrow metrics (see open-telemetry/otel-arrow/issues/280 for limitation). - internal/otelarrow/netstats metrics. I did not implement https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/a25f058256e8339e49e4c89ac622a9ef47b52334/internal/otelarrow/netstats/netstats.go#L133-L136 since `LevelNone` drops all metrics. This attemps to unblock #11601 by hardcoding the metrics here since there is a small number of them. Once we do #11754 we can move this back to the individual components #### Link to tracking issue Updates #11061
…#12126) <!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description Drops `otelhttp` metrics depending of level with a view instead of passing a noop `MeterProvider` ~~I am not sure if this would have a performance impact or not.~~ Checked with Damien, this should not have any visible impact. <!-- Issue number if applicable --> #### Link to tracking issue Updates #11061
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description <!-- Issue number if applicable --> Removes `level` field. This will be added back in the future once #11754 is completed. #### Link to tracking issue Updates #11061
…#12158) <!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description <!-- Issue number if applicable --> Same as #12126, but for `otelgrpc`: drops `otelgrpc` metrics depending of level with a view instead of passing a noop `MeterProvider` #### Link to tracking issue Updates #11061
A discussion on 4-Sept-2024 lead to the agreement that the struct field
MetricsLevel
will be removed fromTelemetrySettings
. This field is there to allow component authors to check a level before deciding whether to incur the cost of calculating an expensive operation.The alternative for component authors will be to check whether an instrument is enabled or not prior to calculating the metric. This is dependent on the PR in otel go to implement this and on the spec to stabilize this feature.
The decision was that if the change would take too long to implement upstream, then we would remove this field and move any component to configure their own metric level instead. This will allow the unblocking of the component module stabilization work.
The text was updated successfully, but these errors were encountered: