From 693d3d4cea18adcbe59225b906c0f6fea5eca372 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ivan=20Miri=C4=87?= Date: Wed, 22 Jul 2020 12:03:01 +0200 Subject: [PATCH] Fix wrong name tag for HTTP metrics from redirects 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. --- js/modules/k6/http/request_test.go | 25 ++++++++++++----------- lib/netext/httpext/request.go | 14 ++----------- lib/netext/httpext/transport.go | 32 ++++++++++++++++++++++++++++-- 3 files changed, 45 insertions(+), 26 deletions(-) diff --git a/js/modules/k6/http/request_test.go b/js/modules/k6/http/request_test.go index 973c72933bf..487d5081f4e 100644 --- a/js/modules/k6/http/request_test.go +++ b/js/modules/k6/http/request_test.go @@ -181,7 +181,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) { @@ -628,7 +628,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, "", ) @@ -655,7 +655,7 @@ func TestRequestAndBatch(t *testing.T) { stats.GetBufferedSamples(samples), "GET", sr("HTTPSBIN_URL/cookies"), - sr("HTTPSBIN_URL/cookies/set?key=value"), + "", 200, "", ) @@ -702,7 +702,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, "", ) @@ -828,7 +828,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" }); @@ -839,9 +840,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") @@ -1912,12 +1913,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, "") } diff --git a/lib/netext/httpext/request.go b/lib/netext/httpext/request.go index 6e814429164..95a1cc2b2f4 100644 --- a/lib/netext/httpext/request.go +++ b/lib/netext/httpext/request.go @@ -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 { @@ -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 != "" { diff --git a/lib/netext/httpext/transport.go b/lib/netext/httpext/transport.go index 3950454ebe3..ad9e09bccfe 100644 --- a/lib/netext/httpext/transport.go +++ b/lib/netext/httpext/transport.go @@ -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 @@ -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, } } @@ -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) @@ -204,5 +230,7 @@ func (t *transport) RoundTrip(req *http.Request) (*http.Response, error) { err: err, }) + t.numProcReqs++ + return resp, err }