-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Add Temporality Preference to the OTLP Metrics Exporter. #4127
Add Temporality Preference to the OTLP Metrics Exporter. #4127
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #4127 +/- ##
=====================================
Coverage 83.3% 83.3%
=====================================
Files 181 181
Lines 13925 13945 +20
=====================================
+ Hits 11610 11628 +18
- Misses 2094 2096 +2
Partials 221 221
|
@@ -96,6 +98,7 @@ func getOptionsFromEnv() []GenericOption { | |||
WithEnvCompression("METRICS_COMPRESSION", func(c Compression) { opts = append(opts, WithCompression(c)) }), | |||
envconfig.WithDuration("TIMEOUT", func(d time.Duration) { opts = append(opts, WithTimeout(d)) }), | |||
envconfig.WithDuration("METRICS_TIMEOUT", func(d time.Duration) { opts = append(opts, WithTimeout(d)) }), | |||
WithEnvTemporalitySelector("METRICS_TEMPORALITY_PREFERENCE", func(s metric.TemporalitySelector) { opts = append(opts, WithTemporalitySelector(s)) }), |
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.
The only documentation which tells about OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE
support is CHANGELOG.md
. I think that the support of this env var should be documented in GoDoc for otlpmetricgrpc
and otlpmetrichttp
. It may be worth introducing an # Environment variables
section in doc.go
files.
Probably other env vars should be also documented as well, but if I recall correctly all/most of them are at least mentioned in option functions. I think we can start with documenting only OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE
and document other env vars in separate PR(s).
Related to: #2046
func deltaTemporality(ik metric.InstrumentKind) metricdata.Temporality { | ||
return metricdata.DeltaTemporality | ||
} | ||
func cumulativeTemporality(ik metric.InstrumentKind) metricdata.Temporality { | ||
return metricdata.CumulativeTemporality | ||
} | ||
func lowMemoryTemporality(ik metric.InstrumentKind) metricdata.Temporality { | ||
switch ik { | ||
case metric.InstrumentKindCounter, metric.InstrumentKindHistogram: | ||
return metricdata.DeltaTemporality | ||
} | ||
return metricdata.CumulativeTemporality | ||
} |
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.
nit: Can we add blank lines between the functions?
Side note: Have we ever considered using gofumpt
linter instead of gofmt
? 😉
@@ -402,6 +402,30 @@ func TestConfigs(t *testing.T) { | |||
assert.Equal(t, metricdata.DeltaTemporality, got(undefinedKind)) | |||
}, | |||
}, | |||
{ | |||
name: "WithTemporalityPreference", | |||
env: map[string]string{"OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE": "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.
Can we add test cases for all possible cases (delta
and cumulative
)?
I thought I had done this once before. I'm closing this one in favor of #4287 |
This adds processing of the Temporality Prefence Environment Variable to the OTLP Metrics Exporter.
This comes from the OTLP exporter portion of the spec.