otelhttp: Correctly set url.scheme attr in http client metrics.
#6938
Conversation
wzy9607
force-pushed
the
fix/clientscheme
branch
from
80ab9e0 to
0a5a380
Compare
| @@ -510,7 +510,7 @@ func (n CurrentHTTPClient) MetricAttributes(req *http.Request, statusCode int, a | |||
| attributes = append(attributes, | |||
| semconvNew.HTTPRequestMethodKey.String(standardizeHTTPMethod(req.Method)), | |||
| semconvNew.ServerAddress(requestHost), | |||
| n.scheme(req.TLS != nil), | |||
| n.scheme(req.URL), | |||
There was a problem hiding this comment.
How about passing req here, so the method can fallback to the req.TLS value, and then http if the URL isn't set?
There was a problem hiding this comment.
If we're passing the request directly we do also get the ability to account for proxies by checking the headers. Also leads to the question though of if a proxy or alb terminates TLS, whats the correct scheme to set.
There was a problem hiding this comment.
Also leads to the question though of if a proxy or alb terminates TLS, whats the correct scheme to set.
Let's leave that out for now. The semantic conventions also don't answer this clearly.
I have opened open-telemetry/semantic-conventions#2009
There was a problem hiding this comment.
I have added fallback to req.TLS.
prefer `req.URL.Scheme` to `req.TLS`, since `req.TLS` is ignored by http.Client
wzy9607
force-pushed
the
fix/clientscheme
branch
from
0a5a380 to
ebdf055
Compare
The current method of deciding
url.schemebyreq.TLSis incorrect, becausereq.TLSis ignored byhttp.Client.https://pkg.go.dev/net/http#Request
Therfore the current method will consider every request as http request, unless user manually set the otherwise useless
req.TLSfield.