Skip to content

Commit

Permalink
Add ignore check flags to skip checks
Browse files Browse the repository at this point in the history
  • Loading branch information
jjti committed Dec 28, 2023
1 parent 26724e7 commit aa6945f
Show file tree
Hide file tree
Showing 17 changed files with 297 additions and 103 deletions.
7 changes: 7 additions & 0 deletions .github/dependabot.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
version: 2

updates:
- package-ecosystem: gomod
directory: "/"
schedule:
interval: "monthly"
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,4 @@

# Dependency directories (remove the comment below to include it)
# vendor/
src/
18 changes: 16 additions & 2 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,24 @@ fmt:
golangci-lint run --fix --config ./.golangci.yml

.PHONY: test
test:
$(MAKE) -C testdata vendor
test: testvendor
go test -v ./...

# note: I'm copying https://github.com/ghostiam/protogetter/blob/main/testdata/Makefile
#
# x/tools/go/analysis/analysistest does not support go modules. To work around this issue
# we need to vendor any external modules to `./src`.
#
# Follow https://github.com/golang/go/issues/37054 for more details.
.PHONY: testvendor
testvendor:
@rm -rf base/src
@cd testdata/base && go mod vendor
@cp -r testdata/base/vendor testdata/base/src
@cp -r testdata/base/vendor testdata/disableerrorchecks/src
@cp -r testdata/base/vendor testdata/enableall/src
@rm -rf testdata/base/vendor

.PHONY: install
install:
go install ./cmd/spanlint
Expand Down
34 changes: 31 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,18 @@ spanlint ./...
## Configuration

```bash

$ spanlint -h
Usage of spanlint:
-disable-end-check
disable the check for calling span.End() after span creation
-enable-record-error-check
enable the check for calling span.RecordError(err) when returning an error
-enable-set-status-check
enable the check for calling span.SetStatus(codes.Error, msg) when returning an error
-ignore-record-error-check-signatures string
comma-separated list of function signature regex that should disable the span.RecordError(err) checks on errors
-ignore-set-status-check-signatures string
comma-separated list of function signature regex that should disable the span.SetStatus(codes.Error, msg) checks on errors
```

## Background
Expand Down Expand Up @@ -48,8 +59,16 @@ func task(ctx context.Context) error {
}
```

1. OpenTelemetry docs: [Creating spans](https://opentelemetry.io/docs/instrumentation/go/manual/#creating-spans)
1. Uptrace tutorial: [OpenTelemetry Go Tracing API](https://uptrace.dev/opentelemetry/go-tracing.html#quickstart)
For spans to be _really_ useful, developers need to:

1. call `span.End()`
1. call `span.SetStatus(codes.Error, msg)` on error
1. call `span.RecordError(err)` on error
1. call `span.SetAttributes()` liberally

OpenTelemetry docs: [Creating spans](https://opentelemetry.io/docs/instrumentation/go/manual/#creating-spans)

Uptrace tutorial: [OpenTelemetry Go Tracing API](https://uptrace.dev/opentelemetry/go-tracing.html#quickstart)

### Forgetting to call `span.End()`

Expand Down Expand Up @@ -110,3 +129,12 @@ func _() error {
```

OpenTelemetry docs: [Record errors](https://opentelemetry.io/docs/instrumentation/go/manual/#record-errors).

## Attribution

This linter is the result of liberal copying of:

- [github.com/golang/tools/go/analysis/passes/lostcancel](https://github.com/golang/tools/tree/master/go/analysis/passes/lostcancel) (half the linter)
- [github.com/tomarrell/wrapcheck](https://github.com/tomarrell/wrapcheck) (error type checking and config)
- [github.com/Antonboom/testifylint](https://github.com/Antonboom/testifylint) (README)
- [github.com/ghostiam/protogetter](https://github.com/ghostiam/protogetter/blob/main/testdata/Makefile) (test setup)
48 changes: 47 additions & 1 deletion cmd/spanlint/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,64 @@ package main

import (
"flag"
"fmt"
"log"
"regexp"
"strings"

"golang.org/x/tools/go/analysis/singlechecker"

"github.com/jjti/go-spanlint"
)

var (
ignoreSetStatusCheckSignatures = "ignore-set-status-check-signatures"
ignoreRecordErrorCheckSignatures = "ignore-record-error-check-signatures"
)

func main() {
config := spanlint.DefaultConfig
// Set and parse the flags.
config := spanlint.NewDefaultConfig()
flag.BoolVar(&config.DisableEndCheck, "disable-end-check", config.DisableEndCheck, "disable the check for calling span.End() after span creation")
flag.BoolVar(&config.EnableSetStatusCheck, "enable-set-status-check", config.EnableSetStatusCheck, "enable the check for calling span.SetStatus(codes.Error, msg) when returning an error")
ignoreSetStatusCheckSignatures := flag.String(ignoreSetStatusCheckSignatures, "", "comma-separated list of function signature regex that should disable the span.SetStatus(codes.Error, msg) checks on errors")
flag.BoolVar(&config.EnableRecordErrorCheck, "enable-record-error-check", config.EnableRecordErrorCheck, "enable the check for calling span.RecordError(err) when returning an error")
ignoreRecordErrorCheckSignatures := flag.String(ignoreRecordErrorCheckSignatures, "", "comma-separated list of function signature regex that should disable the span.RecordError(err) checks on errors")
flag.Parse()

// Parse the signatures.
config.IgnoreSetStatusCheckSignatures = parseSignatures(ignoreSetStatusCheckSignatures)
config.IgnoreRecordErrorCheckSignatures = parseSignatures(ignoreRecordErrorCheckSignatures)

// Run the analyzer.
singlechecker.Main(spanlint.NewAnalyzer(config))
}

func parseSignatures(sigs *string) *regexp.Regexp {
if sigs == nil {
return nil
}

disableSigs := []string{}
sigColumns := strings.Split(*sigs, ",")
for _, sig := range sigColumns {
sig = strings.TrimSpace(sig)
if sig == "" {
log.Fatalf("empty disable-error-checks-signature value: %q", sig)
}

disableSigs = append(disableSigs, sig)
}

if len(disableSigs) == 0 {
return nil
}

regex := fmt.Sprintf("(%s)", strings.Join(sigColumns, "|"))
regexCompiled, err := regexp.Compile(regex)
if err != nil {
log.Fatalf("failed to compile signatures: %v", err)
}

return regexCompiled
}
24 changes: 19 additions & 5 deletions config.go
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package spanlint

import "regexp"

// Config is a configuration for the spanlint analyzer.
type Config struct {
// EnableEndCheck enables the check for calling span.End().
Expand All @@ -8,14 +10,26 @@ type Config struct {
// EnableSetStatusCheck enables the check for calling span.SetStatus.
EnableSetStatusCheck bool

// IgnoreSetStatusCheckSignatures is a regex that, if matched, disables the
// SetStatus check for a particular error.
IgnoreSetStatusCheckSignatures *regexp.Regexp

// EnableRecordErrorCheck enables the check for calling span.RecordError.
// By default, this check is disabled.
EnableRecordErrorCheck bool

// IgnoreRecordErrorCheckSignatures is a regex that, if matched, disables the
// RecordError check for a particular error.
IgnoreRecordErrorCheckSignatures *regexp.Regexp
}

// DefaultConfig is the default configuration for the spanlint analyzer.
var DefaultConfig = &Config{
DisableEndCheck: false,
EnableSetStatusCheck: false,
EnableRecordErrorCheck: false,
// NewDefaultConfig returns a new Config with default values.
func NewDefaultConfig() *Config {
return &Config{
DisableEndCheck: false,
EnableSetStatusCheck: false,
IgnoreSetStatusCheckSignatures: nil,
EnableRecordErrorCheck: false,
IgnoreRecordErrorCheckSignatures: nil,
}
}
37 changes: 37 additions & 0 deletions doc.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
// Package spanlint defines a linter that checks for mistakes with OTEL trace spans.
//
// # Analyzer spanlint
//
// spanlint: check for mistakes with OpenTelemetry trace spans.
//
// Common mistakes with OTEL trace spans include forgetting to call End:
//
// func(ctx context.Context) {
// ctx, span := otel.Tracer("app").Start(ctx, "span")
// // defer span.End() should be here
//
// // do stuff
// }
//
// Forgetting to set an Error status:
//
// ctx, span := otel.Tracer("app").Start(ctx, "span")
// defer span.End()
//
// if err := task(); err != nil {
// // span.SetStatus(codes.Error, err.Error()) should be here
// span.RecordError(err)
// return fmt.Errorf("failed to run task: %w", err)
// }
//
// Forgetting to record the Error:
//
// ctx, span := otel.Tracer("app").Start(ctx, "span")
// defer span.End()
//
// if err := task(); err != nil {
// span.SetStatus(codes.Error, err.Error())
// // span.RecordError(err) should be here
// return fmt.Errorf("failed to run task: %w", err)
// }
package spanlint
2 changes: 1 addition & 1 deletion go.work
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ go 1.21.3

use (
.
./testdata
./testdata/base
./testdata/disableerrorchecks
./testdata/enableall
)
Loading

0 comments on commit aa6945f

Please sign in to comment.