Skip to content
This repository was archived by the owner on Nov 24, 2025. It is now read-only.

Conversation

@ocket8888
Copy link
Contributor

This makes errors constructed in from other errors in the lib (everything under lib/ 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
  • Errors aren't constructed with errors.New(fmt.Sprintf(...))

Which Traffic Control components are affected by this PR?

  • Traffic Control Cache Config (T3C, formerly ORT)
  • Traffic Ops Go Client
  • Traffic Monitor
  • Traffic Ops
  • Traffic Stats
  • 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 some false positives where the name of a variable being formatted is incorrectly matched as an error. 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\(

and replaced all found instances with a single call to fmt.Errorf

PR submission checklist

  • This PR DOES NOT FIX A SERIOUS SECURITY VULNERABILITY

@ocket8888 ocket8888 added the tech debt rework due to choosing easy/limited solution label Sep 2, 2021
@ocket8888 ocket8888 added cache-config Cache config generation cdn-in-a-box related to the Docker-based CDN-in-a-Box system experimental a feature/component not directly supported by ATC grove related to the "grove" HTTP caching server implementation tests related to tests and/or testing infrastructure TO Client (Go) related to the Go implementation of a TC client tools related to tools outside of main components, e.g. 'compare' Traffic Monitor related to Traffic Monitor Traffic Ops related to Traffic Ops Traffic Stats related to Traffic Stats Traffic Vault related to Traffic Vault labels Sep 9, 2021
@rob05c
Copy link
Member

rob05c commented Sep 16, 2021

See my comment on #6159 (comment) about performance.

t3c isn't nearly as performance-critical as Grove, but we're trying to reduce the runtime of cache config deployment. As we get down from minutes to seconds, it'd be good if we didn't add things that make the whole run take say, 5 seconds instead of 2.

Again, I support the idea of this. And I can't say how much an impact this has without testing. But it'd be really good if, before merging something potentially performance-impacting like this, if someone benchmarked t3c-apply runs for every concievable ordinary configuration on a large CDN, and confirmed it doesn't significantly impact performance.

@zrhoffman
Copy link
Member

@ocket8888 #6160 would be good to merge if it did not use format strings

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cache-config Cache config generation cdn-in-a-box related to the Docker-based CDN-in-a-Box system experimental a feature/component not directly supported by ATC grove related to the "grove" HTTP caching server implementation tech debt rework due to choosing easy/limited solution tests related to tests and/or testing infrastructure TO Client (Go) related to the Go implementation of a TC client tools related to tools outside of main components, e.g. 'compare' Traffic Monitor related to Traffic Monitor Traffic Ops related to Traffic Ops Traffic Stats related to Traffic Stats Traffic Vault related to Traffic Vault

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants