ocket8888 opened a new pull request #6665:
URL: https://github.com/apache/trafficcontrol/pull/6665
This PR adds a utility function to
`github.com/apache/trafficcontrol/lib/go-util` that allows wrapping errors such
that they may be compared to error constants using `errors.Is` without paying
the cost of the expensive reflection used by the standard library's
`fmt.Errorf` function. The idea is to use this function to preserve the
identity of an underlying error when you want to add context to an error - e.g.
prepending "querying cdns: " to an error message encountered when querying the
`public.cdns` database table - in situations where the performance cost of
using `fmt.Errorf` is too high and instead we are currently using `errors.New`
with string concatenation - e.g. `errors.New("querying cdns: " + err.Error())`.
This PR has a few benchmarks for the different ways an error can be
constructed from an existing one. The common case of making a database query
for some Traffic Ops object (e.g. a CDN in this case) was chosen, such that the
error
message when printed reads
> querying cdns: sql: no rows in result set
The benchmarks cover the new `WrapError` function as well as the two methods
most commonly used today: errors.New("context: " + err.Error())` and
`fmt.Errorf("context: %w", err)`.
There are two different sets of benchmarks, a simpler set that just
repeatedly constructs a single, simple error many times like stated above, and
a more intensive test that recursively wraps a single error many times.
<details><summary>Testing Platform Information</summary>
```
goos: linux
goarch: amd64
pkg: github.com/apache/trafficcontrol/lib/go-util
cpu: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
```
</details>
The simpler benchmarks as-written simply perform the operation necessary to
construct the error using the prescribed method and throw the resulting value
away. Under those conditions it appears that the utility provided here is
between 100 and 300 times faster than the currently favored `errors.New("doing
something: "+err.Error)` method.
```
BenchmarkErrorStringConcatenation-12 30479540 61.17 ns/op 48
B/op 1 allocs/op
BenchmarkErrorWrapping-12 1000000000 0.2495 ns/op 0
B/op 0 allocs/op
BenchmarkErrorf-12 6151614 200.4 ns/op 80
B/op 2 allocs/op
```
That's the most impressive set of results I could get, which is why I kept
that implementation. But I tried it a couple different ways. The other ways are
less impressive, but in none of the benchmark implementations was
error-wrapping using this utility
slower than `errors.New` with string concatenation.
<details><summary>Storing the returned value (overwriting each
time)</summary>
Using the code:
```go
func BenchmarkErrorStringConcatenation(b *testing.B) {
var err error
for i := 0; i < b.N; i++ {
err = errors.New("querying for cdns: " + sql.ErrNoRows.Error())
}
b.StopTimer()
b.Logf("finished benchmark string concatenating '%s'", err)
}
func BenchmarkErrorWrapping(b *testing.B) {
var err error
for i := 0; i < b.N; i++ {
err = WrapError("querying for cdns", sql.ErrNoRows)
}
b.StopTimer()
b.Logf("finished benchmark wrapping '%s'", err)
}
func BenchmarkErrorf(b *testing.B) {
var err error
for i := 0; i < b.N; i++ {
err = fmt.Errorf("querying for cdns: %w", sql.ErrNoRows)
}
b.StopTimer()
b.Logf("finished benchmark fmt.Errorf-ing '%s'", err)
}
```
I got these results:
```
BenchmarkErrorStringConcatenation-12 18886911 78.70 ns/op 64 B/op 2
allocs/op
BenchmarkErrorWrapping-12 39583196 39.77 ns/op 32 B/op 1
allocs/op
BenchmarkErrorf-12 5614521 213.5 ns/op 80 B/op 2
allocs/op
```
Logging output has been omitted. Logging was only necessary because
otherwise the value of `err` is never read, which causes compilation to fail.
But since the timer is stopped before logging, it shouldn't be affecting
results.
</details>
<details><summary>Storing the returned value (keeping every one in a
slice)</summary>
Using the code:
```go
func BenchmarkErrorStringConcatenation(b *testing.B) {
errs := make([]error, 0, b.N)
b.ResetTimer()
for i := 0; i < b.N; i++ {
errs = append(errs, errors.New("querying for cdns:
"+sql.ErrNoRows.Error()))
}
}
func BenchmarkErrorWrapping(b *testing.B) {
errs := make([]error, 0, b.N)
b.ResetTimer()
for i := 0; i < b.N; i++ {
errs = append(errs, WrapError("querying for cdns",
sql.ErrNoRows))
}
}
func BenchmarkErrorf(b *testing.B) {
errs := make([]error, 0, b.N)
b.ResetTimer()
for i := 0; i < b.N; i++ {
errs = append(errs, fmt.Errorf("querying for cdns: %w",
sql.ErrNoRows))
}
}
```
I got the results:
```
BenchmarkErrorStringConcatenation-12 13846206 75.36
ns/op 64 B/op 2 allocs/op
BenchmarkErrorWrapping-12 33168076 32.72
ns/op 32 B/op 1 allocs/op
BenchmarkErrorf-12 6001684 194.0
ns/op 80 B/op 2 allocs/op
```
</details>
I repeated each test a few times, and I'm satisfied they're consistent at
least within order of magnitude. I never saw a situation where wrapping either
used more memory or was slower than string concatenation.
The more complex benchmarks are even more impressive than the best result
from the simpler benchmarks. The performance difference between the new
function and the string concatenation method widens as the number of of times
an error is
wrapped in a context grows. Doing multiple wrapping on a single error a
great many times is more than 1000 times faster than constructing the same
error message by repeatedly concatenating strings:
```
BenchmarkNestedWraps-12 17224899 72.80
ns/op 32 B/op 1 allocs/op
BenchmarkNestedStringConcatenations-12 111364 110801
ns/op 1062059 B/op 2 allocs/op
BenchmarkNestedErrorf-12 22525 108500
ns/op 430623 B/op 4 allocs/op
```
The general case for our errors in ATC is that they are wrapped usually no
less than once and no more than 5 times, at a guess. Probably 2 or 3 is most
common. I think, given the results, under those conditions we shouldn't expect
such a
drastic difference - and in all likelihood those operations aren't
performance bottlenecks anyway. Still, it doesn't seem like there's any
situation where constructing an error through this kind of wrapping would be
better accomplished through string concatenation, and since this method doesn't
destroy the identity of the underlying error it seems best to use it instead in
scenarios where performance needs preclude the use of `fmt.Errorf`. In cases
where performance isn't an issue, `fmt.Errorf` should probably still be used,
IMO, to avoid doing things like `util.WrapError(fmt.Sprintf(...), err)`.
<hr/>
## Which Traffic Control components are affected by this PR?
- None - function is currently unused. Overhauls should be in separate PRs,
if at all.
## What is the best way to verify this PR?
Make sure the provided test passes.
## PR submission checklist
- [x] This PR has tests
- [x] This PR has documentation
- [ ] This PR has a CHANGELOG.md entry
- [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY**
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
