Skip to content

Conversation

weyert
Copy link
Contributor

@weyert weyert commented Apr 6, 2025

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.env to using the helper utility getStringFromEnv

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

I have manually run the test script command in the package directory

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated

Updates the code to user the helper utilities to retrieve environment variable values

fixes open-telemetry#5560

Signed-off-by: Weyert de Boer <[email protected]>
@weyert weyert requested a review from a team as a code owner April 6, 2025 14:50
@david-luna
Copy link
Contributor

Hi @weyert,

Thanks for your contribution. Please resolve the conflict and I think we're good to go :)

@david-luna david-luna changed the title refactor: use getStringFromEnv instead of process.env refactor(otlp-grpc-exporter-base): use getStringFromEnv instead of process.env Apr 7, 2025
Copy link

codecov bot commented Apr 9, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.03%. Comparing base (b0e4e4f) to head (150fe77).
⚠️ Report is 198 commits behind head on main.

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           
Files with missing lines Coverage Δ
...e/src/configuration/otlp-grpc-env-configuration.ts 100.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@weyert
Copy link
Contributor Author

weyert commented Apr 9, 2025

Thanks for your contribution. Please resolve the conflict and I think we're good to go :)

@david-luna that should be resolved now

return fallbackIfNullishOrBlank(
specificEndpoint?.trim(),
nonSpecificEndpoint?.trim()
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

.trim();
)?.toLowerCase();

return (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 = getStringFromEnv(signalSpecificEnvVar);
const nonSignalSpecificPath = getStringFromEnv(nonSignalSpecificEnvVar);

const filePath = fallbackIfNullishOrBlank(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

@github-actions github-actions bot added the stale label Jul 21, 2025
@maryliag
Copy link
Contributor

Hi @weyert , are you still planning on working on this PR? Just smaller issues to update

@weyert
Copy link
Contributor Author

weyert commented Jul 21, 2025

Yes, let me have a look at these open PRs of mine during the week. Totally forgot about them

@github-actions github-actions bot removed the stale label Aug 11, 2025
Copy link

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

@github-actions github-actions bot added the stale label Oct 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[contribfest] use new get*FromEnv() functions in OTLP gRPC exporter env var configuration

5 participants