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

feat: add error tracing #479

Closed
wants to merge 1 commit into from
Closed

Conversation

zyllee
Copy link

@zyllee zyllee commented May 16, 2023

Prerequisite checklist

  • Read the contribution guidelines regarding submitting new changes to the project;
  • Tested your changes against relevant architectures and platforms;
  • Ran make fmt on your commit series before opening this PR;
  • Updated relevant documentation.

Description of changes

To add the feature related to #34.
The juju package for error handling is currently imported. Wrote the test code.

@zyllee zyllee marked this pull request as ready for review May 16, 2023 01:41
@zyllee zyllee marked this pull request as draft May 16, 2023 01:51
Copy link
Member

@nderjung nderjung left a comment

Choose a reason for hiding this comment

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

Hi @zyllee, thanks for this contribution!

The issue you've addressed is out-of-date in terms of its implementation but not its requirements. We have since added internal/errs to KraftKit which now stores error types (instead of iota/ints, they're strings).

As discussed on Discord, the need for a error tracing system is still required and juju's errors module still seems like a good fit.

Based on the juju errors' README, you would technically need to use the juju errors package to replace the internal package's definition of errors with juju's, i.e.:

index 8206a12..4d88304 100644
--- a/internal/errs/defs.go
+++ b/internal/errs/defs.go
@@ -4,7 +4,9 @@
 // You may not use this file expect in compliance with the License.
 package errs

-import "errors"
+import
+       "github.com/juju/errors"
+)

However, we actually do NOT need to replace the above, in fact, since internal/errs declares the same (subset) of errors, this can be removed since they will be duplicate. Only when we add new error types (which is possible), would we need to restore this package with custom definitions. Let's hold off for now and simply remove this package.

Instead, to continue this work, I would recommend going through the KraftKit source code and replacing all instances of return fmt.Errorf and errors.New with valid juju errors wrappers. This will allow us to a). classify each error into a category and b). allow us to unwrap each error nicely since the system will homogenise. The moment where errors are actually printed to the console occurs in cmdfactory.Main:

// Main executes the given command
func Main(ctx context.Context, cmd *cobra.Command) {
// Expand flag all dynamically registered flag overrides.
expandRegisteredFlags(cmd)
if err := cmd.ExecuteContext(ctx); err != nil {
fmt.Println(err)
os.Exit(1)
}
}

Replace fmt.Printf(err) with errors.ErrorStack(err)

For the first iteration on your side, this needs to be replace with juju errors' Unwrap or Trace method which will output the call stack or the list of errors which have occurred.

Finally, the code related to "user" look up can be removed entirely since this is out-of-scope of the project.

@nderjung nderjung changed the title feat(errorTracing): add error tracing feat: add error tracing May 16, 2023
@zyllee zyllee closed this May 23, 2023
@zyllee zyllee reopened this May 23, 2023
@zyllee zyllee closed this May 24, 2023
@zyllee zyllee reopened this May 25, 2023
cmd/kraft/build/build.go Outdated Show resolved Hide resolved
cmd/kraft/build/build.go Outdated Show resolved Hide resolved
cmd/kraft/build/build.go Outdated Show resolved Hide resolved
cmd/kraft/build/build.go Outdated Show resolved Hide resolved
cmd/kraft/build/build.go Outdated Show resolved Hide resolved
@zyllee
Copy link
Author

zyllee commented May 25, 2023

The screenshots above(except the first one) are running in cmd kraft build --arch x86_64 --plat kvm. I test it by changing if condition to satisfied return error to happen.

@zyllee zyllee requested a review from nderjung June 7, 2023 14:43
@zyllee zyllee marked this pull request as ready for review June 7, 2023 14:43
@zyllee zyllee marked this pull request as draft June 7, 2023 14:45
@nderjung nderjung added the gsoc Google Summer of Code Contribution label Jun 8, 2023
@zyllee zyllee force-pushed the add-error-tracing branch 7 times, most recently from 40ea1c2 to ec96227 Compare June 16, 2023 14:49
@zyllee zyllee force-pushed the add-error-tracing branch 4 times, most recently from 2f33ef3 to c800c5b Compare June 24, 2023 03:32
@zyllee zyllee force-pushed the add-error-tracing branch 3 times, most recently from 39b3228 to e2ea8f9 Compare July 1, 2023 15:05
@zyllee zyllee marked this pull request as ready for review July 1, 2023 15:06
@zyllee zyllee force-pushed the add-error-tracing branch 17 times, most recently from acad274 to c17ee8d Compare July 7, 2023 15:11
Signed-off-by: Zeyu Li <mr.lizeyu@outlook.com>
@nderjung
Copy link
Member

nderjung commented Jan 8, 2024

Closing due to age and it requiring a large rebase.

@nderjung nderjung closed this Jan 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gsoc Google Summer of Code Contribution
Projects
Status: 🚀 Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants