-
Notifications
You must be signed in to change notification settings - Fork 772
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 support for OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE environment variable #4667
Add support for OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE environment variable #4667
Conversation
…or-Temporality-Preference
…or-Temporality-Preference
…or-Temporality-Preference
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #4667 +/- ##
==========================================
- Coverage 85.08% 85.07% -0.02%
==========================================
Files 314 314
Lines 12725 12747 +22
==========================================
+ Hits 10827 10844 +17
- Misses 1898 1903 +5
|
[InlineData("DELTA", MetricReaderTemporalityPreference.Delta)] | ||
public void TestTemporalityPreferenceConfiguration(string configValue, MetricReaderTemporalityPreference expectedTemporality) | ||
{ | ||
var configData = new Dictionary<string, string> { ["OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE"] = configValue }; |
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: Use the constant OtlpMetricExporterExtensions.OtlpMetricExporterTemporalityPreferenceEnvVarKey
here (I'm assuming the tests can see internals)
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.
This was done on purpose. If someone accidentally changes the environment variable key name in the src
file, then our tests should complain about it. If I use the const string
defined in the src
file then our tests would simply pass even when the value of the key is changed.
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.
I don't necessarily disagree with your logic, but it does break with all other envvar tests that I can recall seeing. Ex:
Lines 62 to 65 in e7f7cd6
Environment.SetEnvironmentVariable(OtlpExporterOptions.EndpointEnvVarName, "http://test:8888"); | |
Environment.SetEnvironmentVariable(OtlpExporterOptions.HeadersEnvVarName, "A=2,B=3"); | |
Environment.SetEnvironmentVariable(OtlpExporterOptions.TimeoutEnvVarName, "2000"); | |
Environment.SetEnvironmentVariable(OtlpExporterOptions.ProtocolEnvVarName, "http/protobuf"); |
I would probably go with the constant to be consistent and follow DRY principle. If someone does have a valid reason to change it they should only have to do it in one spot.
But I made this a "nit" comment because I figured you might disagree 🤣 Up to you!
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.
I think we should think about what do we want to test? If we want to test, that a user a can configure something by providing a value for a particular string in the config, then our tests shouldn't be using the same const as the src file. Our tests would be too dynamic for their own good. 😄
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.
LGTM with a CHANGELOG update
@@ -2,6 +2,12 @@ | |||
|
|||
## Unreleased | |||
|
|||
* Added support for configuring the metric exporter's temporality using the |
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.
Remove this todo as well. Can be a separate PR
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.
Good call out. I was thinking we should update that README once we have released a version of the package with this feature instead of doing that now.
Fixes #3756
Spec: https://github.com/open-telemetry/opentelemetry-specification/blob/v1.14.0/specification/metrics/sdk_exporters/otlp.md#additional-configuration
Changes
OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE
environment variablePlease provide a brief description of the changes here.
Merge requirement checklist
CHANGELOG.md
files updated for non-trivial changes