Skip to content
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

refactor(core): drop unnecessary assignment of HOSTNAME #4421

Merged
merged 5 commits into from
Jan 18, 2024

Conversation

pichlermarc
Copy link
Member

@pichlermarc pichlermarc commented Jan 16, 2024

Which problem is this PR solving?

We currently try to assign os.hostname() as the default, but we override it with the emtpy string from DEFAULT_ENVIRONMENT. This PR removes that assignment of HOSTNAME as it's not doing anything.

While we don't use HOSTNAME anywhere other than in tests, getEnv() may be used by consumers of the @opentelemetry/core library and they might depend on the default for that value being the empty string. Also since getEnv() indicates us getting environment variables, I think we should not fall back to anything that's not a static default (as is the case for all other variables).

Fixes #4405

Type of change

  • refactor

How Has This Been Tested?

  • Existing tests

@pichlermarc pichlermarc changed the title fix(core): drop unnecessary assignemnt of HOSTNAME fix(core): drop unnecessary assignment of HOSTNAME Jan 16, 2024
Copy link

codecov bot commented Jan 16, 2024

Codecov Report

Merging #4421 (0431b98) into main (bf8714e) will increase coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head 0431b98 differs from pull request most recent head 3141a92. Consider uploading reports for the commit 3141a92 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4421      +/-   ##
==========================================
+ Coverage   92.19%   92.21%   +0.02%     
==========================================
  Files         336      336              
  Lines        9512     9511       -1     
  Branches     2016     2016              
==========================================
+ Hits         8770     8771       +1     
+ Misses        742      740       -2     
Files Coverage Δ
...pentelemetry-core/src/platform/node/environment.ts 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

@pichlermarc pichlermarc added bug Something isn't working pkg:core priority:p4 Bugs and spec inconsistencies which do not fall into a higher prioritization labels Jan 16, 2024
@pichlermarc pichlermarc marked this pull request as ready for review January 16, 2024 15:50
@pichlermarc pichlermarc requested a review from a team January 16, 2024 15:50
CHANGELOG.md Outdated Show resolved Hide resolved
@pichlermarc pichlermarc changed the title fix(core): drop unnecessary assignment of HOSTNAME refactor(core): drop unnecessary assignment of HOSTNAME Jan 17, 2024
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

LGTM % a nit

CHANGELOG.md Outdated Show resolved Hide resolved
@legendecas legendecas merged commit 2a3c264 into open-telemetry:main Jan 18, 2024
18 checks passed
@pichlermarc pichlermarc deleted the fix/4405 branch January 18, 2024 06:36
Zirak pushed a commit to Zirak/opentelemetry-js that referenced this pull request Sep 14, 2024
…ry#4421)

* fix(core): drop unnecessary assignemnt of HOSTNAME

* fix(changelog): add changelog entry

* Update CHANGELOG.md

* fix(changelog): move entry to internal
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working pkg:core priority:p4 Bugs and spec inconsistencies which do not fall into a higher prioritization
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HOSTNAME is blank when not present in environment
2 participants