-
Notifications
You must be signed in to change notification settings - Fork 501
[SDK] Implement env var configuration for PeriodicExportingMetricReader #3549
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
[SDK] Implement env var configuration for PeriodicExportingMetricReader #3549
Conversation
✅ Deploy Preview for opentelemetry-cpp-api-docs canceled.
|
b0aea62
to
4c3e120
Compare
I changed the title to [SDK], because I use the [CONFIGURATION] tag is for everything related to declarative configuration (aka, config.yaml) |
sdk/include/opentelemetry/sdk/metrics/export/periodic_exporting_metric_reader_options.h
Outdated
Show resolved
Hide resolved
sdk/include/opentelemetry/sdk/metrics/export/periodic_exporting_metric_reader_options.h
Outdated
Show resolved
Hide resolved
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.
Thanks for the fix, see minor comments.
Looks like you found an existing bug. Please use The issue with 0 / infinite can be fixed separately. |
71bfae5
to
a1311d1
Compare
sdk/include/opentelemetry/sdk/metrics/export/periodic_exporting_metric_reader_options.h
Outdated
Show resolved
Hide resolved
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3549 +/- ##
==========================================
+ Coverage 89.99% 90.00% +0.02%
==========================================
Files 219 220 +1
Lines 7048 7056 +8
==========================================
+ Hits 6342 6350 +8
Misses 706 706
🚀 New features to boost your workflow:
|
The include-what-you-use failures can be seen in CI, go to "Summary" for the CI job, and download artifacts that contains the full logs. Search for Logs provided below
Also, there is a build break for tests on Windows. See existing tests for how to use setenv. |
c919c83
to
755345a
Compare
Thanks for your feedback, forgot to include env_variables.h for Windows build. |
755345a
to
34b1f06
Compare
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.
Thanks.
Build failure in windows CI:
To fix it:
Since a new patch is needed to fix this, I am adding minor nitpicks as well. Code looks good overall, will approve once CI is clean. |
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 overall.
Thanks for the fix, will approve once CI is clean.
34b1f06
to
0076eef
Compare
0076eef
to
2b44323
Compare
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.
Thanks for the fix, and the misc cleanup.
@mathieurobin1 Congratulations on your first patch. Merged. |
Fixes #3515
Changes
Implements the configuration of
PeriodicExportingMetricReader
via environment variables with default values.It introduces support for:
OTEL_METRIC_EXPORT_INTERVAL
(Duration)OTEL_METRIC_EXPORT_TIMEOUT
(Timeout)Known Limitation / Discussion Point:
During implementation, I identified that the existing utility function
common::GetDurationEnvironmentVariable
is not fully compliant with the specification forTimeout
types.Timeout
value of0
SHOULD be interpreted as "infinite".GetTimeoutFromString
) explicitly rejects a value of0
as invalid.To keep the scope of this PR focused on the
PeriodicExportingMetricReaderOptions
, I have used the existing utility for both environment variables. This means thatOTEL_METRIC_EXPORT_TIMEOUT=0
is currently not handled as an infinite timeout but will instead fall back to the default value.I believe fixing the common utility function should be addressed in a separate issue/PR to ensure it doesn't introduce unintended side effects elsewhere.
For significant contributions please make sure you have completed the following items:
CHANGELOG.md
updated for non-trivial changes