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

Add support for "Start" utility methods #16

Merged
merged 8 commits into from
Apr 21, 2024
Merged

Add support for "Start" utility methods #16

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ac45587
feat: replace hardcoded span start function names with regex patterns
trixnz Apr 19, 2024
f969e4e
tests: update tests to use new default config
trixnz Apr 19, 2024
995dd9d
Add test, skip validating spans within starter funcs
jjti Apr 21, 2024
952565d
Hack: fix check for start func method name
jjti Apr 21, 2024
a04fa0a
fix: increase robustness of skipping custom matchers
trixnz Apr 21, 2024
5be5d87
docs: document new behaviour
trixnz Apr 21, 2024
225f854
Remove sig split in config
jjti Apr 21, 2024
3d1cb1a
nit: small wording changes
jjti Apr 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions cmd/spancheck/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}
103 changes: 94 additions & 9 deletions config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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 {
Expand All @@ -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,
}
}

Expand All @@ -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
Expand All @@ -92,6 +118,65 @@ 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]

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)
}
}

if len(customMatchers) == 0 {
return
}

customStartRegex, err := regexp.Compile(fmt.Sprintf("(%s)", strings.Join(customMatchers, "|")))
if err != nil {
log.Default().Printf("[WARN] failed to compile regex from combo of customer start signatures %v: %v\n", customMatchers, err)
} else {
c.startSpanMatchersCustomRegex = customStartRegex
}
}

func parseChecks(checksSlice []string) []Check {
if len(checksSlice) == 0 {
return nil
Expand Down
74 changes: 45 additions & 29 deletions spancheck.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -84,6 +88,11 @@ 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]

// Skip checking spans in this function if it's a custom starter/creator.
if config.startSpanMatchersCustomRegex != nil && config.startSpanMatchersCustomRegex.MatchString(v.Name.Name) {
Copy link
Owner

Choose a reason for hiding this comment

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

this is another hack/workaround I had to throw in. Without it, we'll get linting exceptions for the func that defines the spans. Like the function below (just a quick example from memory, not exact) would get a linting error because it itself is in violation of the other rules like calling .End() on spans

func startSpan() trace.Span { // 
  _, span := otel.StartTrace()
  return span
} // return reached without calling span.SetStatus

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice one! We can remove a few nolint from our codebase :). I updated your check to match the regex against the full function signature, rather than just the function name itself, as that might unintentionally hide lints from functions with the same name as utility methods.

return
}
}

// Maps each span variable to its defining ValueSpec/AssignStmt.
Expand All @@ -108,8 +117,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
}

Expand Down Expand Up @@ -169,23 +182,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())
}
Expand All @@ -194,25 +207,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 {
Expand All @@ -225,11 +235,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 {
Copy link
Owner

@jjti jjti Apr 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

right now I'm hard-coding the index of the span to get its ID. For the existing libraries, it was as simple as grabbing the second var (since that's the case for all the funcs being tested from opencensus and otel). For these new/custom func signatures, we don't know which index the returned span is. What I did here is the quickest dirtiest fix, to grab the span's ID assuming it's the only return value (if only one var is being assigned). So this works with funcs like I use in the test:

func startTrace() trace.Span {
  ...
}

If would be better to walk thru the stmts and find whatever var is of a span type... (supporting more arbitrary custom span start functions)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, potential for future robustness work here!

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
Expand All @@ -244,13 +259,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
Expand All @@ -272,7 +288,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
}

Expand Down Expand Up @@ -314,7 +330,7 @@ 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
}
Expand All @@ -329,7 +345,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
Expand All @@ -352,8 +368,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
Expand Down
32 changes: 22 additions & 10 deletions spancheck_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,27 +11,39 @@ import (
func Test(t *testing.T) {
t.Parallel()

for dir, config := range map[string]*spancheck.Config{
"base": spancheck.NewDefaultConfig(),
"disableerrorchecks": {
EnabledChecks: []string{
type configFactory func() *spancheck.Config

for dir, configFactory := range map[string]configFactory{
"base": spancheck.NewDefaultConfig,
"disableerrorchecks": func() *spancheck.Config {
cfg := spancheck.NewDefaultConfig()
cfg.EnabledChecks = []string{
spancheck.EndCheck.String(),
spancheck.RecordErrorCheck.String(),
spancheck.SetStatusCheck.String(),
},
IgnoreChecksSignaturesSlice: []string{"telemetry.Record", "recordErr"},
}
cfg.IgnoreChecksSignaturesSlice = []string{"telemetry.Record", "recordErr"}

return cfg
},
"enableall": {
EnabledChecks: []string{
"enableall": func() *spancheck.Config {
cfg := spancheck.NewDefaultConfig()
cfg.EnabledChecks = []string{
spancheck.EndCheck.String(),
spancheck.RecordErrorCheck.String(),
spancheck.SetStatusCheck.String(),
},
}
cfg.StartSpanMatchersSlice = append(cfg.StartSpanMatchersSlice,
"util.TestStartTrace:opentelemetry",
"testStartTrace:opencensus",
)

return cfg
},
} {
dir := dir
t.Run(dir, func(t *testing.T) {
analysistest.Run(t, "testdata/"+dir, spancheck.NewAnalyzerWithConfig(config))
analysistest.Run(t, "testdata/"+dir, spancheck.NewAnalyzerWithConfig(configFactory()))
})
}
}
Loading
Loading