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.

Sign up for GitHub

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

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
35d3790
- fix how tls phase is computed
kgersen Mar 14, 2021
92febc8
- revert 'connect' phase duration
kgersen Mar 16, 2021
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
19 changes: 18 additions & 1 deletion prober/http.go
View file
Open in desktop
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 @@ -534,7 +551,7 @@ func ProbeHTTP(ctx context.Context, target string, module config.Module, registr
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())
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.

} else {
durationGaugeVec.WithLabelValues("connect").Add(trace.gotConn.Sub(trace.dnsDone).Seconds())
}
Expand Down