Add support for "Start" utility methods (#16)

* feat: replace hardcoded span start function names with regex patterns Replaces the hardcoded list of function names and packages with a list of regex patterns that may be extended using the `-extra-start-span-signatures` command line option. The format is `<regex>:<telemetry-type>`. Ideally this would just be a list of regex patterns, but we need to know what telemetry library we're matching so we don't run RecordError on opencensus. * tests: update tests to use new default config The tests need to unconditionally include the DefaultStartSpanSignatures, which was not possible with the old harness. Instead of returning an object directly, they now return a function to prepare a config, which ensures they all start out with the default config as a base. * Add test, skip validating spans within starter funcs * Hack: fix check for start func method name * fix: increase robustness of skipping custom matchers Instead of checking if the function name matches, which might result in overzealous exclusions, prefer to match against the function signature itself * docs: document new behaviour * Remove sig split in config * nit: small wording changes --------- Co-authored-by: josh <[email protected]>
jjti · Apr 21, 2024 · 34d21b4 · 34d21b4
1 parent 41d395c
commit 34d21b4
Show file tree

Hide file tree

Showing 7 changed files with 219 additions and 48 deletions.
diff --git a/README.md b/README.md
@@ -63,6 +63,13 @@ linters-settings:
     # Default: []
     ignore-check-signatures:
       - "telemetry.RecordError"
+    # A list of regexes for additional function signatures that create spans. This is useful if you have a utility
+    # method to create spans. Each entry should be of the form <regex>:<telemetry-type>, where `telemetry-type`
+    # can be `opentelemetry` or `opencensus`.
+    # https://github.com/jjti/go-spancheck#extra-start-span-signatures
+    # Default: []
+    extra-start-span-signatures:
+      - "github.com/user/repo/telemetry/trace.Start:opentelemetry"
 ```
 
 ### CLI
@@ -123,6 +130,21 @@ The warnings are can be ignored by setting `-ignore-check-signatures` flag to `r
 spancheck -checks 'end,set-status,record-error' -ignore-check-signatures 'recordErr' ./...
 ```
 
+### Extra Start Span Signatures
+
+By default, Span creation will be tracked from calls to [(go.opentelemetry.io/otel/trace.Tracer).Start](https://github.com/open-telemetry/opentelemetry-go/blob/98b32a6c3a87fbee5d34c063b9096f416b250897/trace/trace.go#L523), [go.opencensus.io/trace.StartSpan](https://pkg.go.dev/go.opencensus.io/trace#StartSpan), or [go.opencensus.io/trace.StartSpanWithRemoteParent](https://github.com/census-instrumentation/opencensus-go/blob/v0.24.0/trace/trace_api.go#L66).
+
+You can use the `-extra-start-span-signatures` flag to list additional Span creation functions. For all such functions:
+
+1. their Spans will be linted (for all enable checks)
+1. checks will be disabled (i.e. there is no linting of Spans within the creation functions)
+
+You must pass a comma-separated list of regex patterns and the telemetry library corresponding to the returned Span. Each entry should be of the form `<regex>:<telemetry-type>`, where `telemetry-type` can be `opentelemetry` or `opencensus`. For example, if you have created a function named `StartTrace` in a `telemetry` package, using the `go.opentelemetry.io/otel` library, you can include this function for analysis like so:
+
+```bash
+spancheck -extra-start-span-signatures 'github.com/user/repo/telemetry/StartTrace:opentelemetry' ./...
+```
+
 ## Problem Statement
 
 Tracing is a celebrated [[1](https://andydote.co.uk/2023/09/19/tracing-is-better/),[2](https://charity.wtf/2022/08/15/live-your-best-life-with-structured-events/)] and well marketed [[3](https://docs.datadoghq.com/tracing/),[4](https://www.honeycomb.io/distributed-tracing)] pillar of observability. But self-instrumented tracing requires a lot of easy-to-forget boilerplate:

diff --git a/cmd/spancheck/main.go b/cmd/spancheck/main.go
@@ -23,11 +23,17 @@ func main() {
 	// Set the list of function signatures to ignore checks for.
 	ignoreCheckSignatures := ""
 	flag.StringVar(&ignoreCheckSignatures, "ignore-check-signatures", "", "comma-separated list of regex for function signatures that disable checks on errors")
+
+	extraStartSpanSignatures := ""
+	flag.StringVar(&extraStartSpanSignatures, "extra-start-span-signatures", "", "comma-separated list of regex:telemetry-type for function signatures that indicate the start of a span")
+
 	flag.Parse()
 
 	cfg := spancheck.NewDefaultConfig()
 	cfg.EnabledChecks = strings.Split(checkStrings, ",")
 	cfg.IgnoreChecksSignaturesSlice = strings.Split(ignoreCheckSignatures, ",")
 
+	cfg.StartSpanMatchersSlice = append(cfg.StartSpanMatchersSlice, strings.Split(extraStartSpanSignatures, ",")...)
+
 	singlechecker.Main(spancheck.NewAnalyzerWithConfig(cfg))
 }
diff --git a/config.go b/config.go
@@ -22,6 +22,18 @@ const (
 	RecordErrorCheck
 )
 
+var (
+	startSpanSignatureCols     = 2
+	defaultStartSpanSignatures = []string{
+		// https://github.com/open-telemetry/opentelemetry-go/blob/98b32a6c3a87fbee5d34c063b9096f416b250897/trace/trace.go#L523
+		`\(go.opentelemetry.io/otel/trace.Tracer\).Start:opentelemetry`,
+		// https://pkg.go.dev/go.opencensus.io/trace#StartSpan
+		`go.opencensus.io/trace.StartSpan:opencensus`,
+		// https://github.com/census-instrumentation/opencensus-go/blob/v0.24.0/trace/trace_api.go#L66
+		`go.opencensus.io/trace.StartSpanWithRemoteParent:opencensus`,
+	}
+)
+
 func (c Check) String() string {
 	switch c {
 	case EndCheck:
@@ -35,14 +47,17 @@ func (c Check) String() string {
 	}
 }
 
-var (
-	// Checks is a list of all checks by name.
-	Checks = map[string]Check{
-		EndCheck.String():         EndCheck,
-		SetStatusCheck.String():   SetStatusCheck,
-		RecordErrorCheck.String(): RecordErrorCheck,
-	}
-)
+// Checks is a list of all checks by name.
+var Checks = map[string]Check{
+	EndCheck.String():         EndCheck,
+	SetStatusCheck.String():   SetStatusCheck,
+	RecordErrorCheck.String(): RecordErrorCheck,
+}
+
+type spanStartMatcher struct {
+	signature *regexp.Regexp
+	spanType  spanType
+}
 
 // Config is a configuration for the spancheck analyzer.
 type Config struct {
@@ -55,19 +70,25 @@ type Config struct {
 	// the IgnoreSetStatusCheckSignatures regex.
 	IgnoreChecksSignaturesSlice []string
 
+	StartSpanMatchersSlice []string
+
 	endCheckEnabled    bool
 	setStatusEnabled   bool
 	recordErrorEnabled bool
 
 	// ignoreChecksSignatures is a regex that, if matched, disables the
 	// SetStatus and RecordError checks on error.
 	ignoreChecksSignatures *regexp.Regexp
+
+	startSpanMatchers            []spanStartMatcher
+	startSpanMatchersCustomRegex *regexp.Regexp
 }
 
 // NewDefaultConfig returns a new Config with default values.
 func NewDefaultConfig() *Config {
 	return &Config{
-		EnabledChecks: []string{EndCheck.String()},
+		EnabledChecks:          []string{EndCheck.String()},
+		StartSpanMatchersSlice: defaultStartSpanSignatures,
 	}
 }
 
@@ -83,6 +104,11 @@ func (c *Config) finalize() {
 
 // parseSignatures sets the Ignore*CheckSignatures regex from the string slices.
 func (c *Config) parseSignatures() {
+	c.parseIgnoreSignatures()
+	c.parseStartSpanSignatures()
+}
+
+func (c *Config) parseIgnoreSignatures() {
 	if c.ignoreChecksSignatures == nil && len(c.IgnoreChecksSignaturesSlice) > 0 {
 		if len(c.IgnoreChecksSignaturesSlice) == 1 && c.IgnoreChecksSignaturesSlice[0] == "" {
 			return
@@ -92,6 +118,62 @@ func (c *Config) parseSignatures() {
 	}
 }
 
+func (c *Config) parseStartSpanSignatures() {
+	if c.startSpanMatchers != nil {
+		return
+	}
+
+	customMatchers := []string{}
+	for i, sig := range c.StartSpanMatchersSlice {
+		parts := strings.Split(sig, ":")
+
+		// Make sure we have both a signature and a telemetry type
+		if len(parts) != startSpanSignatureCols {
+			log.Default().Printf("[WARN] invalid start span signature \"%s\". expected regex:telemetry-type\n", sig)
+
+			continue
+		}
+
+		sig, sigType := parts[0], parts[1]
+		if len(sig) < 1 {
+			log.Default().Print("[WARN] invalid start span signature, empty pattern")
+
+			continue
+		}
+
+		spanType, ok := SpanTypes[sigType]
+		if !ok {
+			validSpanTypes := make([]string, 0, len(SpanTypes))
+			for k := range SpanTypes {
+				validSpanTypes = append(validSpanTypes, k)
+			}
+
+			log.Default().
+				Printf("[WARN] invalid start span type \"%s\". expected one of %s\n", sigType, strings.Join(validSpanTypes, ", "))
+
+			continue
+		}
+
+		regex, err := regexp.Compile(sig)
+		if err != nil {
+			log.Default().Printf("[WARN] failed to compile regex from signature %s: %v\n", sig, err)
+
+			continue
+		}
+
+		c.startSpanMatchers = append(c.startSpanMatchers, spanStartMatcher{
+			signature: regex,
+			spanType:  spanType,
+		})
+
+		if i >= len(defaultStartSpanSignatures) {
+			customMatchers = append(customMatchers, sig)
+		}
+	}
+
+	c.startSpanMatchersCustomRegex = createRegex(customMatchers)
+}
+
 func parseChecks(checksSlice []string) []Check {
 	if len(checksSlice) == 0 {
 		return nil

diff --git a/spancheck.go b/spancheck.go
@@ -23,11 +23,15 @@ const (
 	spanOpenCensus                    // from go.opencensus.io/trace
 )
 
-var (
-	// this approach stolen from errcheck
-	// https://github.com/kisielk/errcheck/blob/7f94c385d0116ccc421fbb4709e4a484d98325ee/errcheck/errcheck.go#L22
-	errorType = types.Universe.Lookup("error").Type().Underlying().(*types.Interface)
-)
+// SpanTypes is a list of all span types by name.
+var SpanTypes = map[string]spanType{
+	"opentelemetry": spanOpenTelemetry,
+	"opencensus":    spanOpenCensus,
+}
+
+// this approach stolen from errcheck
+// https://github.com/kisielk/errcheck/blob/7f94c385d0116ccc421fbb4709e4a484d98325ee/errcheck/errcheck.go#L22
+var errorType = types.Universe.Lookup("error").Type().Underlying().(*types.Interface)
 
 // NewAnalyzerWithConfig returns a new analyzer configured with the Config passed in.
 // Its config can be set for testing.
@@ -84,6 +88,12 @@ func runFunc(pass *analysis.Pass, node ast.Node, config *Config) {
 		funcScope = pass.TypesInfo.Scopes[v.Type]
 	case *ast.FuncDecl:
 		funcScope = pass.TypesInfo.Scopes[v.Type]
+		fnSig := pass.TypesInfo.ObjectOf(v.Name).String()
+
+		// Skip checking spans in this function if it's a custom starter/creator.
+		if config.startSpanMatchersCustomRegex != nil && config.startSpanMatchersCustomRegex.MatchString(fnSig) {
+			return
+		}
 	}
 
 	// Maps each span variable to its defining ValueSpec/AssignStmt.
@@ -108,8 +118,12 @@ func runFunc(pass *analysis.Pass, node ast.Node, config *Config) {
 		//   ctx, span     := otel.Tracer("app").Start(...)
 		//   ctx, span     = otel.Tracer("app").Start(...)
 		//   var ctx, span = otel.Tracer("app").Start(...)
-		sType, sStart := isSpanStart(pass.TypesInfo, n)
-		if !sStart || !isCall(stack[len(stack)-2]) {
+		sType, isStart := isSpanStart(pass.TypesInfo, n, config.startSpanMatchers)
+		if !isStart {
+			return true
+		}
+
+		if !isCall(stack[len(stack)-2]) {
 			return true
 		}
 
@@ -169,23 +183,23 @@ func runFunc(pass *analysis.Pass, node ast.Node, config *Config) {
 	for _, sv := range spanVars {
 		if config.endCheckEnabled {
 			// Check if there's no End to the span.
-			if ret := getMissingSpanCalls(pass, g, sv, "End", func(pass *analysis.Pass, ret *ast.ReturnStmt) *ast.ReturnStmt { return ret }, nil); ret != nil {
+			if ret := getMissingSpanCalls(pass, g, sv, "End", func(_ *analysis.Pass, ret *ast.ReturnStmt) *ast.ReturnStmt { return ret }, nil, config.startSpanMatchers); ret != nil {
 				pass.ReportRangef(sv.stmt, "%s.End is not called on all paths, possible memory leak", sv.vr.Name())
 				pass.ReportRangef(ret, "return can be reached without calling %s.End", sv.vr.Name())
 			}
 		}
 
 		if config.setStatusEnabled {
 			// Check if there's no SetStatus to the span setting an error.
-			if ret := getMissingSpanCalls(pass, g, sv, "SetStatus", getErrorReturn, config.ignoreChecksSignatures); ret != nil {
+			if ret := getMissingSpanCalls(pass, g, sv, "SetStatus", getErrorReturn, config.ignoreChecksSignatures, config.startSpanMatchers); ret != nil {
 				pass.ReportRangef(sv.stmt, "%s.SetStatus is not called on all paths", sv.vr.Name())
 				pass.ReportRangef(ret, "return can be reached without calling %s.SetStatus", sv.vr.Name())
 			}
 		}
 
 		if config.recordErrorEnabled && sv.spanType == spanOpenTelemetry { // RecordError only exists in OpenTelemetry
 			// Check if there's no RecordError to the span setting an error.
-			if ret := getMissingSpanCalls(pass, g, sv, "RecordError", getErrorReturn, config.ignoreChecksSignatures); ret != nil {
+			if ret := getMissingSpanCalls(pass, g, sv, "RecordError", getErrorReturn, config.ignoreChecksSignatures, config.startSpanMatchers); ret != nil {
 				pass.ReportRangef(sv.stmt, "%s.RecordError is not called on all paths", sv.vr.Name())
 				pass.ReportRangef(ret, "return can be reached without calling %s.RecordError", sv.vr.Name())
 			}
@@ -194,25 +208,22 @@ func runFunc(pass *analysis.Pass, node ast.Node, config *Config) {
 }
 
 // isSpanStart reports whether n is tracer.Start()
-func isSpanStart(info *types.Info, n ast.Node) (spanType, bool) {
+func isSpanStart(info *types.Info, n ast.Node, startSpanMatchers []spanStartMatcher) (spanType, bool) {
 	sel, ok := n.(*ast.SelectorExpr)
 	if !ok {
 		return spanUnset, false
 	}
 
-	switch sel.Sel.Name {
-	case "Start": // https://github.com/open-telemetry/opentelemetry-go/blob/98b32a6c3a87fbee5d34c063b9096f416b250897/trace/trace.go#L523
-		obj, ok := info.Uses[sel.Sel]
-		return spanOpenTelemetry, ok && obj.Pkg().Path() == "go.opentelemetry.io/otel/trace"
-	case "StartSpan": // https://pkg.go.dev/go.opencensus.io/trace#StartSpan
-		obj, ok := info.Uses[sel.Sel]
-		return spanOpenCensus, ok && obj.Pkg().Path() == "go.opencensus.io/trace"
-	case "StartSpanWithRemoteParent": // https://github.com/census-instrumentation/opencensus-go/blob/v0.24.0/trace/trace_api.go#L66
-		obj, ok := info.Uses[sel.Sel]
-		return spanOpenCensus, ok && obj.Pkg().Path() == "go.opencensus.io/trace"
-	default:
-		return spanUnset, false
+	fnSig := info.ObjectOf(sel.Sel).String()
+
+	// Check if the function is a span start function
+	for _, matcher := range startSpanMatchers {
+		if matcher.signature.MatchString(fnSig) {
+			return matcher.spanType, true
+		}
 	}
+
+	return 0, false
 }
 
 func isCall(n ast.Node) bool {
@@ -225,11 +236,16 @@ func getID(node ast.Node) *ast.Ident {
 	case *ast.ValueSpec:
 		if len(stmt.Names) > 1 {
 			return stmt.Names[1]
+		} else if len(stmt.Names) == 1 {
+			return stmt.Names[0]
 		}
 	case *ast.AssignStmt:
 		if len(stmt.Lhs) > 1 {
 			id, _ := stmt.Lhs[1].(*ast.Ident)
 			return id
+		} else if len(stmt.Lhs) == 1 {
+			id, _ := stmt.Lhs[0].(*ast.Ident)
+			return id
 		}
 	}
 	return nil
@@ -244,13 +260,14 @@ func getMissingSpanCalls(
 	selName string,
 	checkErr func(pass *analysis.Pass, ret *ast.ReturnStmt) *ast.ReturnStmt,
 	ignoreCheckSig *regexp.Regexp,
+	spanStartMatchers []spanStartMatcher,
 ) *ast.ReturnStmt {
 	// blockUses computes "uses" for each block, caching the result.
 	memo := make(map[*cfg.Block]bool)
 	blockUses := func(pass *analysis.Pass, b *cfg.Block) bool {
 		res, ok := memo[b]
 		if !ok {
-			res = usesCall(pass, b.Nodes, sv, selName, ignoreCheckSig, 0)
+			res = usesCall(pass, b.Nodes, sv, selName, ignoreCheckSig, spanStartMatchers, 0)
 			memo[b] = res
 		}
 		return res
@@ -272,7 +289,7 @@ outer:
 	}
 
 	// Is the call "used" in the remainder of its defining block?
-	if usesCall(pass, rest, sv, selName, ignoreCheckSig, 0) {
+	if usesCall(pass, rest, sv, selName, ignoreCheckSig, spanStartMatchers, 0) {
 		return nil
 	}
 
@@ -314,7 +331,15 @@ outer:
 }
 
 // usesCall reports whether stmts contain a use of the selName call on variable v.
-func usesCall(pass *analysis.Pass, stmts []ast.Node, sv spanVar, selName string, ignoreCheckSig *regexp.Regexp, depth int) bool {
+func usesCall(
+	pass *analysis.Pass,
+	stmts []ast.Node,
+	sv spanVar,
+	selName string,
+	ignoreCheckSig *regexp.Regexp,
+	startSpanMatchers []spanStartMatcher,
+	depth int,
+) bool {
 	if depth > 1 { // for perf reasons, do not dive too deep thru func literals, just one level deep check.
 		return false
 	}
@@ -329,7 +354,7 @@ func usesCall(pass *analysis.Pass, stmts []ast.Node, sv spanVar, selName string,
 					cfgs := pass.ResultOf[ctrlflow.Analyzer].(*ctrlflow.CFGs)
 					g := cfgs.FuncLit(n)
 					if g != nil && len(g.Blocks) > 0 {
-						return usesCall(pass, g.Blocks[0].Nodes, sv, selName, ignoreCheckSig, depth+1)
+						return usesCall(pass, g.Blocks[0].Nodes, sv, selName, ignoreCheckSig, startSpanMatchers, depth+1)
 					}
 
 					return false
@@ -352,8 +377,8 @@ func usesCall(pass *analysis.Pass, stmts []ast.Node, sv spanVar, selName string,
 			stack = append(stack, n) // push
 
 			// Check whether the span was assigned over top of its old value.
-			_, spanStart := isSpanStart(pass.TypesInfo, n)
-			if spanStart {
+			_, isStart := isSpanStart(pass.TypesInfo, n, startSpanMatchers)
+			if isStart {
 				if id := getID(stack[len(stack)-3]); id != nil && id.Obj.Decl == sv.id.Obj.Decl {
 					reAssigned = true
 					return false