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

http probe - fix how the tls phase is calculated #758

Merged
merged 2 commits into from
Mar 18, 2021
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 21 additions & 5 deletions prober/http.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ package prober

import (
"context"
"crypto/tls"
"errors"
"fmt"
"io"
Expand Down Expand Up @@ -119,6 +120,8 @@ type roundTripTrace struct {
gotConn time.Time
responseStart time.Time
end time.Time
tlsStart time.Time
tlsDone time.Time
}

// transport is a custom transport keeping traces for each HTTP roundtrip.
Expand Down Expand Up @@ -202,6 +205,16 @@ func (t *transport) GotFirstResponseByte() {
defer t.mu.Unlock()
t.current.responseStart = time.Now()
}
func (t *transport) TLSHandshakeStart() {
t.mu.Lock()
defer t.mu.Unlock()
t.current.tlsStart = time.Now()
}
func (t *transport) TLSHandshakeDone(_ tls.ConnectionState, _ error) {
t.mu.Lock()
defer t.mu.Unlock()
t.current.tlsDone = time.Now()
}

// byteCounter implements an io.ReadCloser that keeps track of the total
// number of bytes it has read.
Expand Down Expand Up @@ -411,6 +424,8 @@ func ProbeHTTP(ctx context.Context, target string, module config.Module, registr
ConnectDone: tt.ConnectDone,
GotConn: tt.GotConn,
GotFirstResponseByte: tt.GotFirstResponseByte,
TLSHandshakeStart: tt.TLSHandshakeStart,
Copy link
Contributor

Choose a reason for hiding this comment

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

this got introduced in Go 1.8, which is fine, as we are already requiring at least Go 1.13

TLSHandshakeDone: tt.TLSHandshakeDone,
}
request = request.WithContext(httptrace.WithClientTrace(request.Context(), trace))

Expand Down Expand Up @@ -521,6 +536,8 @@ func ProbeHTTP(ctx context.Context, target string, module config.Module, registr
"connectDone", trace.connectDone,
"gotConn", trace.gotConn,
"responseStart", trace.responseStart,
"tlsStart", trace.tlsStart,
"tlsDone", trace.tlsDone,
"end", trace.end,
)
// We get the duration for the first request from chooseProtocol.
Expand All @@ -532,13 +549,12 @@ func ProbeHTTP(ctx context.Context, target string, module config.Module, registr
continue
}
if trace.tls {
// dnsDone must be set if gotConn was set.
durationGaugeVec.WithLabelValues("connect").Add(trace.connectDone.Sub(trace.dnsDone).Seconds())
durationGaugeVec.WithLabelValues("tls").Add(trace.gotConn.Sub(trace.dnsDone).Seconds())
} else {
durationGaugeVec.WithLabelValues("connect").Add(trace.gotConn.Sub(trace.dnsDone).Seconds())
durationGaugeVec.WithLabelValues("tls").Add(trace.tlsDone.Sub(trace.tlsStart).Seconds())
Copy link
Contributor

Choose a reason for hiding this comment

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

this is subtly changing the meaning of the tls phase, but I think it's OK since this reflects more accurately the duration of the TLS handshake. The existing method is measuring the handshake plus whatever time it takes to finish setting the connection up plus whatever time it takes to get to the point where the TLS handshake starts after DNS is done.

}

// actual connection - we could add a new phase between connectDone and gotConn
durationGaugeVec.WithLabelValues("connect").Add(trace.gotConn.Sub(trace.dnsDone).Seconds())

// Continue here if we never got a response from the server.
if trace.responseStart.IsZero() {
continue
Expand Down