fix(opentelemetry-instrumentation-http): handle null ports in options

[danielgblanco]

Which problem is this PR solving?

Handles null port when getting request info from options where host and port are undefined or null.

Fixes #2947

Short description of the changes

Asserts port is defined when appending to origin string returned.

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?

Unit tests and local testing with the case presented in #2947

Checklist:

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

[danielgblanco]

Thanks @blumamir, do you think a changelog entry is required for this?

[blumamir]

#2947

Yes, you need to add an entry in CHANGELOG.md file. You can see examples in other PRs

[codecov]

Codecov Report

Merging #2948 (78a16e9) into main (97bc632) will decrease coverage by 0.21%.
The diff coverage is n/a.

❗ Current head 78a16e9 differs from pull request most recent head 6bb46a9. Consider uploading reports for the commit 6bb46a9 to get more accurate results

@@            Coverage Diff             @@
##             main    #2948      +/-   ##
==========================================
- Coverage   90.36%   90.14%   -0.22%     
==========================================
  Files          68       68              
  Lines        1847     1847              
  Branches      394      394              
==========================================
- Hits         1669     1665       -4     
- Misses        178      182       +4     

Impacted Files	Coverage Δ
...ckages/opentelemetry-sdk-trace-web/karma.worker.js	0.00% <0.00%> (-100.00%)	⬇️

[danielgblanco]

Thanks, will do and push change soon.

[danielgblanco]

New machine, forgot to set up GPG keys. That's it in with a verified commit.

[blumamir]

Amazing, LGTM

Thanks for contributing this fix

[blumamir]

I looked again and found the following code in the function you fixed:

    if (options.port !== '') {
      optionsParsed.port = Number(options.port);
    }

I haven't tested it myself, but I guess if options.port is null, it will store Number(null) which is 0 into optionsParsed.port. Do you think it should be fixed here as well? looks like the relevant test does not assert the parsed port which is something that might also be addressed in this PR if you have motivation.

[danielgblanco]

Thanks. I see your point, although that check for port that's within the following block

if (options instanceof url.URL)

I can only assume that url.URL is correctly handled in the absence of port, but this case I've added is an edge case where options is neither a string nor a url.URL, which could be anything really. I think other possible attributes are accounted for, but there was an assumption that port would be there.

[blumamir]

Thanks. I see your point, although that check for port that's within the following block

if (options instanceof url.URL)

I can only assume that url.URL is correctly handled in the absence of port, but this case I've added is an edge case where options is neither a string nor a url.URL, which could be anything really. I think other possible attributes are accounted for, but there was an assumption that port would be there.

right 👍 thanks for the clarification

[dyladan]

LGTM % nit

[danielgblanco]

Thank you so much for picking this up! I've been on holiday and AFK but I really appreciate the review comments being addressed 👍