Gate PeriodicMetricReader self-observability by internal telemetry version#8597
Conversation
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8597 +/- ##
============================================
+ Coverage 91.62% 91.63% +0.01%
- Complexity 10321 10325 +4
============================================
Files 1013 1013
Lines 27287 27295 +8
Branches 3203 3203
============================================
+ Hits 25001 25012 +11
+ Misses 1559 1558 -1
+ Partials 727 725 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
I'm ok with the API change in this PR |
jack-berg
left a comment
There was a problem hiding this comment.
My position has been that we should only add the setInternalTelemetryVersion once a component supports multiple telemetry versions. And if a component only supports a single telemetry version like LATEST and the resolved version is LEGACY, then we set the meter provider to a noop. For example:
This accomplishes the same thing but avoids adding public API surface area which may imply that the component supports different telemetry versions when it doesn't. A debateable win since it also means we have inconsistent APIs across our components, some with setInternalTelemetryVersion, some without.
PeriodicMetricReader is a special case though because it does not have a setMeterProvider method, and so there is no way to disable internal telemetry when the version is set to LEGACY.
That's why this PR makes sense. And now I'm warming up to going further and adding setInternalTelemetryVersion on all components, and documenting the versions supported by each component in javadoc. Of course, we would do that in a separate PR.
A couple of minor comments, but looks good. Thanks!
| * | ||
| * @since 1.65.0 |
There was a problem hiding this comment.
| * | |
| * @since 1.65.0 |
These get added as part of the release flow. Better to omit them at this stage because some PRs take longer than expected and its easy to forget to update the version.
| InternalTelemetryVersion telemetryVersion = InternalTelemetryConfiguration.getVersion(config); | ||
| MetricReader reader = | ||
| PeriodicMetricReader.builder(customizedMetricExporter) | ||
| .setInternalTelemetryVersion(telemetryVersion) |
There was a problem hiding this comment.
Can you do the equivalent in MetricReaderFactory as well? Check out LoggerProviderFactory for a reference on how to resolve / set internal telemetry version in declarative configuration.
|
This PR has review comments. Review suggestions, whether from maintainers or automated reviewers, aren't always correct or required. Please evaluate each comment on its merits, then make sure each thread has a clear outcome. For example, link to the commit if you applied a suggestion, explain why it wasn't applied, or ask a follow-up question. Automation flags a PR for human review once every review thread has a reply or is marked as resolved. Status across open PRs is visible on the pull request dashboard. |
What was changed
PeriodicMetricReaderwith anInternalTelemetryVersion.MetricExporterConfigurationto propagate the configured telemetry version when creatingPeriodicMetricReader.PeriodicMetricReaderBuildertoInternalTelemetryVersion.LATEST.Why?
PeriodicMetricReaderalways emittedotel.sdk.metric_reader.collection.durationwhenever a realMeterProviderwas available, regardless ofOTEL_EXPERIMENTAL_SDK_TELEMETRY_VERSION.Tracing and logging already gate SDK self-observability using
InternalTelemetryConfiguration.getVersion(config). This change aligns metric reader self-observability with the existing behavior while preserving the default behavior for direct SDK users.Fixes #8566