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

satellite/repair: improve logging #3287

Merged
merged 6 commits into from
Oct 16, 2019
Merged

satellite/repair: improve logging #3287

merged 6 commits into from
Oct 16, 2019

Conversation

littleskunk
Copy link
Member

@littleskunk littleskunk commented Oct 15, 2019

What: Improve the logging around failed repair.

satellite/0      12fbck97kq 01:36:35.511 | ERROR	repairer	repairer/repairer.go:95	repair worker failed:	{"Segment": "ZDY0Zjg1MDEtMjJkMC00YjI0LWI1YjItNmIyYzllZGIxOTRhL2wvcmVsZWFzZS1hbHBoYTE3LwI2ebiX+gjbGB9ZJFHU5/T0tgfN5zH9/f4B1GBDk9j9Vg==", "error": "repairer error: repairing injured segment: repairer error: couldn't download enough pieces, number of successful downloaded pieces (1) is less than required number (4)"

Why: At the moment the satellite prints out the encrypted path. It is hard to grep a logfile for the encrypted path. My hope is that with the new logging will allow us to grep, sort and count the binary value instead of the encrypted path. There is still one line that prints out the encrypted path so that we still have it if we need it.

Please describe the tests:

  • Test 1:
  • Test 2:

Please describe the performance impact:

Code Review Checklist (to be filled out by reviewer)

  • Does the PR describe what changes are being made?
  • Does the PR describe why the changes are being made?
  • Does the code follow our style guide?
  • Does the code follow our testing guide?
  • Is the PR appropriately sized? (If it could be broken into smaller PRs it should be)
  • Does the new code have enough tests? (every PR should have tests or justification otherwise. Bug-fix PRs especially)
  • Does the new code have enough documentation that answers "how do I use it?" and "what does it do?"? (both source documentation and higher level, diagrams?)
  • Does any documentation need updating?
  • Do the database access patterns make sense?

@littleskunk littleskunk requested a review from a team October 15, 2019 23:41
@cla-bot cla-bot bot added the cla-signed label Oct 15, 2019
@ghost ghost requested review from cam-a and jenlij and removed request for a team October 15, 2019 23:41
@littleskunk littleskunk added Request Code Review Code review requested Reviewer Can Merge If all checks have passed, non-owner can merge PR labels Oct 15, 2019
navillasa
navillasa previously approved these changes Oct 16, 2019
satellite/repair/repairer/ec.go Outdated Show resolved Hide resolved
Copy link
Member

@jtolio jtolio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment as #3285

Copy link
Contributor

@stefanbenten stefanbenten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least with comparison to #3285 i would prefer to have a more straight logging in terms when to log the path. Sometimes its logged in front, sometimes at the end.

Copy link
Contributor

@stefanbenten stefanbenten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To get it into the release, lets merge it as is and make a follow up PR @littleskunk

@littleskunk littleskunk merged commit 6e76072 into master Oct 16, 2019
@littleskunk littleskunk deleted the jh/repair-logging branch October 16, 2019 15:28
littleskunk added a commit that referenced this pull request Oct 16, 2019
* satellite/repair: improve logging

* use Stringer wherever possible

(cherry picked from commit 6e76072)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed Request Code Review Code review requested Reviewer Can Merge If all checks have passed, non-owner can merge PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants