Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error wrapping in t3c #6157

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

ocket8888
Copy link
Contributor

This makes errors constructed in from other errors in t3c (everything under cache-config/) 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

Originally, I was going to do that for everything in the repo at once, but I drastically underestimated how large that changeset would be. So I just started with the first component alphabetically. Partially because I did originally want this to encompass the whole repo, there's also a change in here that that removes a newline from an error string in traffic_vault_migrate and some changes to the db/admin program - I left those in because I figured it was harmless to do so, but I can take them out at a reviewer's request.


Which Traffic Control components are affected by this PR?

  • Traffic Control Cache Config (T3C, formerly ORT)

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. 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

  • This PR DOES NOT FIX A SERIOUS SECURITY VULNERABILITY

@ocket8888 ocket8888 added tech debt rework due to choosing easy/limited solution cache-config Cache config generation labels Sep 1, 2021
@ocket8888 ocket8888 force-pushed the error-wrapping branch 3 times, most recently from b62a18b to a3d9335 Compare September 16, 2021 14:04
@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.

Copy link

codecov bot commented Nov 14, 2023

Codecov Report

Attention: 275 lines in your changes are missing coverage. Please review.

Comparison is base (61865fb) 31.91% compared to head (5092d88) 31.92%.

Files Patch % Lines
cache-config/t3cutil/toreq/clientfuncs.go 0.00% 51 Missing ⚠️
cache-config/t3cutil/toreq/toreqold/clientfuncs.go 0.00% 46 Missing ⚠️
cache-config/t3c-apply/torequest/cmd.go 0.00% 38 Missing ⚠️
cache-config/t3c-apply/torequest/torequest.go 0.00% 33 Missing ⚠️
cache-config/t3c-apply/util/gitutil.go 0.00% 23 Missing ⚠️
cache-config/t3cutil/getdata.go 0.00% 19 Missing ⚠️
cache-config/t3cutil/getdatacfg.go 0.00% 18 Missing ⚠️
cache-config/t3c-apply/util/util.go 0.00% 11 Missing ⚠️
cache-config/t3cutil/toreq/client.go 0.00% 8 Missing ⚠️
cache-config/t3c-apply/config/config.go 0.00% 5 Missing ⚠️
... and 12 more
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #6157      +/-   ##
============================================
+ Coverage     31.91%   31.92%   +0.01%     
  Complexity       98       98              
============================================
  Files           719      719              
  Lines         82788    82774      -14     
  Branches        970      970              
============================================
+ Hits          26423    26428       +5     
+ Misses        54202    54184      -18     
+ Partials       2163     2162       -1     
Flag Coverage Δ
golib_unit 53.86% <ø> (+<0.01%) ⬆️
grove_unit 12.02% <ø> (ø)
t3c_unit 5.91% <0.36%> (+0.10%) ⬆️
traffic_monitor_unit 26.44% <ø> (ø)
traffic_ops_unit 21.68% <ø> (ø)
traffic_portal_v2 74.35% <ø> (+0.01%) ⬆️
traffic_stats_unit 10.78% <ø> (ø)
unit_tests 29.25% <0.36%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cache-config Cache config generation tech debt rework due to choosing easy/limited solution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants