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

[WIP] Annotates errors from reconciler package with stack trace #297

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

Conversation

yfl92
Copy link

@yfl92 yfl92 commented Jan 23, 2021

Fixes # .

Motivation

Previously, we don't have stack trace when it comes to error which makes rosetta-sdk-go and rosetta-cli a blackbox. It's hard to debug especially when rosetta-cli exited with an error.

Solution

This PR annotates errors in package reconciler with stack trace. All behaviors should remain the same. User of this SDK can now print out stack trace through fmt.Printf("%+v", err)

Future Work

  • Use similar mechanism for other packages
  • Refactor this when Go2 comes out which should drastically change on how error handling works

@coveralls
Copy link

coveralls commented Jan 23, 2021

Pull Request Test Coverage Report for Build 14403

  • 36 of 55 (65.45%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.008%) to 77.914%

Changes Missing Coverage Covered Lines Changed/Added Lines %
reconciler/reconciler.go 36 55 65.45%
Totals Coverage Status
Change from base Build 14250: -0.008%
Covered Lines: 8283
Relevant Lines: 10631

💛 - Coveralls

@yfl92 yfl92 changed the title [WIP] Add stack trace to reconciliation errors [WIP] Add stack trace to reconciler package errors Jan 28, 2021
@yfl92 yfl92 changed the title [WIP] Add stack trace to reconciler package errors [WIP] Annotates errors from reconciler package with stack trace Jan 29, 2021
@yfl92 yfl92 force-pushed the lin/reconciliation-error-overhaul branch 2 times, most recently from 2e7cceb to 8688ed3 Compare January 29, 2021 01:05
@yfl92 yfl92 force-pushed the lin/reconciliation-error-overhaul branch from 8688ed3 to 7c38fba Compare January 29, 2021 01:09
ErrGetCurrentBlockFailed,
err,
)
return zeroString, "", 0, errors.Wrapf(ErrGetCurrentBlockFailed, "%v", err)
Copy link
Contributor

@septerr septerr Feb 2, 2021

Choose a reason for hiding this comment

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

I think this should be return zeroString, "", 0, errors.Wrapf(err, "%v: %v", ErrGetCurrentBlockFailed.Error(), err.Error()).

This will make sure we are wrapping the err and getting its stack trace. And we use ErrGetCurrentBlockFailed only for the error message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants