HTTPClientAttributesFromHTTPRequest always sets http.scheme to http

[gdavison]

Description

Calling HTTPClientAttributesFromHTTPRequest always sets the attribute http.scheme to http, even when the URL uses the https scheme.

(*SemanticConventions).httpBasicAttributesFromHTTPRequest checks for the presence of a value in request.TLS to set the value of http.scheme. However, this is not set on the client side, so will always be unset:

TLS allows HTTP servers and other software to record information about the TLS connection on which the request was received. This field is not filled in by ReadRequest. The HTTP server in this package sets the field for TLS-enabled connections before invoking a handler; otherwise it leaves the field nil. This field is ignored by the HTTP client.

Environment

• OS: macOS
• Architecture: amd64
• Go Version: 1.19
• opentelemetry-go version: v1.11.2

Steps To Reproduce

1. Create an HTTP request pointing to an HTTPS URL
2. Call HTTPClientAttributesFromHTTPRequest on the request
3. Check value of http.scheme

Expected behavior

http.scheme should be set to https.

[fatsheep9146]

Thanks for reporting this. @gdavison

I think that according to the description of http.scheme from https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/http.md#http-server-semantic-conventions

Attribute	Type	Description	Examples	Requirement Level
http.scheme	string	The URI scheme identifying the used protocol.	http; https	Required

Should we derive this field from request.URL?

For example

        
	if request.URL.Scheme == "https"  {
		attrs = append(attrs, sc.HTTPSchemeHTTPS)
	} else {
		attrs = append(attrs, sc.HTTPSchemeHTTP)
	}

@MrAlias

[MrAlias]

The changes introduced in #3499 align our implementation with the specification. We no longer add the http.schema attribute to HTTP client spans.

This attribute is still added to server spans. The most reliable way to detect if TLS was used for the request the server is serving is still to check if the TLS field is non-nil, which is still done.

The only thing that still needs to be addressed is the net.peer.port determination. We should use the request URL there to make a best effort in determining if the target port should be included or not.