Metrics now counts 300s as success

[jpfuentes2]

refs #88

[jpfuentes2]

Also, 300s are still logged as an error in the Error Set:

Below I GET http://google.com since it always redirects to https

echo "GET http://google.com" | ./vegeta attack -duration=1s -rate=1 -redirects=0 | ./vegeta report
Requests    [total]             1
Duration    [total, attack, wait]       21.246215ms, 0, 21.246215ms
Latencies   [mean, 50, 95, 99, max]     21.246215ms, 21.246215ms, 21.246215ms, 21.246215ms, 21.246215ms
Bytes In    [total, mean]           0, 0.00
Bytes Out   [total, mean]           0, 0.00
Success     [ratio]             0.00%
Status Codes    [code:count]            0:1
Error Set:
Get http://www.google.com/: stopped after 0 redirects

The reason it is added to the Error Set is because of the Redirects option. Either -redirects=0 here doesn't count as an error or there must be another flag to ignore it as an error.

Thoughts?

[tsenart]

I'd say you change the error returned by the CheckRedirect function to: errors.New("too many redirects") and in NewMetrics skip that error:

if result.Error != "" && result.Error != "too many redirects" {
    errorSet[result.Error] = struct{}{}
}

[jpfuentes2]

Any reason we shouldn't return nil for error from CheckRedirect if -timeouts=0? Less changes this way, I think:

a.client.CheckRedirect = func(_ *http.Request, via []*http.Request) error {
            if len(via) > n {
                return fmt.Errorf("stopped after %d redirects", n)
            }
            return nil
        }

to

a.client.CheckRedirect = func(_ *http.Request, via []*http.Request) error {
            if n > 0 && len(via) > n {
                return fmt.Errorf("stopped after %d redirects", n)
            }
            return nil
        }

[tsenart]

From the documentation:

// CheckRedirect specifies the policy for handling redirects.
// If CheckRedirect is not nil, the client calls it before
// following an HTTP redirect. The arguments req and via are
// the upcoming request and the requests made already, oldest
// first. If CheckRedirect returns an error, the Client's Get
// method returns both the previous Response and
// CheckRedirect's error (wrapped in a url.Error) instead of
// issuing the Request req.
//
// If CheckRedirect is nil, the Client uses its default policy,
// which is to stop after 10 consecutive requests.

As I understood it, you need to not follow redirects and accept 3XX responses as successes. In order not to follow redirects, we need to return an error on the first redirect processed by CheckRedirect, otherwise it will be followed.

[jpfuentes2]

Ah, sorry, I clearly didn't dig deep enough. Ok, I'll go this route, thanks.

[jpfuentes2]

Okay, I implemented your suggestion by using a custom error type and added a test which I proved failed, locally, before I made the change.

[tsenart]

I find this too verbose. I'd prefer ErrTooManyRedirects

[jpfuentes2]

Ok, will change.
On Thu, Nov 13, 2014 at 18:15 Tomás Senart notifications@github.com wrote:

In lib/attack.go:

@@ -29,6 +30,9 @@ var (
DefaultLocalAddr = net.IPAddr{IP: net.IPv4zero}
// DefaultTLSConfig is the default tls.Config an Attacker uses.
DefaultTLSConfig = &tls.Config{InsecureSkipVerify: true}
+

• // ErrRedirectedButNotFollowing is when the user specifies not to follow redirects but we received one

I find this too verbose. I'd prefer ErrTooManyRedirects

—
Reply to this email directly or view it on GitHub
https://github.com/tsenart/vegeta/pull/93/files#r20331482.

[jpfuentes2]

Followed your recommendations and rebased since the commits would be confusing backtracking.

[jpfuentes2]

One second! I just did a sanity check and I'm not sure you'll like the results:

echo "GET http://google.com" | ./vegeta attack -rate=1 -duration=1s -redirects=0 | ./vegeta report
Requests    [total]             1
Duration    [total, attack, wait]       802.321656ms, 0, 802.321656ms
Latencies   [mean, 50, 95, 99, max]     802.321656ms, 802.321656ms, 802.321656ms, 802.321656ms, 802.321656ms
Bytes In    [total, mean]           0, 0.00
Bytes Out   [total, mean]           0, 0.00
Success     [ratio]             0.00%
Status Codes    [code:count]            0:1
Error Set:

Would something like this confuse people if it's not successful but it's also not an error? It seems I did not fully think through this, sorry.

Notice, success ratio is 0%, it doesn't have the status code - which I'm not sure why since my metrics_test changes seem to show they exist and nothing is in the error set.

[tsenart]

Hmmm. I'll revert this. It's also my fault to merge PRs from my phone at
04:00 AM ;-)

On Fri, Nov 14, 2014, 04:07 Jacques Fuentes notifications@github.com
wrote:

One second! I just did a sanity check and I'm not sure you'll like the
results:

echo "GET http://google.com" | ./vegeta attack -rate=1 -duration=1s -redirects=0 | ./vegeta report
Requests [total] 1
Duration [total, attack, wait] 802.321656ms, 0, 802.321656ms
Latencies [mean, 50, 95, 99, max] 802.321656ms, 802.321656ms, 802.321656ms, 802.321656ms, 802.321656ms
Bytes In [total, mean] 0, 0.00
Bytes Out [total, mean] 0, 0.00
Success [ratio] 0.00%
Status Codes [code:count] 0:1
Error Set:

Would something like this confuse people if it's not successful but it's
also not an error? It seems I did not fully think through this, sorry.

—
Reply to this email directly or view it on GitHub
#93 (comment).

[jpfuentes2]

No worries, you should get some rest! If you come up with some good ideas for a strategy I will gladly implement it. Thanks for the hard work on this project.

I should probably say: I might have a few more things to add to vegeta as I'm currently building a tool very similar to https://github.com/newsapps/beeswithmachineguns except with your tool as the load tester. I'm hoping to have a basic release in the next few weeks and would like to put a thank you on the README for your awesome tool and speedy help!

[tsenart]

Thanks for the words :-) I'd be glad to know more about that project of yours sometime soon.
Regarding this change set, I'll think about a solution tomorrow. Now, bed.

[jpfuentes2]

Perhaps this is as simple as checking for the specific error in the func (a *Attacker) hit(tr Targeter, tm time.Time) *Result { and ignore the error if it is a ErrTooManyRedirects error? Then, it'd be considered a success and marked as such?

[tsenart]

Thinking a bit more about this made me realise we should actually distinguish between reaching a redirect limit and treating redirects as successes. Sorry for this back and forth. We need two different errors that signal these two cases returned from CheckRedirect.

[jpfuentes2]

Yeah, that was what I was originally thinking. Okay, what about -ignore-redirects which will never follow redirects and also considers 300s as success. If -ignore-directs=false then existing -redirects logic is preserved entirely?

[tsenart]

I'm trying to keep combinatorial flag explosion to a minimum. With that in mind, what do you think of giving --redirects=-1 a special meaning, which would be to not follow redirects?

[jpfuentes2]

So --redirects=-1 would not follow but consider success and --redirects=0 would follow but consider failure if got redirect?

[tsenart]

Precisely.

[jpfuentes2]

Hmm, if we can put a short description of the behavior in the help output I think this is fine. I'll start a new PR.

[tsenart]

Yes, this absolutely needs to be well documented. I'd also create a constant in the code which represents this special behaviour.

[jpfuentes2]

Will do. Expect something shortly :)