Skip to content

Fix IPv6 parsing errors in semconv.NetAttributesFromHTTPRequest#2285

Merged
MrAlias merged 9 commits intoopen-telemetry:mainfrom
evantorrie:http_attribute_fix
Oct 18, 2021
Merged

Fix IPv6 parsing errors in semconv.NetAttributesFromHTTPRequest#2285
MrAlias merged 9 commits intoopen-telemetry:mainfrom
evantorrie:http_attribute_fix

Conversation

@evantorrie
Copy link
Copy Markdown
Contributor

fixes #2283

@codecov
Copy link
Copy Markdown

codecov Bot commented Oct 13, 2021

Codecov Report

Merging #2285 (69b88ab) into main (43465e8) will increase coverage by 0.0%.
The diff coverage is 100.0%.

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #2285   +/-   ##
=====================================
  Coverage   72.6%   72.6%           
=====================================
  Files        172     172           
  Lines      11919   11898   -21     
=====================================
- Hits        8661    8646   -15     
+ Misses      3023    3019    -4     
+ Partials     235     233    -2     
Impacted Files Coverage Δ
semconv/v1.4.0/http.go 96.3% <100.0%> (+1.2%) ⬆️
...s/otlp/otlptrace/internal/connection/connection.go 16.2% <0.0%> (+1.5%) ⬆️

Standardize order of checks for IP, Name, Port
@garthk
Copy link
Copy Markdown
Contributor

garthk commented Oct 14, 2021

I don't recall touching any code relevant to TestNew_collectorConnectionDiesThenReconnects. Is that test flakey, by any chance, or do I need to dig deeper? Oh, sorry, wrong bug; I thought this was #2284. Thanks for picking this one up

Copy link
Copy Markdown
Contributor

@garthk garthk left a comment

Choose a reason for hiding this comment

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

I was expecting the implementation to shrink given net.SplitHostPort. Can we use it in the first line or two and take the fallback measures only if required—perhaps not at all? Under what circumstances would Go populate RemoteAddr without a port?

i.e. assume net.SplitHostPort(input) will succeed
Copy link
Copy Markdown
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

👍

@evantorrie
Copy link
Copy Markdown
Contributor Author

@garthk I took your advice and refactored the code to assume the happy path succeeds, i.e. has a combination of both host and port. I do use the same routine to extract the hostIP/hostName and port for both request.RemoteAddr and the various permutations of request.Host, Host header and request URL. That means it needs to be a bit more robust than just assuming the Go runtime has set these reasonably (especially with things like parsing the Host header which comes direct from the "untrusted" client).

In any case, it looks like all the new IPv6 tests are passing and there are 20 fewer lines of code in http.go after this PR.

@MrAlias MrAlias merged commit 1f4b606 into open-telemetry:main Oct 18, 2021
@pellared pellared added this to the untracked milestone Nov 8, 2024
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.

net.peer.ip not filled for IPv6

5 participants