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.

Sign up for GitHub

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

Jump to bottom

Generate call stack for sdk error and preserve stack trace #3310

Merged
merged 8 commits into from
Oct 5, 2018
Merged

Generate call stack for sdk error and preserve stack trace #3310

merged 8 commits into from
Oct 5, 2018

Conversation

richardlt
Copy link
Member

@richardlt richardlt commented Sep 10, 2018 •
edited
Loading

  1. Description
  • Printing call stack and stack trace automatically in wrapped errors.
  • Add UUID on error returned from API.
  • Show error uuid in ctl when an error occured.
  • Create admin endpoint to get error from Graylog by uuid.
  • Create ctl command to get error by uuid.
  1. Log strategy
  • For a simple error: catched by the router and associated with a unknown sdk.Error, then logged with a UUID.
  • For a sdk.Error: catched by the router the logged with a UUID.
  • For a wrapped simple error: same as simple error but logged with stack trace and call stack.
  • For a wrapped sdk.Error: same as sdk.Error but logged with stack trace and call stack.
  1. Err return strategy
  • All error from lib should be wrapped like sdk.WithStack(err) or sdk.WrapError(err, format, values...), WithStack and WrapError do not override existing callers on already wrapped errors. This will generate unknown http error (500).
  • Create new error from existing sdk.Error like sdk.NewError(ErrForbidden, err). This will generated a predefined http error with stack trace. New errors can also be wrapped to add more details.
  • Return sdk.Error{} or predefined sdk.Error like ErrForbidden. This will generated a http error that will be logged with a uuid but without stack trace or call stack.
  • If a simple error is return it will be catched be the router and returns as an unknown error (500).
    @ovh/cds

@richardlt richardlt changed the title WIP feat(sdk): Update new error and wrap to preserve root stack trace and… WIP Generate call stack for sdk error and preserve stack trace Sep 10, 2018
@richardlt richardlt force-pushed the feat-error-with-stack branch from acf2cd4 to 884f9ae Compare September 26, 2018 07:49
golangcibot
golangcibot reviewed Sep 27, 2018
View reviewed changes
sdk/error.go Outdated
}
fallthrough
case 's':
io.WriteString(s, fmt.Sprintf("%s: %s", w.stack.String(), w.Cause().Error()))
Copy link

Choose a reason for hiding this comment

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

Error return value of io.WriteString is not checked

@richardlt richardlt force-pushed the feat-error-with-stack branch from 7fd38e3 to 2b27275 Compare September 27, 2018 20:51
golangcibot
golangcibot reviewed Sep 27, 2018
View reviewed changes
engine/service/http.go Outdated
}

_ = WriteJSON(w, sdkErr, errProcessed.Status)
// safely ignore error returned by WriteJSON
WriteJSON(w, httpErr, httpErr.Status)
Copy link

Choose a reason for hiding this comment

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

Error return value of WriteJSON is not checked

@richardlt richardlt force-pushed the feat-error-with-stack branch 4 times, most recently from 438368e to 4960938 Compare September 28, 2018 11:47
@richardlt richardlt force-pushed the feat-error-with-stack branch from 4960938 to 5141c27 Compare September 28, 2018 12:43
@ovh ovh deleted a comment from ovh-cds Sep 28, 2018
@ovh ovh deleted a comment from ovh-cds Sep 28, 2018
@ovh ovh deleted a comment from ovh-cds Sep 28, 2018
@ovh ovh deleted a comment from ovh-cds Sep 28, 2018
@ovh ovh deleted a comment from ovh-cds Sep 28, 2018
@ovh ovh deleted a comment from ovh-cds Sep 28, 2018
@ovh ovh deleted a comment from ovh-cds Sep 28, 2018
@ovh ovh deleted a comment from ovh-cds Sep 28, 2018
@ovh ovh deleted a comment from ovh-cds Sep 28, 2018
@richardlt richardlt force-pushed the feat-error-with-stack branch 2 times, most recently from 5dec0b0 to 7890e36 Compare September 28, 2018 15:08
@richardlt richardlt force-pushed the feat-error-with-stack branch from 7890e36 to 0a6b710 Compare September 28, 2018 15:38
@ovh-cds
Copy link
Collaborator

ovh-cds commented Sep 30, 2018

CDS Report build-ui#6240.0 ✘

  • Compile

    • Build UI ✔
    • TU All -
    • TU model ✔
    • TU service ✘
    • TU shared ✔
    • TU views ✔
      Unit Tests Report

  • HeadlessChrome 0.0.0 (Linux 0.0.0)

  • HeadlessChrome 0.0.0 (Linux 0.0.0).2185480

    • CDS: pipeline Store should create/update and delete a job ✘

    • CDS: project Store should add/update/delete a variable ✘

    • CDS: project Store should add/update/delete a permission ✘

  • HeadlessChrome 0.0.0 (Linux 0.0.0).2185482

  • HeadlessChrome 0.0.0 (Linux 0.0.0).2185481

@richardlt richardlt changed the title WIP Generate call stack for sdk error and preserve stack trace Generate call stack for sdk error and preserve stack trace Sep 30, 2018
fsamin
fsamin approved these changes Oct 1, 2018
View reviewed changes
Copy link
Member

@fsamin fsamin left a comment

Choose a reason for hiding this comment

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

LGTM,
you should have a look in the worker commands error management, it's a mess

bnjjj
bnjjj suggested changes Oct 5, 2018
View reviewed changes
cli/cobra.go Outdated
@@ -14,6 +14,7 @@ import (

"github.com/fsamin/go-dump"
"github.com/olekukonko/tablewriter"
"github.com/ovh/cds/sdk"
Copy link
Contributor

Choose a reason for hiding this comment

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

bad import position

engine/api/error.go Outdated
"time"

"github.com/gorilla/mux"
"github.com/ovh/cds/sdk"
Copy link
Contributor

Choose a reason for hiding this comment

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

bad import position

sdk/error.go Show resolved Hide resolved
@bnjjj bnjjj merged commit dc10dfc into master Oct 5, 2018
@fsamin fsamin deleted the feat-error-with-stack branch November 15, 2018 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@golangcibot golangcibot golangcibot left review comments

@bnjjj bnjjj bnjjj approved these changes

@sguiheux sguiheux sguiheux approved these changes

@fsamin fsamin fsamin approved these changes

@yesnault yesnault yesnault approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@richardlt @ovh-cds @yesnault @fsamin @sguiheux @bnjjj @golangcibot