feat: add spancheck linter

[jjti]

Background

This adds a spancheck linter: https://github.com/jjti/go-spancheck

The linter checks usage of OpenTelemetry spans, which are easy to get wrong. Self-instrumented traces requires a lot of easy-to-forget boilerplate:

import (
	"go.opentelemetry.io/otel"
	"go.opentelemetry.io/otel/codes"
)

func task(ctx context.Context) error {
    ctx, span := otel.Tracer("foo").Start(ctx, "bar")
    defer span.End() // call `.End()`

    if err := subTask(ctx); err != nil {
        span.SetStatus(codes.Error, err.Error()) // call SetStatus(codes.Error, msg) to set status:error
        span.RecordError(err) // call RecordError(err) to record an error event
        return err
    }

    return nil
}

Checks for spans include:

1. checking that span.End() is called (by default, not having this can cause memory leaks, and spans never being closed/useful)
2. optionally checking span.SetStatus(codes.Error, msg) is called on error
3. optionally checking span.RecordError(err) is called on error

Testing

Like protogetter I have an external dependency (otel) so made spancheckers tests their own module in test/testdata/spancheck/spancheck.go

$ T=spancheck make test_linters 
GL_TEST_RUN=1 go test -v ./test -count 1 -run TestSourcesFromTestdata/spancheck
=== RUN   TestSourcesFromTestdata
    linters_test.go:21: testdata/*.go
--- PASS: TestSourcesFromTestdata (0.30s)
=== RUN   TestSourcesFromTestdataSubDir
=== RUN   TestSourcesFromTestdataSubDir/spancheck
    linters_test.go:41: testdata/spancheck/*.go
=== RUN   TestSourcesFromTestdataSubDir/spancheck/spancheck_default.go
=== PAUSE TestSourcesFromTestdataSubDir/spancheck/spancheck_default.go
=== RUN   TestSourcesFromTestdataSubDir/spancheck/spancheck_enable_all_copy.go
=== PAUSE TestSourcesFromTestdataSubDir/spancheck/spancheck_enable_all_copy.go
=== CONT  TestSourcesFromTestdataSubDir/spancheck/spancheck_default.go
=== CONT  TestSourcesFromTestdataSubDir/spancheck/spancheck_enable_all_copy.go
level=info msg="[test] ran [/Users/josh/GitHub/golangci/golangci-lint/golangci-lint run --internal-cmd-test -c configs/enable_all.yml --allow-parallel-runners --disable-all --out-format=json --max-same-issues=100 --disable-all -Espancheck spancheck_enable_all copy.go] in 734.076417ms"
level=info msg="[test] ran [/Users/josh/GitHub/golangci/golangci-lint/golangci-lint run --internal-cmd-test -c configs/default.yml --allow-parallel-runners --disable-all --out-format=json --max-same-issues=100 --disable-all -Espancheck spancheck_default.go] in 770.173792ms"
level=info msg="[test] ran [/Users/josh/GitHub/golangci/golangci-lint/golangci-lint run --internal-cmd-test -c configs/enable_all.yml --allow-parallel-runners --disable-all --out-format=json --max-same-issues=100 -Etypecheck --disable-all -Espancheck spancheck_enable_all copy.go] in 307.45575ms"
level=info msg="[test] ran [/Users/josh/GitHub/golangci/golangci-lint/golangci-lint run --internal-cmd-test -c configs/default.yml --allow-parallel-runners --disable-all --out-format=json --max-same-issues=100 -Etypecheck --disable-all -Espancheck spancheck_default.go] in 322.52025ms"
--- PASS: TestSourcesFromTestdataSubDir (1.40s)
    --- PASS: TestSourcesFromTestdataSubDir/spancheck (0.31s)
        --- PASS: TestSourcesFromTestdataSubDir/spancheck/spancheck_enable_all_copy.go (1.04s)
        --- PASS: TestSourcesFromTestdataSubDir/spancheck/spancheck_default.go (1.09s)
PASS
ok      github.com/golangci/golangci-lint/test  2.112s

Fixes #3722

[boring-cyborg]

Hey, thank you for opening your first Pull Request !

[CLAassistant]

All committers have signed the CLA.

[ldez]

In order for a pull request adding a linter to be reviewed, the linter and the PR must follow some requirements.

• The CLA must be signed

Pull Request Description

• It must have a link to the linter repository.
• It must provide a short description of the linter.

Linter

• It must not be a duplicate of another linter or a rule of a linter. (the team will help to verify that)
• It must have a valid license (AGPL is not allowed) and the file must contain the required information by the license, ex: author, year, etc.
• The linter repository must have a CI and tests.
• It must use go/analysis.
• It must have a valid tag, ex: v1.0.0, v0.1.0.
• It must not contain init().
• It must not contain panic().
• It must not contain log.fatal(), os.exit(), or similar.
• It must not modify the AST.
• It must not have false positives/negatives. (the team will help to verify that)
• It must have tests inside golangci-lint.

The Linter Tests Inside Golangci-lint

• They must have at least one std lib import.
• They must work with T=<name of the linter test file>.go make test_linters. (the team will help to verify that)

.golangci.reference.yml

• The linter must be added to the list of available linters (alphabetical case-insensitive order).
  • enable and disable options
• If the linter has a configuration, the exhaustive configuration of the linter must be added (alphabetical case-insensitive order)
  • The values must be different from the default ones.
  • The default values must be defined in a comment.
  • The option must have a short description.

Others Requirements

• The files (tests and linter) inside golangci-lint must have the same name as the linter.
• The .golangci.yml of golangci-lint itself must not be edited and the linter must not be added to this file.
• The linters must be sorted in the alphabetical order (case-insensitive) in the Manager.GetAllSupportedLinterConfigs(...) and .golangci.reference.yml.
• The load mode (WithLoadMode(...)):
  • if the linter doesn't use types: goanalysis.LoadModeSyntax
  • goanalysis.LoadModeTypesInfo required WithLoadForGoAnalysis() in the Manager.GetAllSupportedLinterConfigs(...)
• The version in WithSince(...) must be the next minor version (v1.X.0) of golangci-lint.
• WithURL() must contain the URL of the repository.

Recommendations

• The linter should not use SSA. (SSA can be a problem with generics)
• The linter repository should have a readme and linting.
• The linter should be published as a binary. (useful to diagnose bug origins)

---

The golangci-lint team will edit this comment to check the boxes before and during the review.

The code review will start after the completion of those checkboxes (except for the specific items that the team will help to verify).

If the author of the PR is a member of the golangci-lint team, he should not edit this message.

This checklist does not imply that we will accept the linter.

[Antonboom]

Related to

#3722

[jjti]

Related to

#3722

Hey, @Antonboom. I totally agree they're related. Both check usage of OpenTelemetry traces (which are difficult to use and get right). They have quite different goals and checks, though. I tried to describe the differences below.

The summary of below is: go-spancheck checks that spans are used "correctly" after they're created. End() is called and, for the purists that enable it, SetStatus and RecordError are called as well. go-opentelemetry-lint checks that they're created in the first place.

• instantiation: polyfloyd/go-opentelemetry-lint throws warnings for every function that does not create a span. go-spancheck does not
• usage - End(): go-spancheck checks that spans are End()'ed, which helps prevent memory leaks. This is important. I've seen memory leaks of this flavor crash services from OOM-kills. Calling End is important and called out as such in the OTEL SDK
• usage - SetStatus(): go-spancheck checks that SetStatus is called when a function with a span returns an error. The rational for this is described in the README. In short: retention and searching for error traces is often based on status
• usage - RecordError(): go-spancheck checks that RecordError() is called with a function with a span returns an error. This is described in the README. This creates an event on the span that also helps with debugging.

Different goals

The goals of the linters are different enough to where I don't know if we could/should combine them. My 2c is that not every function that takes a context.Context should have a span. That leads to large traces, with lots of spans, and lots of span boilerplate, but without adding much context besides reproducing the call stack in traces. I.e. I would, in the projects I work on, recommend against one-span-per-function.

My personal experience has been that a small number of spans, each liberally annotated with SetAttributes, SetStatus, and RecordError, is more useful than many sparsely annotated spans.

[ldez]

The names of the options (enable-all, disable-end-check, enable-record-error-check, enable-set-status-check) are a bit weird:

• there is no list of checks to enable, and all checks (except one) are enabled by default, so enable-all seems off-topic and gives the feeling of unneeded complexity. And the fact this option overrides settings is confusing and feels useless.
• the option names that start with enable- or disable- are a bit redundant, I expect something like end-check, record-error-check, set-status-check with a simple boolean with the right default (true or false).

[jjti]

• there is no list of checks to enable

Point taken @ldez re: there being no list, and the settings being a bit verbose. I just changed settings to leave only a checks list of strings. Now the settings are down to just two fields:

type SpancheckSettings struct {
	Checks                []string `mapstructure:"checks"`
	IgnoreCheckSignatures []string `mapstructure:"ignore-check-signatures"`
}

If unset, Checks defaults to [end] alone

[jjti]

@ldez I see the slices issue with go 1.20, fixing now

[ldez]

You are using the package slices that is not available in go1.20 (the current min version for golangci-lint)
https://github.com/jjti/go-spancheck/blob/e6faa1d972f28050ae0dc13c79ab6f75021e5df2/config.go#L8

Also, your CI configuration should use a fixed minor version of Go to ensure the Go min version supported by your linter, the current CI configuration doesn't allow that:
https://github.com/jjti/go-spancheck/blob/e6faa1d972f28050ae0dc13c79ab6f75021e5df2/.github/workflows/ci.yaml#L11
At least you can use the alias oldstable as a version on the CI.

[ldez]

I recommend using 1.20 (fixed minor version without patch) or oldstable instead of 1.20.12 (fixed "patched" version).
https://github.com/jjti/go-spancheck/blob/40b71645c36eea47962a73e061ef261fbd2d5a4d/.github/workflows/ci.yaml#L11C15-L11C22

[jjti]

I recommend using 1.20 (fixed minor version without patch) or oldstable

Makes sense and thanks for that tip, I was unaware of that alias. Just made the switch in ci and confirmed it passed for spancheck v0.4.2 which doesn't have the slices pkg

[ldez]

It's always frustrating not to be able to use the latest language features, but this is the price of the compatibility with the previous Go version.
when go1.22 will be GA, I hope soon, you (and me 😸) will be able to use the slices package.

[ldez]

LGTM