Add support for "Start" utility methods #16

trixnz · 2024-04-19T22:28:03Z

Fixes #7.

Replaces the list of names and packages for the Start span functions 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 against so we don't run RecordError on opencensus.

I've tested this on our rather large code base, and was pleased to see many new lints that we had previously missed after moving to a utility method.

This is a great lint check, thank you!

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.

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.

jjti · 2024-04-19T22:55:38Z

Thank you a ton, @trixnz! I've been feeling guilty about not having found the time to implement this... I'll review and merge ASAP (tomorrow AM)

coveralls · 2024-04-21T00:07:38Z

Pull Request Test Coverage Report for Build 8774843807

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

62 of 82 (75.61%) changed or added relevant lines in 2 files are covered.
3 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-3.3%) to 83.942%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
spancheck.go	33	34	97.06%
config.go	29	48	60.42%

Files with Coverage Reduction	New Missed Lines	%
spancheck.go	3	87.5%

Totals
Change from base Build 8105344262:	-3.3%
Covered Lines:	345
Relevant Lines:	411

💛 - Coveralls

jjti

@trixnz this looks dope. I added a couple tests and found a couple things that I had to change which I called out below. Let me know what you think of those changes.

Two other things I wanted to bring up:

are there any other tests you're aware of that we should throw into testdata/enableall/enable_all.go?
do you want to take a stab at documenting this new setting in the README? Up to you, I can in a follow up if you'd prefer

jjti · 2024-04-21T00:08:57Z

spancheck.go

@@ -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 {


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)

Good catch, potential for future robustness work here!

jjti · 2024-04-21T00:11:07Z

testdata/enableall/util/start_tracer.go

+	"go.opentelemetry.io/otel/trace"
+)
+
+func TestStartTrace() trace.Span {


this is another limitation I found with the existing implementation of spancheck's isSpanStartfunc. It will only work with SelectorExpr which means the span starting utility functions need to be in a package different from the one being linted. To test that in this function I just made a new /util package. But isSpanStart should probably be updated to also allow for utility funcs within the same package.

func isSpanStart(info *types.Info, n ast.Node, startSpanMatchers []spanStartMatcher) (spanType, bool) { sel, ok := n.(*ast.SelectorExpr) if !ok { return spanUnset, false }

I did notice this, I agree - it might miss a few calls in places where the tracing packages themselves are misbehaving with spans.

jjti · 2024-04-21T00:26:05Z

spancheck.go

@@ -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) {


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

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.

Instead of checking if the function name matches, which might result in overzealous exclusions, prefer to match against the function signature itself

trixnz · 2024-04-21T17:58:17Z

@trixnz this looks dope. I added a couple tests and found a couple things that I had to change which I called out below. Let me know what you think of those changes.

Two other things I wanted to bring up:

are there any other tests you're aware of that we should throw into testdata/enableall/enable_all.go?

do you want to take a stab at documenting this new setting in the README? Up to you, I can in a follow up if you'd prefer

Looks good to me! I took a pass at documentation, let me know what you think 😄

jjti · 2024-04-21T19:09:43Z

Just merged @trixnz! I'll need to make a follow-up to golangci-lint to get that YAML config thing in. I also added a shout out in the README for your help. Thank you again! Please lmk if this works for you (or not and we can patch it)

trixnz · 2024-04-21T19:32:09Z

How gracious, thanks! 😄 Looks great on my end, I appreciate the suggestions and how quick you were to land this. Once again, great idea for a tool - love it!

Crocmagnon · 2024-04-22T08:06:25Z

Thanks!

trixnz added 2 commits April 20, 2024 00:20

Add test, skip validating spans within starter funcs

995dd9d

Hack: fix check for start func method name

952565d

jjti reviewed Apr 21, 2024

View reviewed changes

trixnz added 2 commits April 21, 2024 19:22

fix: increase robustness of skipping custom matchers

a04fa0a

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

5be5d87

jjti added 2 commits April 21, 2024 14:45

Remove sig split in config

225f854

nit: small wording changes

3d1cb1a

jjti merged commit 34d21b4 into jjti:main Apr 21, 2024
2 of 3 checks passed

trixnz deleted the feat/add-start-span-matchers branch April 21, 2024 19:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for "Start" utility methods #16

Add support for "Start" utility methods #16

trixnz commented Apr 19, 2024

jjti commented Apr 19, 2024

coveralls commented Apr 21, 2024 •

edited

Loading

jjti left a comment •

edited

Loading

jjti Apr 21, 2024 •

edited

Loading

trixnz Apr 21, 2024

jjti Apr 21, 2024

trixnz Apr 21, 2024

jjti Apr 21, 2024

trixnz Apr 21, 2024

trixnz commented Apr 21, 2024

jjti commented Apr 21, 2024

trixnz commented Apr 21, 2024

Crocmagnon commented Apr 22, 2024

Add support for "Start" utility methods #16

Add support for "Start" utility methods #16

Conversation

trixnz commented Apr 19, 2024

jjti commented Apr 19, 2024

coveralls commented Apr 21, 2024 • edited Loading

Pull Request Test Coverage Report for Build 8774843807

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

jjti left a comment • edited Loading

Choose a reason for hiding this comment

jjti Apr 21, 2024 • edited Loading

Choose a reason for hiding this comment

trixnz Apr 21, 2024

Choose a reason for hiding this comment

jjti Apr 21, 2024

Choose a reason for hiding this comment

trixnz Apr 21, 2024

Choose a reason for hiding this comment

jjti Apr 21, 2024

Choose a reason for hiding this comment

trixnz Apr 21, 2024

Choose a reason for hiding this comment

trixnz commented Apr 21, 2024

jjti commented Apr 21, 2024

trixnz commented Apr 21, 2024

Crocmagnon commented Apr 22, 2024

coveralls commented Apr 21, 2024 •

edited

Loading

jjti left a comment •

edited

Loading

jjti Apr 21, 2024 •

edited

Loading