lint: Revamp our linting rules, mostly around doc comments

Several things done here:

- Set `max-issues-per-linter` to 0 so that we actually see all linter
  warnings and not just 50 per linter. (As we also set
  `max-same-issues` to 0, I assume this was the intention from the
  beginning.)

- Stop using the golangci-lint default excludes (by setting
  `exclude-use-default: false`. Those are too generous and don't match
  our style conventions. (I have re-added some of the excludes
  explicitly in this commit. See below.)

- Re-add the `errcheck` exclusion we have used so far via the
  defaults.

- Exclude the signature requirement `govet` has for `Seek` methods
  because we use non-standard `Seek` methods a lot. (But we keep other
  requirements, while the default excludes completely disabled the
  check for common method segnatures.)

- Exclude warnings about missing doc comments on exported symbols. (We
  used to be pretty adamant about doc comments, but stopped that at
  some point in the past. By now, we have about 500 missing doc
  comments. We may consider reintroducing this check, but that's
  outside of the scope of this commit. The default excludes of
  golangci-lint essentially ignore doc comments completely.)

- By stop using the default excludes, we now get warnings back on
  malformed doc comments. That's the most impactful change in this
  commit. It does not enforce doc comments (again), but _if_ there is
  a doc comment, it has to have the recommended form. (Most of the
  changes in this commit are fixing this form.)

- Improve wording/spelling of some comments in .golangci.yml, and
  remove an outdated comment.

- Leave `package-comments` inactive, but add a TODO asking if we
  should change that.

- Add a new sub-linter `comment-spacings` (and fix corresponding
  comments), which avoids missing spaces after the leading `//`.

Signed-off-by: beorn7 <beorn@grafana.com>

.golangci.yml

@@ -25,15 +25,34 @@ linters:
     - loggercheck
 
 issues:
+  max-issues-per-linter: 0
   max-same-issues: 0
+  # The default exclusions are too aggressive. For one, they
+  # essentially disable any linting on doc comments. We disable
+  # default exclusions here and add exclusions fitting our codebase
+  # further down.
+  exclude-use-default: false
   exclude-files:
     # Skip autogenerated files.
     - ^.*\.(pb|y)\.go$
   exclude-dirs:
-    # Copied it from a different source
+    # Copied it from a different source.
     - storage/remote/otlptranslator/prometheusremotewrite
     - storage/remote/otlptranslator/prometheus
   exclude-rules:
+    - linters:
+        - errcheck
+      # Taken from the default exclusions (that are otherwise disabled above).
+      text: Error return value of .((os\.)?std(out|err)\..*|.*Close|.*Flush|os\.Remove(All)?|.*print(f|ln)?|os\.(Un)?Setenv). is not checked
+    - linters:
+        - govet
+      # We use many Seek methods that do not follow the usual pattern.
+      text: "stdmethods: method Seek.* should have signature Seek"
+    - linters:
+        - revive
+      # We have stopped at some point to write doc comments on exported symbols.
+      # TODO(beorn7): Maybe we should enforce this again? There are ~500 offenders right now.
+      text: exported (.+) should have comment( \(or a comment on this block\))? or be unexported
     - linters:
         - gocritic
       text: "appendAssign"
@@ -94,15 +113,14 @@ linters-settings:
     errorf: false
   revive:
     # By default, revive will enable only the linting rules that are named in the configuration file.
-    # So, it's needed to explicitly set in configuration all required rules.
+    # So, it's needed to explicitly enable all required rules here.
-    # The following configuration enables all the rules from the defaults.toml
-    # https://github.com/mgechev/revive/blob/master/defaults.toml
     rules:
       # https://github.com/mgechev/revive/blob/master/RULES_DESCRIPTIONS.md
       - name: blank-imports
+      - name: comment-spacings
       - name: context-as-argument
         arguments:
-          # allow functions with test or bench signatures
+          # Allow functions with test or bench signatures.
           - allowTypesBefore: "*testing.T,testing.TB"
       - name: context-keys-type
       - name: dot-imports
@@ -118,6 +136,8 @@ linters-settings:
       - name: increment-decrement
       - name: indent-error-flow
       - name: package-comments
+        # TODO(beorn7): Currently, we have a lot of missing package doc comments. Maybe we should have them.
+        disabled: true
       - name: range
       - name: receiver-naming
       - name: redefines-builtin-id

cmd/promtool/main.go

@@ -471,7 +471,7 @@ func (ls lintConfig) lintDuplicateRules() bool {
 	return ls.all || ls.duplicateRules
 }
 
-// Check server status - healthy & ready.
+// CheckServerStatus - healthy & ready.
 func CheckServerStatus(serverURL *url.URL, checkEndpoint string, roundTripper http.RoundTripper) error {
 	if serverURL.Scheme == "" {
 		serverURL.Scheme = "http"

cmd/promtool/metrics.go

@@ -31,7 +31,7 @@ import (
 	"github.com/prometheus/prometheus/util/fmtutil"
 )
 
-// Push metrics to a prometheus remote write (for testing purpose only).
+// PushMetrics to a prometheus remote write (for testing purpose only).
 func PushMetrics(url *url.URL, roundTripper http.RoundTripper, headers map[string]string, timeout time.Duration, labels map[string]string, files ...string) int {
 	addressURL, err := url.Parse(url.String())
 	if err != nil {

discovery/discoverer_metrics_noop.go

@@ -13,7 +13,7 @@
 
 package discovery
 
-// Create a dummy metrics struct, because this SD doesn't have any metrics.
+// NoopDiscovererMetrics creates a dummy metrics struct, because this SD doesn't have any metrics.
 type NoopDiscovererMetrics struct{}
 
 var _ DiscovererMetrics = (*NoopDiscovererMetrics)(nil)

discovery/discovery.go

@@ -39,7 +39,7 @@ type Discoverer interface {
 	Run(ctx context.Context, up chan<- []*targetgroup.Group)
 }
 
-// Internal metrics of service discovery mechanisms.
+// DiscovererMetrics are internal metrics of service discovery mechanisms.
 type DiscovererMetrics interface {
 	Register() error
 	Unregister()
@@ -56,7 +56,7 @@ type DiscovererOptions struct {
 	HTTPClientOptions []config.HTTPClientOption
 }
 
-// Metrics used by the "refresh" package.
+// RefreshMetrics are used by the "refresh" package.
 // We define them here in the "discovery" package in order to avoid a cyclic dependency between
 // "discovery" and "refresh".
 type RefreshMetrics struct {
@@ -64,17 +64,18 @@ type RefreshMetrics struct {
 	Duration prometheus.Observer
 }
 
-// Instantiate the metrics used by the "refresh" package.
+// RefreshMetricsInstantiator instantiates the metrics used by the "refresh" package.
 type RefreshMetricsInstantiator interface {
 	Instantiate(mech string) *RefreshMetrics
 }
 
-// An interface for registering, unregistering, and instantiating metrics for the "refresh" package.
+// RefreshMetricsManager is an interface for registering, unregistering, and
-// Refresh metrics are registered and unregistered outside of the service discovery mechanism.
+// instantiating metrics for the "refresh" package. Refresh metrics are
-// This is so that the same metrics can be reused across different service discovery mechanisms.
+// registered and unregistered outside of the service discovery mechanism. This
-// To manage refresh metrics inside the SD mechanism, we'd need to use const labels which are
+// is so that the same metrics can be reused across different service discovery
-// specific to that SD. However, doing so would also expose too many unused metrics on
+// mechanisms. To manage refresh metrics inside the SD mechanism, we'd need to
-// the Prometheus /metrics endpoint.
+// use const labels which are specific to that SD. However, doing so would also
+// expose too many unused metrics on the Prometheus /metrics endpoint.
 type RefreshMetricsManager interface {
 	DiscovererMetrics
 	RefreshMetricsInstantiator
@@ -145,7 +146,8 @@ func (c StaticConfig) NewDiscoverer(DiscovererOptions) (Discoverer, error) {
 	return staticDiscoverer(c), nil
 }
 
-// No metrics are needed for this service discovery mechanism.
+// NewDiscovererMetrics returns NoopDiscovererMetrics because no metrics are
+// needed for this service discovery mechanism.
 func (c StaticConfig) NewDiscovererMetrics(prometheus.Registerer, RefreshMetricsInstantiator) DiscovererMetrics {
 	return &NoopDiscovererMetrics{}
 }

discovery/manager.go

@@ -64,7 +64,7 @@ func (p *Provider) Config() interface{} {
 	return p.config
 }
 
-// Registers the metrics needed for SD mechanisms.
+// CreateAndRegisterSDMetrics registers the metrics needed for SD mechanisms.
 // Does not register the metrics for the Discovery Manager.
 // TODO(ptodev): Add ability to unregister the metrics?
 func CreateAndRegisterSDMetrics(reg prometheus.Registerer) (map[string]DiscovererMetrics, error) {

discovery/metrics_refresh.go

@@ -17,7 +17,7 @@ import (
 	"github.com/prometheus/client_golang/prometheus"
 )
 
-// Metric vectors for the "refresh" package.
+// RefreshMetricsVecs are metric vectors for the "refresh" package.
 // We define them here in the "discovery" package in order to avoid a cyclic dependency between
 // "discovery" and "refresh".
 type RefreshMetricsVecs struct {

discovery/util.go

@@ -19,8 +19,8 @@ import (
 	"github.com/prometheus/client_golang/prometheus"
 )
 
-// A utility to be used by implementations of discovery.Discoverer
+// MetricRegisterer is used by implementations of discovery.Discoverer that need
-// which need to manage the lifetime of their metrics.
+// to manage the lifetime of their metrics.
 type MetricRegisterer interface {
 	RegisterMetrics() error
 	UnregisterMetrics()
@@ -34,7 +34,7 @@ type metricRegistererImpl struct {
 
 var _ MetricRegisterer = &metricRegistererImpl{}
 
-// Creates an instance of a MetricRegisterer.
+// NewMetricRegisterer creates an instance of a MetricRegisterer.
 // Typically called inside the implementation of the NewDiscoverer() method.
 func NewMetricRegisterer(reg prometheus.Registerer, metrics []prometheus.Collector) MetricRegisterer {
 	return &metricRegistererImpl{

model/exemplar/exemplar.go

@@ -15,7 +15,9 @@ package exemplar
 
 import "github.com/prometheus/prometheus/model/labels"
 
-// The combined length of the label names and values of an Exemplar's LabelSet MUST NOT exceed 128 UTF-8 characters
+// ExemplarMaxLabelSetLength is defined by OpenMetrics: "The combined length of
+// the label names and values of an Exemplar's LabelSet MUST NOT exceed 128
+// UTF-8 characters."
 // https://github.com/OpenObservability/OpenMetrics/blob/main/specification/OpenMetrics.md#exemplars
 const ExemplarMaxLabelSetLength = 128
 
@@ -49,7 +51,7 @@ func (e Exemplar) Equals(e2 Exemplar) bool {
 	return e.Value == e2.Value
 }
 
-// Sort first by timestamp, then value, then labels.
+// Compare first timestamps, then values, then labels.
 func Compare(a, b Exemplar) int {
 	if a.Ts < b.Ts {
 		return -1

model/labels/labels.go

@@ -315,7 +315,8 @@ func Compare(a, b Labels) int {
 	return len(a) - len(b)
 }
 
-// Copy labels from b on top of whatever was in ls previously, reusing memory or expanding if needed.
+// CopyFrom copies labels from b on top of whatever was in ls previously,
+// reusing memory or expanding if needed.
 func (ls *Labels) CopyFrom(b Labels) {
 	(*ls) = append((*ls)[:0], b...)
 }
@@ -422,7 +423,7 @@ type ScratchBuilder struct {
 	add Labels
 }
 
-// Symbol-table is no-op, just for api parity with dedupelabels.
+// SymbolTable is no-op, just for api parity with dedupelabels.
 type SymbolTable struct{}
 
 func NewSymbolTable() *SymbolTable { return nil }
@@ -458,7 +459,7 @@ func (b *ScratchBuilder) Add(name, value string) {
 	b.add = append(b.add, Label{Name: name, Value: value})
 }
 
-// Add a name/value pair, using []byte instead of string.
+// UnsafeAddBytes adds a name/value pair, using []byte instead of string.
 // The '-tags stringlabels' version of this function is unsafe, hence the name.
 // This version is safe - it copies the strings immediately - but we keep the same name so everything compiles.
 func (b *ScratchBuilder) UnsafeAddBytes(name, value []byte) {
@@ -475,14 +476,14 @@ func (b *ScratchBuilder) Assign(ls Labels) {
 	b.add = append(b.add[:0], ls...) // Copy on top of our slice, so we don't retain the input slice.
 }
 
-// Return the name/value pairs added so far as a Labels object.
+// Labels returns the name/value pairs added so far as a Labels object.
 // Note: if you want them sorted, call Sort() first.
 func (b *ScratchBuilder) Labels() Labels {
 	// Copy the slice, so the next use of ScratchBuilder doesn't overwrite.
 	return append([]Label{}, b.add...)
 }
 
-// Write the newly-built Labels out to ls.
+// Overwrite the newly-built Labels out to ls.
 // Callers must ensure that there are no other references to ls, or any strings fetched from it.
 func (b *ScratchBuilder) Overwrite(ls *Labels) {
 	*ls = append((*ls)[:0], b.add...)

model/textparse/interface.go

@@ -106,8 +106,8 @@ const (
 	EntryInvalid   Entry = -1
 	EntryType      Entry = 0
 	EntryHelp      Entry = 1
-	EntrySeries    Entry = 2 // A series with a simple float64 as value.
+	EntrySeries    Entry = 2 // EntrySeries marks a series with a simple float64 as value.
 	EntryComment   Entry = 3
 	EntryUnit      Entry = 4
-	EntryHistogram Entry = 5 // A series with a native histogram as a value.
+	EntryHistogram Entry = 5 // EntryHistogram marks a series with a native histogram as a value.
 )

promql/engine.go

@@ -573,7 +573,7 @@ func (ng *Engine) validateOpts(expr parser.Expr) error {
 	return validationErr
 }
 
-// NewTestQuery: inject special behaviour into Query for testing.
+// NewTestQuery injects special behaviour into Query for testing.
 func (ng *Engine) NewTestQuery(f func(context.Context) error) Query {
 	qry := &query{
 		q:           "test statement",
@@ -3531,14 +3531,14 @@ func makeInt64Pointer(val int64) *int64 {
 	return valp
 }
 
-// Add RatioSampler interface to allow unit-testing (previously: Randomizer).
+// RatioSampler allows unit-testing (previously: Randomizer).
 type RatioSampler interface {
 	// Return this sample "offset" between [0.0, 1.0]
 	sampleOffset(ts int64, sample *Sample) float64
 	AddRatioSample(r float64, sample *Sample) bool
 }
 
-// Use Hash(labels.String()) / maxUint64 as a "deterministic"
+// HashRatioSampler uses Hash(labels.String()) / maxUint64 as a "deterministic"
 // value in [0.0, 1.0].
 type HashRatioSampler struct{}
 

promql/parser/ast.go

@@ -352,8 +352,7 @@ func (f inspector) Visit(node Node, path []Node) (Visitor, error) {
 // f(node, path); node must not be nil. If f returns a nil error, Inspect invokes f
 // for all the non-nil children of node, recursively.
 func Inspect(node Node, f inspector) {
-	//nolint: errcheck
+	Walk(f, node, nil) //nolint:errcheck
-	Walk(f, node, nil)
 }
 
 // Children returns a list of all child nodes of a syntax tree node.
@@ -419,7 +418,7 @@ func mergeRanges(first, last Node) posrange.PositionRange {
 	}
 }
 
-// Item implements the Node interface.
+// PositionRange implements the Node interface.
 // This makes it possible to call mergeRanges on them.
 func (i *Item) PositionRange() posrange.PositionRange {
 	return posrange.PositionRange{

scrape/clientprotobuf.go

@@ -23,7 +23,7 @@ import (
 	dto "github.com/prometheus/client_model/go"
 )
 
-// Write a MetricFamily into a protobuf.
+// MetricFamilyToProtobuf writes a MetricFamily into a protobuf.
 // This function is intended for testing scraping by providing protobuf serialized input.
 func MetricFamilyToProtobuf(metricFamily *dto.MetricFamily) ([]byte, error) {
 	buffer := &bytes.Buffer{}
@@ -34,7 +34,7 @@ func MetricFamilyToProtobuf(metricFamily *dto.MetricFamily) ([]byte, error) {
 	return buffer.Bytes(), nil
 }
 
-// Append a MetricFamily protobuf representation to a buffer.
+// AddMetricFamilyToProtobuf appends a MetricFamily protobuf representation to a buffer.
 // This function is intended for testing scraping by providing protobuf serialized input.
 func AddMetricFamilyToProtobuf(buffer *bytes.Buffer, metricFamily *dto.MetricFamily) error {
 	protoBuf, err := proto.Marshal(metricFamily)

storage/interface.go

@@ -227,9 +227,9 @@ type LabelHints struct {
 	Limit int
 }
 
-// TODO(bwplotka): Move to promql/engine_test.go?
 // QueryableFunc is an adapter to allow the use of ordinary functions as
 // Queryables. It follows the idea of http.HandlerFunc.
+// TODO(bwplotka): Move to promql/engine_test.go?
 type QueryableFunc func(mint, maxt int64) (Querier, error)
 
 // Querier calls f() with the given parameters.

storage/remote/azuread/azuread.go

@@ -31,13 +31,15 @@ import (
 	"github.com/google/uuid"
 )
 
-const (
 // Clouds.
+const (
 	AzureChina      = "AzureChina"
 	AzureGovernment = "AzureGovernment"
 	AzurePublic     = "AzurePublic"
+)
 
 // Audiences.
+const (
 	IngestionChinaAudience      = "https://monitor.azure.cn//.default"
 	IngestionGovernmentAudience = "https://monitor.azure.us//.default"
 	IngestionPublicAudience     = "https://monitor.azure.com//.default"

template/template.go

@@ -166,7 +166,7 @@ func NewTemplateExpander(
 				return html_template.HTML(text)
 			},
 			"match":     regexp.MatchString,
-			"title":     strings.Title, //nolint:staticcheck
+			"title":     strings.Title, //nolint:staticcheck // TODO(beorn7): Need to come up with a replacement using the cases package.
 			"toUpper":   strings.ToUpper,
 			"toLower":   strings.ToLower,
 			"graphLink": strutil.GraphLinkForExpression,

tsdb/chunks/head_chunks.go

@@ -191,7 +191,7 @@ func (f *chunkPos) bytesToWriteForChunk(chkLen uint64) uint64 {
 // ChunkDiskMapper is for writing the Head block chunks to disk
 // and access chunks via mmapped files.
 type ChunkDiskMapper struct {
-	/// Writer.
+	// Writer.
 	dir             *os.File
 	writeBufferSize int
 
@@ -210,7 +210,7 @@ type ChunkDiskMapper struct {
 	crc32        hash.Hash
 	writePathMtx sync.Mutex
 
-	/// Reader.
+	// Reader.
 	// The int key in the map is the file number on the disk.
 	mmappedChunkFiles map[int]*mmappedChunkFile // Contains the m-mapped files for each chunk file mapped with its index.
 	closers           map[int]io.Closer         // Closers for resources behind the byte slices.

tsdb/db.go

@@ -49,7 +49,7 @@ import (
 )
 
 const (
-	// Default duration of a block in milliseconds.
+	// DefaultBlockDuration in milliseconds.
 	DefaultBlockDuration = int64(2 * time.Hour / time.Millisecond)
 
 	// Block dir suffixes to make deletion and creation operations atomic.

tsdb/encoding/encoding.go

@@ -201,8 +201,8 @@ func (d *Decbuf) UvarintStr() string {
 	return string(d.UvarintBytes())
 }
 
-// The return value becomes invalid if the byte slice goes away.
+// UvarintBytes returns invalid values if the byte slice goes away.
-// Compared to UvarintStr, this avoid allocations.
+// Compared to UvarintStr, it avoid allocations.
 func (d *Decbuf) UvarintBytes() []byte {
 	l := d.Uvarint64()
 	if d.E != nil {

tsdb/head_other.go

@@ -26,7 +26,7 @@ func (s *memSeries) labels() labels.Labels {
 	return s.lset
 }
 
-// No-op when not using dedupelabels.
+// RebuildSymbolTable is a no-op when not using dedupelabels.
 func (h *Head) RebuildSymbolTable(logger log.Logger) *labels.SymbolTable {
 	return nil
 }

tsdb/index/index.go

@@ -196,8 +196,9 @@ func NewTOCFromByteSlice(bs ByteSlice) (*TOC, error) {
 	return toc, d.Err()
 }
 
-// NewWriter returns a new Writer to the given filename. It serializes data in format version 2.
+// NewWriterWithEncoder returns a new Writer to the given filename. It
-// It uses the given encoder to encode each postings list.
+// serializes data in format version 2. It uses the given encoder to encode each
+// postings list.
 func NewWriterWithEncoder(ctx context.Context, fn string, encoder PostingsEncoder) (*Writer, error) {
 	dir := filepath.Dir(fn)
 

tsdb/ooo_head_read.go

@@ -263,8 +263,8 @@ func (cr *HeadAndOOOChunkReader) ChunkOrIterable(meta chunks.Meta) (chunkenc.Chu
 	return nil, mc, err
 }
 
-// ChunkOrIterableWithCopy: implements ChunkReaderWithCopy. The special Copy behaviour
+// ChunkOrIterableWithCopy implements ChunkReaderWithCopy. The special Copy
-// is only implemented for the in-order head chunk.
+// behaviour is only implemented for the in-order head chunk.
 func (cr *HeadAndOOOChunkReader) ChunkOrIterableWithCopy(meta chunks.Meta) (chunkenc.Chunk, chunkenc.Iterable, int64, error) {
 	_, _, isOOO := unpackHeadChunkRef(meta.Ref)
 	if !isOOO {

tsdb/wlog/watcher.go

@@ -58,15 +58,16 @@ type WriteTo interface {
 	StoreSeries([]record.RefSeries, int)
 	StoreMetadata([]record.RefMetadata)
 
-	// Next two methods are intended for garbage-collection: first we call
+	// UpdateSeriesSegment and SeriesReset are intended for
-	// UpdateSeriesSegment on all current series
+	// garbage-collection:
+	// First we call UpdateSeriesSegment on all current series.
 	UpdateSeriesSegment([]record.RefSeries, int)
-	// Then SeriesReset is called to allow the deletion
+	// Then SeriesReset is called to allow the deletion of all series
-	// of all series created in a segment lower than the argument.
+	// created in a segment lower than the argument.
 	SeriesReset(int)
 }
 
-// Used to notify the watcher that data has been written so that it can read.
+// WriteNotified notifies the watcher that data has been written so that it can read.
 type WriteNotified interface {
 	Notify()
 }

tsdb/wlog/wlog.go

@@ -38,8 +38,8 @@ import (
 )
 
 const (
-	DefaultSegmentSize = 128 * 1024 * 1024 // 128 MB
+	DefaultSegmentSize = 128 * 1024 * 1024 // DefaultSegmentSize is 128 MB.
-	pageSize           = 32 * 1024         // 32KB
+	pageSize           = 32 * 1024         // pageSize is 32KB.
 	recordHeaderSize   = 7
 	WblDirName         = "wbl"
 )

util/annotations/annotations.go

@@ -174,7 +174,7 @@ func NewInvalidQuantileWarning(q float64, pos posrange.PositionRange) error {
 	}
 }
 
-// NewInvalidQuantileWarning is used when the user specifies an invalid ratio
+// NewInvalidRatioWarning is used when the user specifies an invalid ratio
 // value, i.e. a float that is outside the range [-1, 1] or NaN.
 func NewInvalidRatioWarning(q, to float64, pos posrange.PositionRange) error {
 	return annoErr{

util/testutil/cmp.go

@@ -23,13 +23,14 @@ import (
 	"github.com/prometheus/prometheus/model/labels"
 )
 
-// Replacement for require.Equal using go-cmp adapted for Prometheus data structures, instead of DeepEqual.
+// RequireEqual is a replacement for require.Equal using go-cmp adapted for
+// Prometheus data structures, instead of DeepEqual.
 func RequireEqual(t testing.TB, expected, actual interface{}, msgAndArgs ...interface{}) {
 	t.Helper()
 	RequireEqualWithOptions(t, expected, actual, nil, msgAndArgs...)
 }
 
-// As RequireEqual but allows extra cmp.Options.
+// RequireEqualWithOptions works like RequireEqual but allows extra cmp.Options.
 func RequireEqualWithOptions(t testing.TB, expected, actual interface{}, extra []cmp.Option, msgAndArgs ...interface{}) {
 	t.Helper()
 	options := append([]cmp.Option{cmp.Comparer(labels.Equal)}, extra...)