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

fallback to URL.Host if Request.Host is empty #2661

Merged
merged 3 commits into from Mar 9, 2022
Merged

fallback to URL.Host if Request.Host is empty #2661

merged 3 commits into from Mar 9, 2022

Conversation

nelzkiddom
Copy link
Contributor

The stdlib http.Request comments for the Host member variable states:

For client requests, Host optionally overrides the Host header to send. If empty, the Request.Write method uses the value of URL.Host. Host may contain an international domain name.

The semconv package(s) wasn't taking url.URL.Host into account when http.Request.Host was empty.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Mar 8, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: nelzkiddom / name: Nelz (68ff433)

@nelzkiddom
Copy link
Contributor Author

I found this behavior in the wild, when looking at requests that were created by the aws-sdk-go (v1).

(Notice there's no http.host entry.)

Screen Shot 2022-03-07 at 16 15 01

Copy link
Member

@Aneurysm9 Aneurysm9 left a comment

Choose a reason for hiding this comment

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

Thanks for this! Can you please add a CHANGELOG.md entry for this enhancement and apply it to the remaining semconv versions?

@nelzkiddom
Copy link
Contributor Author

nelzkiddom commented Mar 8, 2022

I made this change in the "current"(?) package of v1.7.0.

Proceeding forward for completeness sake, I can't tell which direction this should go in, as related to "versioning":

  • will I be told that this is a substantive change, and would require waiting for a new semconv/v1.7.1+ package?
  • will I be told that I should make this same change in all the "previous" packages, e.g. v1.6.1 and v1.5.0 and v1.4.0?

@nelzkiddom
Copy link
Contributor Author

As I typed that, I see now that @Aneurysm9 answered my question. I shall proceed! 🎉

@nelzkiddom
Copy link
Contributor Author

@Aneurysm9 Made the requested changes. 👍

@codecov
Copy link

codecov bot commented Mar 8, 2022

Codecov Report

Merging #2661 (08f1656) into main (0e4c156) will increase coverage by 0.0%.
The diff coverage is 100.0%.

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #2661   +/-   ##
=====================================
  Coverage   75.7%   75.8%           
=====================================
  Files        173     173           
  Lines      11664   11672    +8     
=====================================
+ Hits        8840    8854   +14     
+ Misses      2614    2608    -6     
  Partials     210     210           
Impacted Files Coverage Δ
semconv/v1.4.0/http.go 96.5% <100.0%> (+<0.1%) ⬆️
semconv/v1.5.0/http.go 96.5% <100.0%> (+<0.1%) ⬆️
semconv/v1.6.1/http.go 96.5% <100.0%> (+<0.1%) ⬆️
semconv/v1.7.0/http.go 96.5% <100.0%> (+<0.1%) ⬆️
exporters/jaeger/jaeger.go 91.1% <0.0%> (+0.8%) ⬆️
sdk/trace/batch_span_processor.go 82.1% <0.0%> (+1.8%) ⬆️

@MrAlias MrAlias merged commit 68e2495 into open-telemetry:main Mar 9, 2022
@nelzkiddom nelzkiddom deleted the nelz-host-fallback branch March 9, 2022 17:07
@MrAlias MrAlias added this to the Release v1.5.0 milestone Mar 15, 2022
@MrAlias MrAlias mentioned this pull request Mar 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants