Skip to content

Commit

Permalink
refactor: switch to errtrace for error stacks (#691)
Browse files Browse the repository at this point in the history
[errtrace][1] includes a tool that automatically instruments error
returns to provide stack traces.

This obviates the need for us to manually wrap errors to get stacks.
Instead, we'll just use normal Go error functions (`errors.New()` and
`fmt.Errorf()` with `%w` - see docs) and automatically add the
instrumentation when building production release binaries.

[1]: https://github.com/bracesdev/errtrace
  • Loading branch information
alecthomas committed Dec 3, 2023
1 parent 338989b commit 4921513
Show file tree
Hide file tree
Showing 95 changed files with 665 additions and 716 deletions.
3 changes: 0 additions & 3 deletions .githooks/pre-commit

This file was deleted.

3 changes: 2 additions & 1 deletion .github/workflows/release.yml
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,8 @@ jobs:
run: bit frontend/dist/index.html
- name: Publish Go Binaries
run: |
git ls-files -- '*.go' | xargs errtrace -w && go mod tidy
bit build/release/ftl
goreleaser release
goreleaser release --skip=validate
env:
GITHUB_TOKEN: ${{ github.token }}
17 changes: 10 additions & 7 deletions .golangci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ linters:
- interfacebloat
- tagalign
- nolintlint
- wrapcheck # We might want to re-enable this if we manually wrap all the existing errors with fmt.Errorf

