refactor(otlp-grpc-exporter-base): use getStringFromEnv instead of process.env
#5595
Conversation
Updates the code to user the helper utilities to retrieve environment variable values fixes open-telemetry#5560 Signed-off-by: Weyert de Boer <wdb@innerfuse.biz>
Signed-off-by: Weyert de Boer <wdb@innerfuse.biz>
|
Hi @weyert, Thanks for your contribution. Please resolve the conflict and I think we're good to go :) |
david-luna
changed the title
getStringFromEnv instead of process.envgetStringFromEnv instead of process.env
…b.com/weyert/opentelemetry-js into contribfest-exporter-otlp-grpc
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5595 +/- ##
=======================================
Coverage 95.03% 95.03%
=======================================
Files 310 310
Lines 7952 7952
Branches 1605 1605
=======================================
Hits 7557 7557
Misses 395 395
🚀 New features to boost your workflow:
|
@david-luna that should be resolved now |
| return fallbackIfNullishOrBlank( | ||
| specificEndpoint?.trim(), | ||
| nonSpecificEndpoint?.trim() | ||
| ); |
There was a problem hiding this comment.
I think this whole block could be simplified to (untested, and needs npm run lint:fix for formatting):
return getStringFromEnv(`OTEL_EXPORTER_OTLP_${signalIdentifier}_ENDPOINT`) ?? getStringFromEnv('OTEL_EXPORTER_OTLP_ENDPOINT');i.e. .trim() and fallbackIfNullishOrBlank should no longer be necessary.
| ] | ||
| ?.toLowerCase() | ||
| .trim(); | ||
| )?.toLowerCase(); | ||
|
|
||
| return ( |
There was a problem hiding this comment.
I think this whole function could be simplified to (untested):
// Use `getStringFromEnv` because any value, even if invalid, in the signal-specific envvar should win.
return (getStringFromEnv(`OTEL_EXPORTER_OTLP_${signalIdentifier}_INSECURE`)
? getBooleanFromEnv(`OTEL_EXPORTER_OTLP_${signalIdentifier}_INSECURE`)
: getBooleanFromEnv('OTEL_EXPORTER_OTLP_INSECURE')
);And the long comment on block starting with It will allow the following values as {@code true} can be removed.
| const signalSpecificPath = process.env[signalSpecificEnvVar]?.trim(); | ||
| const nonSignalSpecificPath = process.env[nonSignalSpecificEnvVar]?.trim(); | ||
| const signalSpecificPath = getStringFromEnv(signalSpecificEnvVar); | ||
| const nonSignalSpecificPath = getStringFromEnv(nonSignalSpecificEnvVar); | ||
|
|
||
| const filePath = fallbackIfNullishOrBlank( |
There was a problem hiding this comment.
I think this could be simplified to (untested):
const filePath = getStringFromEnv(signalSpecificEnvVar) ?? getStringFromEnv(nonSignalSpecificEnvVar);Then, with this suggestion and those above, fallbackIfNullishOrBlank is now unused in this file and can be removed.
Which problem is this PR solving?
Updates the code to user the helper utilities to retrieve environment variable values
Fixes #5560
Short description of the changes
Switched from using
process.envto using the helper utilitygetStringFromEnvType of change
Please delete options that are not relevant.
How Has This Been Tested?
I have manually run the
testscript command in the package directoryChecklist: