[GitHub] [trafficcontrol] ocket8888 opened a new pull request #6160: Error-wrapping in lib

[GitBox <gi...@apache.org>]

ocket8888 opened a new pull request #6160:
URL: https://github.com/apache/trafficcontrol/pull/6160


   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(...))`
   
   <!-- **^ Add meaningful description above** --><hr>
   
   ## 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:
   ```regexp
   (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
   
   ```regexp
   errors\.New\(fmt\.Sprintf\(
   ```
   and replaced all found instances with a single call to `fmt.Errorf`
   
   ## PR submission checklist
   - [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: issues-unsubscribe@trafficcontrol.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] rob05c edited a comment on pull request #6160: Error-wrapping in lib

[GitBox <gi...@apache.org>]

rob05c edited a comment on pull request #6160:
URL: https://github.com/apache/trafficcontrol/pull/6160#issuecomment-920957106


   See my comment on https://github.com/apache/trafficcontrol/pull/6159#issuecomment-920952811 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.


-- 
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: issues-unsubscribe@trafficcontrol.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] rob05c commented on pull request #6160: Error-wrapping in lib

[GitBox <gi...@apache.org>]

rob05c commented on pull request #6160:
URL: https://github.com/apache/trafficcontrol/pull/6160#issuecomment-920957106


   See my comment on https://github.com/apache/trafficcontrol/pull/6159#issuecomment-920952811 about performance.
   
   `t3c` isn't nearly as performance-critical as Grove, but we're trying to reduce the runtime of cache. 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.


-- 
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: issues-unsubscribe@trafficcontrol.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org