otelhttptrace: add TLS info to the "http.tls" span #5563
Conversation
|
Please don't change the semconv attributes, despite the deprecations. This work is happening in other PRs, as part of #5400. |
|
Ok, noted. How would you like me to proceed for the time being? Revert to 1.20 and manually enter the attribute keys? Is there any merit in the idea proposed? |
|
I think we could add the new attributes as string for now. |
|
Super, I'll make the changes and come back to you. Thanks! |
|
Sorry for the delay, hopefully that's more acceptable now! Let me know if you want anything else changing! |
instrumentation/net/http/httptrace/otelhttptrace/clienttrace.go
Outdated
Show resolved
Hide resolved
instrumentation/net/http/httptrace/otelhttptrace/clienttrace.go
Outdated
Show resolved
Hide resolved
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5563 +/- ##
=====================================
Coverage 63.2% 63.3%
=====================================
Files 194 194
Lines 11983 11990 +7
=====================================
+ Hits 7585 7592 +7
Misses 4181 4181
Partials 217 217
|
This makes all the files in this package import the same version.
dotwaffle
force-pushed
the
otelhttptrace_tls_attributes
branch
from
18d1675
to
3469bf8
Compare
instrumentation/net/http/httptrace/otelhttptrace/clienttrace.go
Outdated
Show resolved
Hide resolved
instrumentation/net/http/httptrace/otelhttptrace/httptrace_test.go
Outdated
Show resolved
Hide resolved
instrumentation/net/http/httptrace/otelhttptrace/test/clienttrace_test.go
Outdated
Show resolved
Hide resolved
| @@ -69,16 +70,16 @@ func TestRoundtrip(t *testing.T) { | |||
| defer ts.Close() | |||
|
|
|||
| address := ts.Listener.Addr() | |||
| hp := strings.Split(address.String(), ":") | |||
| host, port, _ := net.SplitHostPort(address.String()) | |||
There was a problem hiding this comment.
| host, port, _ := net.SplitHostPort(address.String()) | |
| host, port, err := net.SplitHostPort(address.String()) | |
| if err != nil { | |
| t.Fatalf("Address splitting failed: %s", err.Error()) | |
| } |
| ) | ||
| defer tsHTTPS.Close() | ||
|
|
||
| for _, ts := range []*httptest.Server{tsHTTP, tsHTTPS} { |
There was a problem hiding this comment.
If we're taking the approach of subtests, I think it would be better to loop through anonymous structs with a createServer method which creates the proper (HTTP/HTTPS) server, rather than initializing them both.
| panic("unexpected error in parsing httptest server URL: " + err.Error()) | ||
| } | ||
| // http.tls only exists on HTTPS connections. | ||
| if u.Scheme == "https" { |
There was a problem hiding this comment.
There shouldn't be conditions like this in a subtest loop. These should be part of the attributes defined in the loop definition.
MrAlias
added
the
response needed
I noticed that the
http.tlsspan was not including any useful information for otelhttptrace, and considering httptrace is supposed to show excess debug information about an HTTP connection, I figured it would be good to add some simple attributes on that span.When committing, I noticed that the golangci-lint checks were failing because of some deprecated semconv functions, namely
semconv.NetHostName(), so I fixed them to what those values they set are to eliminate the linter failures. It looks likenet.host.namehas been replaced withserver.addressin semconv, but I didn't want to break existing code -- happy to change them to the new semconv version if requested!