Skip to content

Commit

Permalink
Fix wrong name tag for HTTP metrics from redirects
Browse files Browse the repository at this point in the history 
Closes #1474

The root cause was reusing the same 'name' tag for all requests
processed by a transport. In the case of redirects and digest auth the
transport would process more than one request and needs to update the
'name' tag in the same way as 'url' is, but only for the initial
request, as subsequent ones will by dynamically generated.
  • Loading branch information
Ivan Mirić committed Sep 4, 2020
1 parent 65372c5 commit 077ee7f
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 26 deletions.
25 changes: 13 additions & 12 deletions js/modules/k6/http/request_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ func TestRequestAndBatch(t *testing.T) {
}

assert.Equal(t, 10, reqsCount)
assertRequestMetricsEmitted(t, bufSamples, "GET", sr("HTTPBIN_URL/get"), sr("HTTPBIN_URL/redirect/9"), 200, "")
assertRequestMetricsEmitted(t, bufSamples, "GET", sr("HTTPBIN_URL/get"), sr("HTTPBIN_URL/get"), 200, "")
})

t.Run("10", func(t *testing.T) {
Expand Down Expand Up @@ -651,7 +651,7 @@ func TestRequestAndBatch(t *testing.T) {
stats.GetBufferedSamples(samples),
"GET",
sr("HTTPSBIN_URL/set-cookie-without-redirect"),
sr("HTTPBIN_URL/redirect-to?url=HTTPSBIN_URL/set-cookie-without-redirect"),
"",
200,
"",
)
Expand All @@ -678,7 +678,7 @@ func TestRequestAndBatch(t *testing.T) {
stats.GetBufferedSamples(samples),
"GET",
sr("HTTPSBIN_URL/cookies"),
sr("HTTPSBIN_URL/cookies/set?key=value"),
"",
200,
"",
)
Expand Down Expand Up @@ -725,7 +725,7 @@ func TestRequestAndBatch(t *testing.T) {
stats.GetBufferedSamples(samples),
"GET",
sr("HTTPBIN_IP_URL/get"),
sr("HTTPBIN_IP_URL/redirect-to?url=HTTPSBIN_URL/set-cookie-and-redirect"),
"",
200,
"",
)
Expand Down Expand Up @@ -851,7 +851,8 @@ func TestRequestAndBatch(t *testing.T) {
t.Run("digest", func(t *testing.T) {
t.Run("success", func(t *testing.T) {
url := sr("http://bob:pass@HTTPBIN_IP:HTTPBIN_PORT/digest-auth/auth/bob/pass")
urlExpected := sr("http://****:****@HTTPBIN_IP:HTTPBIN_PORT/digest-auth/auth/bob/pass")
urlMasked := sr("http://****:****@HTTPBIN_IP:HTTPBIN_PORT/digest-auth/auth/bob/pass")
urlRaw := sr("HTTPBIN_IP_URL/digest-auth/auth/bob/pass")

_, err := common.RunString(rt, fmt.Sprintf(`
var res = http.request("GET", "%s", null, { auth: "digest" });
Expand All @@ -862,9 +863,9 @@ func TestRequestAndBatch(t *testing.T) {

sampleContainers := stats.GetBufferedSamples(samples)
assertRequestMetricsEmitted(t, sampleContainers[0:1], "GET",
sr("HTTPBIN_IP_URL/digest-auth/auth/bob/pass"), urlExpected, 401, "")
urlMasked, "", 401, "")
assertRequestMetricsEmitted(t, sampleContainers[1:2], "GET",
sr("HTTPBIN_IP_URL/digest-auth/auth/bob/pass"), urlExpected, 200, "")
urlRaw, "", 200, "")
})
t.Run("failure", func(t *testing.T) {
url := sr("http://bob:pass@HTTPBIN_IP:HTTPBIN_PORT/digest-auth/failure")
Expand Down Expand Up @@ -1934,12 +1935,12 @@ func TestDigestAuthWithBody(t *testing.T) {
`, urlWithCreds))
require.NoError(t, err)

expectedURL := tb.Replacer.Replace(
"http://HTTPBIN_IP:HTTPBIN_PORT/digest-auth-with-post/auth/testuser/testpwd")
expectedName := tb.Replacer.Replace(
urlMasked := tb.Replacer.Replace(
"http://****:****@HTTPBIN_IP:HTTPBIN_PORT/digest-auth-with-post/auth/testuser/testpwd")
urlRaw := tb.Replacer.Replace(
"http://HTTPBIN_IP:HTTPBIN_PORT/digest-auth-with-post/auth/testuser/testpwd")

sampleContainers := stats.GetBufferedSamples(samples)
assertRequestMetricsEmitted(t, sampleContainers[0:1], "POST", expectedURL, expectedName, 401, "")
assertRequestMetricsEmitted(t, sampleContainers[1:2], "POST", expectedURL, expectedName, 200, "")
assertRequestMetricsEmitted(t, sampleContainers[0:1], "POST", urlMasked, "", 401, "")
assertRequestMetricsEmitted(t, sampleContainers[1:2], "POST", urlRaw, "", 200, "")
}
14 changes: 2 additions & 12 deletions lib/netext/httpext/request.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -232,20 +232,10 @@ func MakeRequest(ctx context.Context, preq *ParsedHTTPRequest) (*Response, error
}

tags := state.CloneTags()
// Override any global tags with request-specific ones.
for k, v := range preq.Tags {
tags[k] = v
}
if state.Options.SystemTags.Has(stats.TagMethod) {
tags["method"] = preq.Req.Method
}
if state.Options.SystemTags.Has(stats.TagURL) {
tags["url"] = preq.URL.Clean()
}

// Only set the name system tag if the user didn't explicitly set it beforehand
if _, ok := tags["name"]; !ok && state.Options.SystemTags.Has(stats.TagName) {
tags["name"] = preq.URL.Name
}

// Check rate limit *after* we've prepared a request; no need to wait with that part.
if rpsLimit := state.RPSLimit; rpsLimit != nil {
Expand All @@ -254,7 +244,7 @@ func MakeRequest(ctx context.Context, preq *ParsedHTTPRequest) (*Response, error
}
}

tracerTransport := newTransport(ctx, state, tags)
tracerTransport := newTransport(ctx, state, tags, preq.URL)
var transport http.RoundTripper = tracerTransport

if state.Options.HTTPDebug.String != "" {
Expand Down
32 changes: 30 additions & 2 deletions lib/netext/httpext/transport.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,11 @@ type transport struct {

lastRequest *unfinishedRequest
lastRequestLock *sync.Mutex
// Number of processed requests. It will be >1 when processing
// redirects, digest auth, etc.
numProcReqs uint16
// Original parsed URL, required for tagging purposes.
origURL *URL
}

// unfinishedRequest stores the request and the raw result returned from the
Expand Down Expand Up @@ -76,12 +81,14 @@ func newTransport(
ctx context.Context,
state *lib.State,
tags map[string]string,
origURL *URL,
) *transport {
return &transport{
ctx: ctx,
state: state,
tags: tags,
lastRequestLock: new(sync.Mutex),
origURL: origURL,
}
}

Expand Down Expand Up @@ -115,9 +122,28 @@ func (t *transport) measureAndEmitMetrics(unfReq *unfinishedRequest) *finishedRe
tags["status"] = "0"
}
} else {
cleanURL := URL{u: unfReq.request.URL, URL: unfReq.request.URL.String()}.Clean()
if enabledTags.Has(stats.TagURL) {
u := URL{u: unfReq.request.URL, URL: unfReq.request.URL.String()}
tags["url"] = u.Clean()
// Tag the first request with the original clean URL and
// subsequent ones (e.g. part of a redirect chain or digest auth)
// with the updated clean URL.
if t.origURL != nil && t.numProcReqs == 1 {
tags["url"] = t.origURL.Clean()
} else {
tags["url"] = cleanURL
}
}

// Only override the name tag if not specified by the user.
if _, ok := tags["name"]; !ok && enabledTags.Has(stats.TagName) {
if t.origURL != nil && t.numProcReqs == 1 {
tags["name"] = t.origURL.Name
} else {
tags["name"] = cleanURL
}
}
if enabledTags.Has(stats.TagMethod) {
tags["method"] = unfReq.request.Method
}
if enabledTags.Has(stats.TagStatus) {
tags["status"] = strconv.Itoa(unfReq.response.StatusCode)
Expand Down Expand Up @@ -204,5 +230,7 @@ func (t *transport) RoundTrip(req *http.Request) (*http.Response, error) {
err: err,
})

t.numProcReqs++

return resp, err
}

0 comments on commit 077ee7f

Please sign in to comment.