Skip to content

Commit 1186309

Browse files
committed
test(telemetry): Add a custom linter for checking otel tracing event and attribute names
1 parent 31c9f1e commit 1186309

File tree

11 files changed

+635
-0
lines changed

11 files changed

+635
-0
lines changed
File renamed without changes.

magefiles/lint.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,7 @@ func (Lint) Analyzers() error {
9191
"-zerologmarshalcheck",
9292
"-zerologmarshalcheck.skip-files=_test,zz_",
9393
"-protomarshalcheck",
94+
"-telemetryconvcheck",
9495
// Skip generated protobuf files for this check
9596
// Also skip test where we're explicitly using proto.Marshal to assert
9697
// that the proto.Marshal behavior matches foo.MarshalVT()

tools/analyzers/cmd/analyzers/main.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"github.com/authzed/spicedb/tools/analyzers/nilvaluecheck"
99
"github.com/authzed/spicedb/tools/analyzers/paniccheck"
1010
"github.com/authzed/spicedb/tools/analyzers/protomarshalcheck"
11+
"github.com/authzed/spicedb/tools/analyzers/telemetryconvcheck"
1112
"github.com/authzed/spicedb/tools/analyzers/zerologmarshalcheck"
1213
"golang.org/x/tools/go/analysis/multichecker"
1314
)
@@ -22,5 +23,6 @@ func main() {
2223
lendowncastcheck.Analyzer(),
2324
protomarshalcheck.Analyzer(),
2425
zerologmarshalcheck.Analyzer(),
26+
telemetryconvcheck.Analyzer(),
2527
)
2628
}
Lines changed: 228 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,228 @@
1+
package telemetryconvcheck
2+
3+
import (
4+
"flag"
5+
"go/ast"
6+
"go/constant"
7+
"go/token"
8+
"go/types"
9+
"slices"
10+
"strings"
11+
12+
"github.com/samber/lo"
13+
"golang.org/x/tools/go/analysis"
14+
"golang.org/x/tools/go/analysis/passes/inspect"
15+
"golang.org/x/tools/go/ast/inspector"
16+
)
17+
18+
const defaultConvPackage = "github.com/authzed/spicedb/internal/telemetry/otelconv"
19+
20+
var (
21+
validPrefixes = []string{"spicedb.internal.", "spicedb.external."}
22+
convPackage *string
23+
)
24+
25+
func Analyzer() *analysis.Analyzer {
26+
flagSet := flag.NewFlagSet("telemetryconvcheck", flag.ExitOnError)
27+
skip := flagSet.String("skip-pkg", "", "package(s) to skip for linting")
28+
convPackage = flagSet.String("conv-pkg", defaultConvPackage, "package that should contain the constants to use instead of the hardcoded strings")
29+
30+
return &analysis.Analyzer{
31+
Name: "telemetryconvcheck",
32+
Doc: "disallow OpenTelemetry event and attribute names that use hardcoded strings instead of the constants from otelconv",
33+
Run: func(pass *analysis.Pass) (any, error) {
34+
// Check for a skipped package
35+
if len(*skip) > 0 {
36+
skipped := lo.Map(strings.Split(*skip, ","), func(skipped string, _ int) string { return strings.TrimSpace(skipped) })
37+
for _, s := range skipped {
38+
if strings.Contains(pass.Pkg.Path(), s) {
39+
return nil, nil
40+
}
41+
}
42+
}
43+
44+
inspectorInst := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
45+
inspectorInst.Preorder([]ast.Node{(*ast.CallExpr)(nil)}, func(node ast.Node) {
46+
callExpr := node.(*ast.CallExpr)
47+
functionName := getFunctionName(callExpr)
48+
if functionName == "" {
49+
return
50+
}
51+
52+
if !(isOtelTracingCall(pass, callExpr) || containsPattern(functionName, otelTracingFunctionPatterns)) {
53+
return
54+
}
55+
56+
checkArguments(pass, callExpr, functionName)
57+
})
58+
59+
return nil, nil
60+
},
61+
Requires: []*analysis.Analyzer{inspect.Analyzer},
62+
Flags: *flagSet,
63+
}
64+
}
65+
66+
var otelTracingFunctionPatterns = []string{
67+
// Event methods
68+
"AddEvent",
69+
70+
// Attribute creation functions
71+
"attribute.String",
72+
"attribute.Int",
73+
"attribute.Int64",
74+
"attribute.Float64",
75+
"attribute.Bool",
76+
"attribute.StringSlice",
77+
"attribute.IntSlice",
78+
"attribute.Int64Slice",
79+
"attribute.Float64Slice",
80+
"attribute.BoolSlice",
81+
82+
// Attribute setting methods
83+
"SetAttributes",
84+
"WithAttributes",
85+
}
86+
87+
func containsPattern(functionName string, patterns []string) bool {
88+
for _, pattern := range patterns {
89+
if strings.Contains(functionName, pattern) {
90+
return true
91+
}
92+
}
93+
return false
94+
}
95+
96+
func isOtelTracingCall(pass *analysis.Pass, callExpr *ast.CallExpr) bool {
97+
selExpr, ok := callExpr.Fun.(*ast.SelectorExpr)
98+
if !ok {
99+
return false
100+
}
101+
102+
typ := pass.TypesInfo.TypeOf(selExpr.X)
103+
if typ == nil {
104+
return false
105+
}
106+
107+
typeName := typ.String()
108+
return strings.Contains(typeName, "go.opentelemetry.io/otel/trace") ||
109+
strings.Contains(typeName, "trace.Span") ||
110+
strings.Contains(typeName, "attribute.Key")
111+
}
112+
113+
func getFunctionName(callExpr *ast.CallExpr) string {
114+
switch fun := callExpr.Fun.(type) {
115+
case *ast.Ident:
116+
return fun.Name
117+
case *ast.SelectorExpr:
118+
if ident, ok := fun.X.(*ast.Ident); ok {
119+
return ident.Name + "." + fun.Sel.Name
120+
}
121+
return fun.Sel.Name
122+
default:
123+
return ""
124+
}
125+
}
126+
127+
func checkArguments(pass *analysis.Pass, callExpr *ast.CallExpr, functionName string) {
128+
switch {
129+
case strings.Contains(functionName, "AddEvent"):
130+
if len(callExpr.Args) > 0 {
131+
checkStringArgument(pass, callExpr.Args[0], "event name")
132+
}
133+
if len(callExpr.Args) > 1 {
134+
for _, arg := range callExpr.Args[1:] {
135+
if optCall, ok := arg.(*ast.CallExpr); ok {
136+
if name := getFunctionName(optCall); name == "WithAttributes" || name == "trace.WithAttributes" {
137+
checkAttributes(pass, optCall.Args)
138+
}
139+
}
140+
}
141+
}
142+
case strings.Contains(functionName, "SetAttributes") || strings.Contains(functionName, "WithAttributes"):
143+
checkAttributes(pass, callExpr.Args)
144+
case strings.Contains(functionName, "attribute."):
145+
if len(callExpr.Args) > 0 {
146+
checkStringArgument(pass, callExpr.Args[0], "attribute key")
147+
}
148+
}
149+
}
150+
151+
func checkAttributes(pass *analysis.Pass, args []ast.Expr) {
152+
for _, arg := range args {
153+
ast.Inspect(arg, func(node ast.Node) bool {
154+
callExpr, ok := node.(*ast.CallExpr)
155+
if !ok {
156+
return true
157+
}
158+
159+
functionName := getFunctionName(callExpr)
160+
if strings.Contains(functionName, "attribute.") && len(callExpr.Args) > 0 {
161+
checkStringArgument(pass, callExpr.Args[0], "attribute key")
162+
}
163+
return true
164+
})
165+
}
166+
}
167+
168+
func checkStringArgument(pass *analysis.Pass, arg ast.Expr, description string) {
169+
if lit, ok := arg.(*ast.BasicLit); ok && lit.Kind == token.STRING {
170+
pass.Reportf(arg.Pos(), "use a constant from github.com/authzed/spicedb/internal/telemetry/otelconv for %s instead of the hardcoded string %s", description, lit.Value)
171+
return
172+
}
173+
174+
var ident *ast.Ident
175+
switch n := arg.(type) {
176+
case *ast.Ident:
177+
ident = n
178+
case *ast.SelectorExpr:
179+
ident = n.Sel
180+
default:
181+
return
182+
}
183+
184+
obj := pass.TypesInfo.ObjectOf(ident)
185+
if obj == nil {
186+
return
187+
}
188+
189+
if _, ok := obj.(*types.Const); !ok {
190+
return
191+
}
192+
193+
pkg := obj.Pkg()
194+
if pkg == nil || pkg.Path() != *convPackage {
195+
var pkgPath string
196+
if pkg != nil {
197+
pkgPath = pkg.Path()
198+
} else {
199+
pkgPath = "the current package"
200+
}
201+
pass.Reportf(
202+
arg.Pos(),
203+
"use a constant from `%s` for %s, not from `%s`",
204+
*convPackage,
205+
description,
206+
pkgPath,
207+
)
208+
}
209+
210+
if constObj, ok := obj.(*types.Const); ok {
211+
if constObj.Val().Kind() != constant.String {
212+
pass.Reportf(arg.Pos(), "constant %s from %s should be a string, but has type %s", ident.Name, *convPackage, constObj.Type().String())
213+
}
214+
constValue := constant.StringVal(constObj.Val())
215+
if !slices.ContainsFunc(validPrefixes, func(prefix string) bool {
216+
return strings.HasPrefix(constValue, prefix)
217+
}) {
218+
pass.Reportf(
219+
arg.Pos(),
220+
"constant %s from %s should start with one of %q, but has value %q",
221+
ident.Name,
222+
*convPackage,
223+
validPrefixes,
224+
constValue,
225+
)
226+
}
227+
}
228+
}
Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
package telemetryconvcheck
2+
3+
import (
4+
"testing"
5+
6+
"golang.org/x/tools/go/analysis/analysistest"
7+
)
8+
9+
func TestAnalyzer(t *testing.T) {
10+
analyzer := Analyzer()
11+
12+
testdata := analysistest.TestData()
13+
14+
// Test original functionality
15+
analysistest.Run(t, testdata, analyzer, "validtelemetry")
16+
analysistest.Run(t, testdata, analyzer, "invalidtelemetry")
17+
18+
// Test prefix check
19+
analysistest.Run(t, testdata, analyzer, "validprefixes")
20+
analysistest.Run(t, testdata, analyzer, "invalidprefixes")
21+
}
Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
package invalidprefixes
2+
3+
// Placeholder types
4+
type (
5+
Span struct{}
6+
KeyValue struct{}
7+
SpanStartOption struct{}
8+
)
9+
10+
// Placeholder packages
11+
var attribute attributePackage
12+
var trace tracePackage
13+
14+
type attributePackage struct{}
15+
type tracePackage struct{}
16+
17+
// Placeholder functions
18+
func (s Span) AddEvent(name string, options ...interface{}) {}
19+
func (s Span) SetAttributes(attributes ...KeyValue) {}
20+
21+
func (a attributePackage) String(key string, value string) KeyValue { return KeyValue{} }
22+
func (a attributePackage) Int64(key string, value int64) KeyValue { return KeyValue{} }
23+
func (a attributePackage) Bool(key string, value bool) KeyValue { return KeyValue{} }
24+
func (a attributePackage) StringSlice(key string, values []string) KeyValue { return KeyValue{} }
25+
func (a attributePackage) Int64Slice(key string, values []int64) KeyValue { return KeyValue{} }
26+
func (a attributePackage) BoolSlice(key string, values []bool) KeyValue { return KeyValue{} }
27+
func (t tracePackage) WithAttributes(attributes ...KeyValue) SpanStartOption {
28+
return SpanStartOption{}
29+
}
30+
31+
// Test function using a hardcoded string with an invalid prefix
32+
func invalidPrefixesInEvents(span Span) {
33+
span.AddEvent("spicedb.caveats.names_collected") // want "use a constant from github.com/authzed/spicedb/internal/telemetry/otelconv for event name instead of the hardcoded string \"spicedb.caveats.names_collected\""
34+
}
Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
package _invalidtelemetry
2+
3+
// Placeholder types
4+
type (
5+
Span struct{}
6+
KeyValue struct{}
7+
SpanStartOption struct{}
8+
)
9+
10+
// Placeholder packages
11+
var attribute attributePackage
12+
var trace tracePackage
13+
14+
type attributePackage struct{}
15+
type tracePackage struct{}
16+
17+
// Placeholder functions
18+
func (s Span) AddEvent(name string, options ...interface{}) {}
19+
func (s Span) SetAttributes(attributes ...KeyValue) {}
20+
21+
func (a attributePackage) String(key string, value string) KeyValue { return KeyValue{} }
22+
func (a attributePackage) Int64(key string, value int64) KeyValue { return KeyValue{} }
23+
func (a attributePackage) Bool(key string, value bool) KeyValue { return KeyValue{} }
24+
func (a attributePackage) StringSlice(key string, values []string) KeyValue { return KeyValue{} }
25+
func (a attributePackage) Int64Slice(key string, values []int64) KeyValue { return KeyValue{} }
26+
func (a attributePackage) BoolSlice(key string, values []bool) KeyValue { return KeyValue{} }
27+
func (t tracePackage) WithAttributes(attributes ...KeyValue) SpanStartOption {
28+
return SpanStartOption{}
29+
}
30+
31+
func invalidEventNames(span Span) {
32+
span.AddEvent("spicedb.datastore.read") // want "use a constant from github.com/authzed/spicedb/internal/telemetry/otelconv for event name instead of the hardcoded string \"spicedb.datastore.read\""
33+
}
34+
35+
func invalidAttributeKeys() []KeyValue {
36+
return []KeyValue{
37+
attribute.String("spicedb.datastore.operation", "read"), // want "use a constant from github.com/authzed/spicedb/internal/telemetry/otelconv for attribute key instead of the hardcoded string \"spicedb.datastore.operation\""
38+
}
39+
}
40+
41+
func invalidSetAttributes(span Span) {
42+
attr := attribute.String("spicedb.datastore.operation", "write") // want "use a constant from github.com/authzed/spicedb/internal/telemetry/otelconv for attribute key instead of the hardcoded string \"spicedb.datastore.operation\""
43+
span.SetAttributes(attr)
44+
}
45+
46+
func invalidWithAttributes() SpanStartOption {
47+
attr := attribute.String("spicedb.datastore.operation", "query") // want "use a constant from github.com/authzed/spicedb/internal/telemetry/otelconv for attribute key instead of the hardcoded string \"spicedb.datastore.operation\""
48+
return trace.WithAttributes(attr)
49+
}
50+
51+
func invalidAttributeSlices() []KeyValue {
52+
return []KeyValue{
53+
attribute.StringSlice("spicedb.datastore.operations", []string{"read", "write"}), // want "use a constant from github.com/authzed/spicedb/internal/telemetry/otelconv for attribute key instead of the hardcoded string \"spicedb.datastore.operations\""
54+
}
55+
}
56+
57+
func invalidCompositeLiterals() []KeyValue {
58+
attrs := []KeyValue{
59+
attribute.String("spicedb.datastore.operation", "read"), // want "use a constant from github.com/authzed/spicedb/internal/telemetry/otelconv for attribute key instead of the hardcoded string \"spicedb.datastore.operation\""
60+
}
61+
return attrs
62+
}

0 commit comments

Comments
 (0)