linters-settings:
exhaustive:
Expand All @@ -92,15 +93,17 @@ linters-settings:
rules:
main:
deny:
- pkg: errors
desc: "use github.com/alecthomas/errors"
- pkg: github.com/pkg/errors
desc: "use github.com/alecthomas/errors"
desc: "use fmt.Errorf or errors.New"
- pkg: github.com/stretchr/testify
desc: "use alecthomas/assert"
wrapcheck:
ignorePackageGlobs:
- github.com/TBD54566975/ftl/*
desc: "use fmt.Errorf or errors.New"
- pkg: github.com/alecthomas/errors
desc: "use fmt.Errorf or errors.New"
- pkg: braces.dev/errtrace
desc: "use fmt.Errorf or errors.New"
# wrapcheck:
# ignorePackageGlobs:
# - github.com/TBD54566975/ftl/*

issues:
max-same-issues: 0
Expand Down
8 changes: 0 additions & 8 deletions .pre-commit-config.yaml

This file was deleted.

9 changes: 0 additions & 9 deletions Dockerfile.console

This file was deleted.

1 change: 1 addition & 0 deletions Dockerfile.controller
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ RUN go mod download -x

# Build
COPY . /src/
RUN git ls-files -- '*.go' | xargs errtrace -w && go mod tidy
RUN bit build/release/ftl-controller
RUN bit build/release/ftl-initdb
RUN bit build/release/ftl
Expand Down
1 change: 1 addition & 0 deletions Dockerfile.runner
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ COPY . /src/
RUN bit build/template/ftl/jars/ftl-runtime.jar

# Build runner
RUN git ls-files -- '*.go' | xargs errtrace -w && go mod tidy
RUN bit build/release/ftl-runner
RUN bit build/release/ftl

Expand Down
22 changes: 11 additions & 11 deletions authn/authn.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package authn
import (
"bufio"
"context"
"errors"
"fmt"
"io"
"net/http"
Expand All @@ -12,7 +13,6 @@ import (
"sync"
"time"

"github.com/alecthomas/errors"
"github.com/zalando/go-keyring"

"github.com/TBD54566975/ftl/backend/common/exec"
Expand Down Expand Up @@ -46,7 +46,7 @@ func GetAuthenticationHeaders(ctx context.Context, endpoint *url.URL, authentica

usr, err := user.Current()
if err != nil {
return nil, errors.WithStack(err)
return nil, err
}

// First, check if we have credentials in the keyring and that they work.
Expand All @@ -62,7 +62,7 @@ func GetAuthenticationHeaders(ctx context.Context, endpoint *url.URL, authentica
} else {
logger.Tracef("Credentials found in keyring: %s", creds)
if headers, err := checkAuth(ctx, logger, endpoint, creds); err != nil {
return nil, errors.WithStack(err)
return nil, err
} else if headers != nil {
return headers, nil
}
Expand All @@ -73,13 +73,13 @@ func GetAuthenticationHeaders(ctx context.Context, endpoint *url.URL, authentica
cmd.Stdout = out
err = cmd.Run()
if err != nil {
return nil, errors.Wrapf(err, "authenticator %s failed", authenticator)
return nil, fmt.Errorf("authenticator %s failed: %w", authenticator, err)
}

creds = out.String()
headers, err := checkAuth(ctx, logger, endpoint, creds)
if err != nil {
return nil, errors.WithStack(err)
return nil, err
} else if headers == nil {
return nil, nil
}
Expand Down Expand Up @@ -108,12 +108,12 @@ func checkAuth(ctx context.Context, logger *log.Logger, endpoint *url.URL, creds
line := buf.Text()
name, value, ok := strings.Cut(line, ":")
if !ok {
return nil, errors.Errorf("invalid header %q", line)
return nil, fmt.Errorf("invalid header %q", line)
}
headers[name] = append(headers[name], strings.TrimSpace(value))
}
if buf.Err() != nil {
return nil, errors.WithStack(buf.Err())
return nil, buf.Err()
}

// Issue a HEAD request with the headers to verify we get a 200 back.
Expand All @@ -125,7 +125,7 @@ func checkAuth(ctx context.Context, logger *log.Logger, endpoint *url.URL, creds
}
req, err := http.NewRequestWithContext(ctx, http.MethodHead, endpoint.String(), nil)
if err != nil {
return nil, errors.WithStack(err)
return nil, err
}
logger.Debugf("Authentication probe: %s %s", req.Method, req.URL)
for header, values := range headers {
Expand All @@ -136,7 +136,7 @@ func checkAuth(ctx context.Context, logger *log.Logger, endpoint *url.URL, creds
logger.Tracef("Authenticating with headers %s", headers)
resp, err := client.Do(req)
if err != nil {
return nil, errors.WithStack(err)
return nil, err
}
defer resp.Body.Close() //nolint:gosec
if resp.StatusCode != http.StatusOK {
Expand Down Expand Up @@ -177,7 +177,7 @@ func (a *authnTransport) RoundTrip(r *http.Request) (*http.Response, error) {
var err error
creds, err = GetAuthenticationHeaders(r.Context(), r.URL, a.authenticators)
if err != nil {
return nil, errors.Wrapf(err, "failed to get authentication headers for %s", r.URL.Hostname())
return nil, fmt.Errorf("failed to get authentication headers for %s: %w", r.URL.Hostname(), err)
}
a.lock.Lock()
a.credentials[r.URL.Hostname()] = creds
Expand All @@ -189,5 +189,5 @@ func (a *authnTransport) RoundTrip(r *http.Request) (*http.Response, error) {
}
}
resp, err := a.next.RoundTrip(r)
return resp, errors.WithStack(err)
return resp, err
}
5 changes: 2 additions & 3 deletions backend/common/bind/bind_allocator.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
"strconv"

"github.com/alecthomas/atomic"
"github.com/alecthomas/errors"
)

type BindAllocator struct {
Expand All @@ -18,12 +17,12 @@ type BindAllocator struct {
func NewBindAllocator(url *url.URL) (*BindAllocator, error) {
_, portStr, err := net.SplitHostPort(url.Host)
if err != nil {
return nil, errors.WithStack(err)
return nil, err
}

port, err := strconv.Atoi(portStr)
if err != nil {
return nil, errors.WithStack(err)
return nil, err
}

return &BindAllocator{
Expand Down
14 changes: 7 additions & 7 deletions backend/common/download/download.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,12 @@ package download

import (
"context"
"fmt"
"os"
"path/filepath"
"time"

"connectrpc.com/connect"
"github.com/alecthomas/errors"

"github.com/TBD54566975/ftl/backend/common/log"
"github.com/TBD54566975/ftl/backend/common/model"
Expand All @@ -22,7 +22,7 @@ func Artefacts(ctx context.Context, client ftlv1connect.ControllerServiceClient,
DeploymentName: name.String(),
}))
if err != nil {
return errors.WithStack(err)
return err
}
start := time.Now()
count := 0
Expand All @@ -37,32 +37,32 @@ func Artefacts(ctx context.Context, client ftlv1connect.ControllerServiceClient,
}
count++
if !filepath.IsLocal(artefact.Path) {
return errors.Errorf("path %q is not local", artefact.Path)
return fmt.Errorf("path %q is not local", artefact.Path)
}
logger.Infof("Downloading %s", filepath.Join(dest, artefact.Path))
err = os.MkdirAll(filepath.Join(dest, filepath.Dir(artefact.Path)), 0700)
if err != nil {
return errors.WithStack(err)
return err
}
var mode os.FileMode = 0600
if artefact.Executable {
mode = 0700
}
w, err = os.OpenFile(filepath.Join(dest, artefact.Path), os.O_CREATE|os.O_WRONLY, mode)
if err != nil {
return errors.WithStack(err)
return err
}
digest = artefact.Digest
}

if _, err := w.Write(msg.Chunk); err != nil {
_ = w.Close()
return errors.WithStack(err)
return err
}
}
if w != nil {
w.Close()
}
logger.Infof("Downloaded %d artefacts in %s", count, time.Since(start))
return errors.WithStack(stream.Err())
return stream.Err()
}
7 changes: 3 additions & 4 deletions backend/common/exec/exec.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (
"os/exec"
"syscall"

"github.com/alecthomas/errors"
"github.com/kballard/go-shellquote"

"github.com/TBD54566975/ftl/backend/common/log"
Expand All @@ -18,15 +17,15 @@ type Cmd struct {

func LookPath(exe string) (string, error) {
path, err := exec.LookPath(exe)
return path, errors.WithStack(err)
return path, err
}

func Capture(ctx context.Context, dir, exe string, args ...string) ([]byte, error) {
cmd := Command(ctx, log.Debug, dir, exe, args...)
cmd.Stdout = nil
cmd.Stderr = nil
out, err := cmd.CombinedOutput()
return out, errors.WithStack(err)
return out, err
}

func Command(ctx context.Context, level log.Level, dir, exe string, args ...string) *Cmd {
Expand Down Expand Up @@ -54,5 +53,5 @@ func (c *Cmd) Kill(signal syscall.Signal) error {
if c.Process == nil {
return nil
}
return errors.WithStack(syscall.Kill(c.Process.Pid, signal))
return syscall.Kill(c.Process.Pid, signal)
}
7 changes: 3 additions & 4 deletions backend/common/log/json.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,9 @@ package log
import (
"bufio"
"encoding/json"
"errors"
"io"
"time"

"github.com/alecthomas/errors"
)

var _ Sink = (*jsonSink)(nil)
Expand Down Expand Up @@ -39,7 +38,7 @@ func (j *jsonSink) Log(entry Entry) error {
Error: errStr,
Entry: entry,
}
return errors.WithStack(j.enc.Encode(jentry))
return j.enc.Encode(jentry)
}

// JSONStreamer reads a stream of JSON log entries from r and logs them to log.
Expand Down Expand Up @@ -73,5 +72,5 @@ func JSONStreamer(r io.Reader, log *Logger, defaultLevel Level) error {
if errors.Is(err, io.EOF) {
return nil
}
return errors.WithStack(err)
return err
}
3 changes: 1 addition & 2 deletions backend/common/log/plain.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import (
"io"
"os"

"github.com/alecthomas/errors"
"github.com/mattn/go-isatty"
)

Expand Down Expand Up @@ -51,7 +50,7 @@ func (t *plainSink) Log(entry Entry) error {
_, err = fmt.Fprintf(t.w, "%s%s\n", prefix, entry.Message)
}
if err != nil {
return errors.WithStack(err)
return err
}
return nil
}
2 changes: 1 addition & 1 deletion backend/common/log/tee.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
package log

import (
"github.com/alecthomas/errors"
"errors"
)

// Tee returns a sink that writes to all of the given sinks.
Expand Down
7 changes: 3 additions & 4 deletions backend/common/model/deployment_name.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import (
"fmt"
"strings"

"github.com/alecthomas/errors"
"github.com/alecthomas/types"
)

Expand Down Expand Up @@ -37,14 +36,14 @@ func ParseDeploymentName(name string) (DeploymentName, error) {
var zero DeploymentName
parts := strings.Split(name, "-")
if len(parts) < 2 {
return zero, errors.Errorf("should be at least <deployment>-<hash>: invalid deployment name %q", name)
return zero, fmt.Errorf("should be at least <deployment>-<hash>: invalid deployment name %q", name)
}
hash, err := hex.DecodeString(parts[len(parts)-1])
if err != nil {
return zero, errors.Wrapf(err, "invalid deployment name %q", name)
return zero, fmt.Errorf("invalid deployment name %q: %w", name, err)
}
if len(hash) != 5 {
return zero, errors.Errorf("hash should be 5 bytes: invalid deployment name %q", name)
return zero, fmt.Errorf("hash should be 5 bytes: invalid deployment name %q", name)
}
return DeploymentName(fmt.Sprintf("%s-%010x", strings.Join(parts[0:len(parts)-1], "-"), hash)), nil
}
Expand Down
Loading

0 comments on commit 4921513

Please sign in to comment.