refactor(otlp-exporter-base): use get*FromEnv() for otlp exporter config #5583
Conversation
Migrated away from using `process.env` to use the `getStringFromEnv`-function Signed-off-by: Weyert de Boer <wdb@innerfuse.biz>
…tFromEnv`-functions Signed-off-by: Weyert de Boer <wdb@innerfuse.biz>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5583 +/- ##
==========================================
- Coverage 95.03% 95.02% -0.02%
==========================================
Files 310 310
Lines 7952 7952
Branches 1605 1605
==========================================
- Hits 7557 7556 -1
- Misses 395 396 +1
🚀 New features to boost your workflow:
|
weyert
changed the title
Signed-off-by: Weyert de Boer <wdb@innerfuse.biz>
| @@ -78,11 +78,11 @@ export function testSharedConfigurationFromEnvironment( | |||
| sinon.assert.calledTwice(spyLoggerWarn); | |||
| sinon.assert.calledWithExactly( | |||
| spyLoggerWarn, | |||
| 'Configuration: OTEL_EXPORTER_OTLP_METRICS_TIMEOUT is invalid, expected number greater than 0 (actual: NaN)' | |||
There was a problem hiding this comment.
I would appreciate some guidance for the maintainer how to go about this. The getStringFromEnv -function itself outputs this message and then shared-env-configuration.ts tries to log a warning in one situation too. For now I have updated the check to use the warning created by the getStringFromEnv -function.
I am not sure if that is what we want, though.
There was a problem hiding this comment.
I'm assuming that if you didn't make this update, the test was failing. Is that correct?
There was a problem hiding this comment.
@maryliag Yes, that's correct. My understanding is because of the warn call in get*FromEnv() function
There was a problem hiding this comment.
Yes, you are correct on what is happening. And it is fine, IMO. The specific format of warning messages are not a promised interface.
maryliag
changed the title
| if (Number.isFinite(definedTimeout) && definedTimeout > 0) { | ||
| return definedTimeout; | ||
| const envTimeout = getNumberFromEnv(timeoutEnvVar); | ||
| if (envTimeout) { |
There was a problem hiding this comment.
I believe this will accidentally not go through this if-statement if the envvar value is '0':
> process.env.FOO
'0'
> core.getNumberFromEnv('FOO')
0
> delete process.env.FOO
true
> process.env.FOO
undefined
> core.getNumberFromEnv('FOO')
undefined
The side-effect will be that OTEL_EXPORTER_OTLP_METRICS_TIMEOUT=0 will not result in the diag.warn(...) message.
| if (envTimeout) { | |
| if (envTimeout !== undefined) { |
| const compression = getStringFromEnv(compressionEnvVar)?.trim(); | ||
| if (compression === '') { | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
This can be simplified. getStringFromEnv will handle trimming and returning undefined if the envvar is an empty string.
| const compression = getStringFromEnv(compressionEnvVar)?.trim(); | |
| if (compression === '') { | |
| return undefined; | |
| } | |
| const compression = getStringFromEnv(compressionEnvVar); |
| @@ -78,11 +78,11 @@ export function testSharedConfigurationFromEnvironment( | |||
| sinon.assert.calledTwice(spyLoggerWarn); | |||
| sinon.assert.calledWithExactly( | |||
| spyLoggerWarn, | |||
| 'Configuration: OTEL_EXPORTER_OTLP_METRICS_TIMEOUT is invalid, expected number greater than 0 (actual: NaN)' | |||
There was a problem hiding this comment.
Yes, you are correct on what is happening. And it is fine, IMO. The specific format of warning messages are not a promised interface.
| @@ -96,7 +96,7 @@ export function testSharedConfigurationFromEnvironment( | |||
| sinon.assert.calledTwice(spyLoggerWarn); | |||
| sinon.assert.calledWithExactly( | |||
| spyLoggerWarn, | |||
| 'Configuration: OTEL_EXPORTER_OTLP_METRICS_TIMEOUT is invalid, expected number greater than 0 (actual: -Infinitiy)' | |||
| `Unknown value '-Infinitiy' for OTEL_EXPORTER_OTLP_METRICS_TIMEOUT, expected a number, using defaults` | |||
There was a problem hiding this comment.
I'm surprised at this one. I would expect getNumberFromEnv to pass on '-Infinity':
> process.env.FOO = '-Infinity'
'-Infinity'
> core.getNumberFromEnv('FOO')
-Infinity
Co-authored-by: Trent Mick <trentm@gmail.com>
Which problem is this PR solving?
Migrated away from using
process.envto use thegetStringFromEnv-functionFixes #5562
Short description of the changes
Replaced the usage of
process.envwith the built-in getEnv utility functionsType of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist: