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

proposal: Enhance http.host tag handling #1236

Closed
ramsay opened this issue Apr 6, 2022 · 2 comments · Fixed by #1327
Closed

proposal: Enhance http.host tag handling #1236

ramsay opened this issue Apr 6, 2022 · 2 comments · Fixed by #1327
Assignees

Comments

@ramsay
Copy link

ramsay commented Apr 6, 2022

In contrib/net/http.go the http.host tag is set via the following code:

if r.URL.Host != "" {
	opts = append([]ddtrace.StartSpanOption{
		tracer.Tag("http.host", r.URL.Host),
	}, opts...)
}

But this is not 100% correct. The request.URL is the results of parsing the URI in the HTTP request which will usually be relative to the HTTP Host header. See the following comment from the go docs: https://pkg.go.dev/net/http@go1.18#Request

// URL specifies either the URI being requested (for server
// requests) or the URL to access (for client requests).
//
// For server requests, the URL is parsed from the URI
// supplied on the Request-Line as stored in RequestURI. For
// most requests, fields other than Path and RawQuery will be
// empty. (See RFC 7230, Section 5.3)
//
// For client requests, the URL's Host specifies the server to
// connect to, while the Request's Host field optionally
// specifies the Host header value to send in the HTTP
// request.
URL *url.URL

The code should check the result of request.URL.IsAbs() before deciding to use either request.Host or request.URL.Host

@ajgajg1134 ajgajg1134 added the ack label Jun 3, 2022
@ajgajg1134
Copy link
Contributor

Thanks for opening an issue!
Interesting, this does appear to be a bug in how we create the http.host tag. Is it worth checking for request.URL.IsAbs() at all? It feels like we can just always use request.Host if it's set since it should always be set on the server side. Or am I missing something in the docs you linked?

@ajgajg1134 ajgajg1134 self-assigned this Jun 3, 2022
@ramsay
Copy link
Author

ramsay commented Jun 4, 2022

You're correct, it should be sufficient to always use request.Host.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants