Core: Stop making multiple checks stop on first failing one

[slandelle]

When multiple checks are configured, if one fails, should the next ones be evaluated and get a chance to save data?

[notdryft]

Multiple checks are useful for debugging as they lessen the steps of validation you need to do to ... check your checks.

But, what if you want your checks to fail as soon as possible. You could also want to be able to recover from certain fails if they occur and not others.

What about a mode to choose between fast and "slow" fail ?

[slandelle]

Having a configurable behavior is an option, but then, we have to properly define what the default one would be, so that it suites the needs of most users. From the discussions on the ML, I'd tend to say that "evaluate all checks" should be the default.

[notdryft]

I agree on that.

[slandelle]

Evaluating all checks could help in 2 ways:

• saving as much information as possible
• reporting as much errors as possible

On the latter, the reports are not ready for that (display concatenated list in error block?, separated errors? but then, we'd have more errors than failing requests?).

This matter is important, and IMHO, the 2 points have to come together to make sense, so let's move this to M5 and come up with a proper solution.

[slandelle]

Moving this to 2.0 as this behavior change is big.
@pdalpra @nremond WDYT? Floris Krak was pushing for this on the mailing list.

[nremond]

I share Floris' point of view, but, having this on 2.0 only is fine with me.

[slandelle]

having this on 2.0 only is fine with me

WDYM? Not having this in RC1 as long as it's in final?

[nremond]

I'm an easy-going going man, as long as it will done, one day.

[pdalpra]

As discussed before, I think we should make it configurable, with "evaluate all checks", which is the way it works currently, being the default.
This way, we allow this new behaviour without breaking existing Gatling simulations.

[slandelle]

Actually, things are more complex.

Current behavior is:

• HTTP checks are sorted on their target (basically status < headers < body), so we don't honor the order the user set them up
• If one of the check fails, we ignore all the others, even the ones that were already evaluated, hence discarding data that could have been successfully captured

IMHO, we should:

• honor user defined check order (with request level first, then protocol level)
• honor all the captured data from the succeeding checks

It looks to me very difficult to support both current and this new behavior, as it impacts several places in the code.
I agree with @pdalpra that this is a major breaking change that might cause some existing simulations to suddenly fail, but we have to go this way anyway, hence my opinion to ship such a breaking change in 2.0 instead of postponing to 2.1.
2.1 will bring changes for sure, but mostly for extension developers, not much for end users.

[mushketyk]

@slandelle Would you like to see only change in behavior or some change in the API, say:

.get("http://example.com").failFast().inOrder{ check(...).check(...)) } ?

[slandelle]

I was in favor of only a behavior change. End users don't like having many ways to do the same thing.

[mushketyk]

I guess, I could do it then.

[mushketyk]

If you have no objections :)

[slandelle]

First, I'd like some consensus on this :)
Then, this is absolutely not trivial. It breaks many things in the Check API (that I'm the only one to be familiar with at the moment).

If you interested, please first dig into the code and list what needs to be changed before proceeding.

[mushketyk]

@slandelle

So... from my search, one need to do the following:

1. Remove HttpCheckOrder object
2. Change ctor in HttpCheck to HttpCheck(wrapped: Check[Response], responseBodyUsageStrategy: Option[ResponseBodyUsageStrategy])
3. Don't sort checks in HttpRequestBuilder
4. Check.check should store failures messages to a list
5. Check.check should return Pair[Session => Session, List[String]] (updates and failure messages)
6. AsyncHanderActor.ko should be changed to ko(tx: HttpTx, sessionUpdates: Session => Session, response: Response, messages: List[String])
7. Change output format of AsyncHanderActor.logRequest
8. In dataWriterClient.writeRequestData change parameter message: Option[String] = None to messages: List[String] = Nil
9. In RequestMessage change field message: Option[String] to message: List[String]
10. In ConsoleDataWriter process all error message, instead of a single one
11. In JdbcDataWriter store error messages to a separate table (one-to-many requests to messages)
12. Fix CREATE/INSERT SQL statements in default configuration to accomodate JdbcDataWriter changes
13. RequestMessageSerializer should store all messages instead of a single one
14. JmsRequestTrackerAction should be changed to accomodate change of Check.check

WDYT?

[mushketyk]

@pdalpra Are you in consensus with @slandelle? :)

@slandelle Did I get what should be changed right?

[ceeaspb]

although this is a useful feature, given the amount of change needed and it's not a bug, Is this needed for 2.0 or can it wait for 2.1 ?

[slandelle]

IMHO, this would be great for 2.0 as it's a behavior change.
Note that this was asked for today on SOF: http://stackoverflow.com/questions/24135724/execute-multiple-checks-even-if-the-first-one-fails

[mushketyk]

@slandelle Would you like me to do it? What about my changes list?

[slandelle]

I'll try to review today (so much to do...).

[slandelle]

Beware: breaking behavior change