trafficcontrol
trafficcontrol copied to clipboard
apache
Error-wrapping in Grove
This makes errors constructed in from other errors in grove use error-wrapping formatting so that the identity of the underlying error is not destroyed. It also ensures that:
- Error strings do not start with a capital letter
- Error strings do not end with punctuation
- Error identity is checked with
errors.Is - Error type is checked with
errors.As
Which Traffic Control components are affected by this PR?
- Grove
What is the best way to verify this PR?
Make sure all the tests still pass. To check for non-wrapping errors, I grepped with the regular expression:
(fmt\.Errorf|errors\.New)(.+(\.Error\b|%v.+ \w*[eE]r\w*[,)]|\\n|[!\.?]")|\("[A-Z])
which does output some false positives where an error is passed to fmt.Errorf that just so happens to use %v to format something that isn't an error, and one false positive where an error string begins with an initialism ("IP"). I also used errorlint, which is available through golangci-lint, to catch cases where errors.Is and errors.As should be used (but that won't catch all the things the regexp does, it'll miss things like errors.New(err.Error())). Finally, I checked for matches to
errors\.New\(fmt\.Sprintf\(
but that yielded no results, so no changes were necessary.
PR submission checklist
- [x] This PR DOES NOT FIX A SERIOUS SECURITY VULNERABILITY
I like this in-principle, but I have concerns about the performance.
When I was writing Grove, it took considerable performance-tuning to get it to the level it is today, which should be able to handle >20Gbps on decent hardware.
Very specifically, one of the biggest differences was changing it to log less, and to use errors.New instead of fmt.Errorf. Because Go format strings use Reflection, they're between 2 and 100x slower than string building, depending on the context (Buffers are the fastest, but the performance gain over string concatenation isn't large if you don't know your size beforehand).
Those error changes were the difference in Grove being able to handle <2Gbps, and >20Gbps.
Errors in things like config loading aren't as important, but anything in the request path is critical. Even if the errors objects were built without format strings, I also have concerns about things like the .Is and .As usage - looking at the source code, it looks like it uses Reflection. I see things like reflectlite.TypeOf(target).Comparable() which are concerning.
Again, I like the idea. I'd fully support it, if someone benchmarked production-like traffic as high as Grove handle, and could demonstrate performance is unaffected. But I don't have time to do that right now, unfortunately.
