-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Expose TemporalitySelector funcs #7346
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
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7346 +/- ##
=======================================
- Coverage 82.9% 82.8% -0.2%
=======================================
Files 267 268 +1
Lines 24982 24990 +8
=======================================
- Hits 20729 20698 -31
- Misses 3877 3916 +39
Partials 376 376
🚀 New features to boost your workflow:
|
@dprotaso question from the SIG, why not just expose the temporality selectors, what does the refactor/temporality preference type achieve |
Hey @akats7
Knative currently exposes a config map where users can set some OTel properties. Similar to how temporality preference can be configured using env vars we want our end-users to be able to use the same values supported by the otel-go SDK. Having these functions/values exposed means we don't need to copy stuff from the SDK into our code base. I've highlighted in slack that this has lead to incorrect documentation from different vendors when they're telling users to write their own temporality selectors. |
I think we can expose the functions, but probably can't expose the string values. Those are only part of the OTLP exporter spec, not the SDK spec. See https://github.com/open-telemetry/opentelemetry-specification/blob/1e04e1be0e17cae6d01c862049bbeb298e0ffa06/specification/metrics/sdk_exporters/otlp.md?plain=1#L50 |
Does that mean the strings should be in a different package? |
Yeah, they probably belong in the otelconf package eventually. |
What action should I take now then on this PR? |
I would split this into two PRs. One that exposes the functions, the other that exposes the strings, so both can be discussed/merged separately. |
I pulled the functions out into - #7434 |
// TemporalityPreference defines the user's desired temporality for metrics instruments. | ||
type TemporalityPreference string | ||
|
||
const ( | ||
// TemporalityPreferenceDefault indicates the SDK's default temporality should be used. | ||
TemporalityPreferenceDefault TemporalityPreference = "" | ||
|
||
// TemporalityPreferenceCumulative indicates a cumulative temporality should be used. | ||
TemporalityPreferenceCumulative TemporalityPreference = "cumulative" | ||
|
||
// TemporalityPreferenceDelta indicates a delta temporality should be used. | ||
TemporalityPreferenceDelta TemporalityPreference = "delta" | ||
|
||
// TemporalityPreferenceLowMemory indicates temporality preference that optimizes memory use. | ||
TemporalityPreferenceLowMemory TemporalityPreference = "lowmemory" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should not be added to the SDK. They duplicate existing configuration.
With the functions pulled out where should I put the functionality that maps the |
From: #7346 This PR exposes the default TemporalitySelector funcs that are used by the OTel env var processing. The reason to expose these funcs is because there is some discrepancy in various vendor documentation on how to enable these settings using go option args WithTemporalitySelector https://cloud-native.slack.com/archives/C01NPAXACKT/p1757443233624599 --------- Co-authored-by: Damien Mathieu <[email protected]> Co-authored-by: David Ashpole <[email protected]>
@dprotaso please see https://github.com/open-telemetry/opentelemetry-go/pull/7346/files#r2392819028 I do not plan to accept any additional overlapping configuration for temporarily in the SDK. If you would like too support resolution of the selector functions, look at adding in |
We discussed whether this even makes sense in otelconf, and we decided it probably does not. TemporalityPreference (the string constants) aren't part of the SDK spec. They are part of the declarative config spec for OTLP exporters, which would make the otelconf package a potential place for it to live, but we aren't ready right now to make subsections of the otelconf yaml parsing public right now. To start, we want to limit parsing to parsing the entire otelconf files. |
@dashpole should I close this out and just creating a tracking issue or something? |
yeah. Please create the tracking issue in opentelemetry-go-contrib |
This PR exposes the default
TemporalitySelector
funcs that are used by the OTel env var processing.The reason to expose these funcs is because there is some discrepancy in various vendor documentation on how to enable these settings using go option args
WithTemporalitySelector
https://cloud-native.slack.com/archives/C01NPAXACKT/p1757443233624599