Rework retry and error classification

[maxmoehl]

• A short explanation of the proposed change:

• An explanation of the use cases your change solves

• Instructions to functionally test the behavior change using operator interfaces (BOSH manifest, logs, curl, and metrics)

• Expected result after the change

• Current result before the change

• Links to any other associated PRs

• [ ] I have viewed signed and have submitted the Contributor License Agreement

• [ ] I have made this pull request to the main branch

• [ ] I have run all the unit tests using scripts/run-unit-tests-in-docker from routing-release.

• [ ] (Optional) I have run Routing Acceptance Tests and Routing Smoke Tests on bosh lite

• [ ] (Optional) I have run CF Acceptance Tests on bosh lite

[maxmoehl]

I'm not sure how to continue with this. The main issue right now is that we cannot mock the request tracer. This hasn't been an issue until now because we just return the special errors and the classifier had the final say, but with this change the isRetriable function might return true early (and for tests it currently always does). The usual approach would be to create a new type that is hidden behind an interface with fakes like we do with all the other stuff. But the overlap with the classifiers is substantial and it feels wrong to duplicate this for two if statements in a function. Modifying the classifier is also tricky because they are used in other places where the additional arguments (*http.Request, *requestTracer) don't make sense.

An alternative approach would be to actually write the tracer fields during testing in our FakeProxyRoundTripper. Currently this sounds like the more reasonable approach to me although we have to be very careful how it interacts with all the existing tests and quite a few of them will require changes.

But before I get started I would like to get a second opinion: @domdom82 @geofffranks WDYT?

[geofffranks]

What overlap between request tracer + the classifiers are you concerned about?

My initial reaction is to go with the approach of adding an interface on top of *requestTracer that we can add a fake for. Then we'd modify traceRequest() to be able to return either a *requestTracer or the fake, depending on whether we're testing, or in live code (similar to RoundTripperFactory).

To me, that seems easier to implement than trying to deal with all the knock-on effects of modifying FakeProxyRoundTripper, but i'm not sure i fully understand the problem you're running into.

[maxmoehl]

Yes, that pretty much catches the issue I'm facing.

The overlap is mainly concerned with the isRetriable function which layers two ifs on top of the existing classifiers. I was trying to come up with a way of merging these things since they try to solve the same problem. But I don't think that's possible and agree with you (e.g. mock the tracer).

I'll probably find some time next week to look into this.