Fix codestyle issues (#104)

* Configure linter * Update CONTRIBUTING.md * Lowercase for log message * Rename stats -> metrics * Update CI workflow * Enable imports linter
ozontech · Aug 8, 2022 · acc6276 · acc6276
1 parent 707fc82
commit acc6276
Show file tree

Hide file tree

Showing 31 changed files with 333 additions and 182 deletions.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -0,0 +1,110 @@
+name: CI
+
+on:
+  push:
+    branches: [ master ]
+  pull_request:
+    branches: [ master ]
+
+jobs:
+  test:
+    runs-on: ${{ matrix.runner }}
+    strategy:
+      fail-fast: false
+      matrix:
+        go:
+          - 1.18
+        arch:
+          - amd64
+        runner:
+          - ubuntu-latest
+    steps:
+      - name: Checkout code
+        uses: actions/checkout@v3
+
+      - name: Install Go
+        uses: actions/setup-go@v3
+        with:
+          go-version: ${{ matrix.go }}
+
+      - name: Get Go environment
+        id: go-env
+        run: |
+          echo "::set-output name=cache::$(go env GOCACHE)"
+          echo "::set-output name=modcache::$(go env GOMODCACHE)"
+      - name: Set up cache
+        uses: actions/cache@v3
+        with:
+          path: |
+            ${{ steps.go-env.outputs.cache }}
+            ${{ steps.go-env.outputs.modcache }}
+          key: test-${{ runner.os }}-go-${{ hashFiles('**/go.sum') }}
+          restore-keys: |
+            test-${{ runner.os }}-go-
+
+      - name: Build
+        env:
+          GOARCH: ${{ matrix.arch }}
+          GOFLAGS: ${{ matrix.flags }}
+        run: go build ./...
+
+      - name: Test
+        env:
+          GOARCH: ${{ matrix.arch }}
+          GOFLAGS: ${{ matrix.flags }}
+          LOG_LEVEL: error
+        run: go test -v ./...
+
+  lint:
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout code
+        uses: actions/checkout@v3
+
+      - name: Install Go
+        uses: actions/setup-go@v3
+        with:
+          go-version: 1.18
+
+      - name: Lint
+        uses: golangci/golangci-lint-action@v3
+        with:
+          version: v1.45.2
+          only-new-issues: true
+          args: --timeout 5m
+
+  # Check if there are any dirty changes after go mod tidy & make gen-doc
+  dirty-changes:
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout code
+        uses: actions/checkout@v3
+
+      - name: Install Go
+        uses: actions/setup-go@v3
+        with:
+          go-version: 1.18
+
+      - name: Get Go environment
+        id: go-env
+        run: |
+          echo "::set-output name=cache::$(go env GOCACHE)"
+          echo "::set-output name=modcache::$(go env GOMODCACHE)"
+      - name: Set up cache
+        uses: actions/cache@v3
+        with:
+          path: |
+            ${{ steps.go-env.outputs.cache }}
+            ${{ steps.go-env.outputs.modcache }}
+          key: dirty-changes-${{ runner.os }}-go-${{ hashFiles('**/go.sum') }}
+          restore-keys: |
+            dirty-changes-${{ runner.os }}-go-
+
+      - name: Download dependencies
+        run: go mod download && go mod tidy
+
+      - name: Generate doc
+        run: make gen-doc
+
+      - name: Check git diff
+        run: git diff --exit-code
diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml
diff --git a/.golangci.yml b/.golangci.yml
@@ -0,0 +1,84 @@
+linters-settings:
+  govet:
+    enable:
+      - composites
+  dupl:
+    threshold: 120
+  goconst:
+    min-len: 2
+    min-occurrences: 3
+  misspell:
+    locale: US
+  gocritic:
+    enabled-tags:
+      - diagnostic
+      - experimental
+      - opinionated
+      - performance
+      - style
+    disabled-checks:
+      - whyNoLint
+      - commentFormatting # insane-doc syntax requires "//>" format
+      - paramTypeCombine
+      - ptrToRefParam # we use interface pointer in many places
+      - unnamedResult
+      - sprintfQuotedString
+      - tooManyResultsChecker
+  gosec:
+    excludes:
+      - G304 # Potential file inclusion via variable -- it's ok for this project
+  stylecheck:
+    checks:
+      - '-ST1021' # insane-doc syntax requires "//>" format
+
+linters:
+  disable-all: true
+  enable:
+    - deadcode
+    - depguard
+    - dogsled
+    - dupl
+    - goconst
+    - gocritic
+    - goimports
+    - gosimple
+    - govet
+    - ineffassign
+    - misspell
+    - nakedret
+    - prealloc
+    - structcheck
+    - stylecheck
+    - typecheck
+    - unconvert
+    - unparam
+    - unused
+    - varcheck
+    - whitespace
+    # Do not enable:
+    # - staticcheck (does not work with golangci-lint 1.46.2 and go 1.18.2)
+    # - gosec (not worth it in scope of this project)
+    # - gochecknoglobals (we know when it is ok to use globals)
+    # - gochecknoinits (we know when it is ok to use inits)
+    # - errcheck
+
+issues:
+  exclude-use-default: false
+  exclude-rules:
+    # Disable linters that are annoying in tests.
+    - path: _test\.go
+      linters:
+        - gocyclo
+        - errcheck
+        - dupl
+        - gosec
+        - goconst
+
+    # Ignore shadowing of err.
+    - linters: [ govet ]
+      text: 'declaration of "(err|ctx)"'
+
+run:
+  build-tags:
+    - e2e
+    - fuzz
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
@@ -55,6 +55,12 @@ git push origin <topic-branch-name>
 ```
 7. Open a pull request with a clear title and description.
 
+## Code style
+
+* Log in lower case without a dot in the end of a sentence
+* Log params by pattern: `name=value`, e.g. `logger.Errorf("can't parse url=%s", url)`
+* Code comments should be written with a small letter without a dot at the end
+* Split imports into 2 groups: stdlib and other packages 
 
 ## Collaborating guidelines
 

diff --git a/fd/file.d.go b/fd/file.d.go
@@ -10,14 +10,13 @@ import (
 	"runtime/debug"
 
 	"github.com/bitly/go-simplejson"
-	"github.com/prometheus/client_golang/prometheus"
-	"github.com/prometheus/client_golang/prometheus/promhttp"
-
 	"github.com/ozontech/file.d/cfg"
 	"github.com/ozontech/file.d/logger"
 	"github.com/ozontech/file.d/longpanic"
+	"github.com/ozontech/file.d/metric"
 	"github.com/ozontech/file.d/pipeline"
-	"github.com/ozontech/file.d/stats"
+	"github.com/prometheus/client_golang/prometheus"
+	"github.com/prometheus/client_golang/prometheus/promhttp"
 )
 
 const (
@@ -57,15 +56,15 @@ func (f *FileD) Start() {
 }
 
 func (f *FileD) initMetrics() {
-	stats.InitStats()
+	metric.InitStats()
 
-	stats.RegisterCounter(&stats.MetricDesc{
+	metric.RegisterCounter(&metric.MetricDesc{
 		Subsystem: subsystemLongPanicName,
 		Name:      panics,
 		Help:      "Count of panics in the LongPanic",
 	})
 	longpanic.SetOnPanicHandler(func(_ error) {
-		stats.GetCounter(subsystemLongPanicName, panics).Inc()
+		metric.GetCounter(subsystemLongPanicName, panics).Inc()
 	})
 }
 

diff --git a/stats/stats.go → metric/metrics.go b/stats/stats.go → metric/metrics.go
@@ -1,9 +1,9 @@
-package stats
+// Package metric contains utilities for creating and getting file.d metric.
+package metric
 
 import (
 	"github.com/ozontech/file.d/buildinfo"
 	"github.com/ozontech/file.d/logger"
-
 	prom "github.com/prometheus/client_golang/prometheus"
 )
 
@@ -45,7 +45,7 @@ var statsGlobal *stats
 
 func InitStats() {
 	if statsGlobal != nil {
-		logger.Error("attempt to initialize stats more than once")
+		logger.Error("attempt to initialize metric more than once")
 		return
 	}
 
@@ -162,6 +162,6 @@ func getKey(subsystem, metricName string) key {
 
 func checkStatsInitialized() {
 	if statsGlobal == nil {
-		logger.Panicf("stats package uninitialized")
+		logger.Panicf("metric package uninitialized")
 	}
 }
diff --git a/pipeline/antispamer.go b/pipeline/antispamer.go
@@ -5,7 +5,7 @@ import (
 	"time"
 
 	"github.com/ozontech/file.d/logger"
-	"github.com/ozontech/file.d/stats"
+	"github.com/ozontech/file.d/metric"
 	"go.uber.org/atomic"
 )
 
@@ -27,15 +27,15 @@ func newAntispamer(threshold int, unbanIterations int, maintenanceInterval time.
 		logger.Infof("antispam enabled, threshold=%d/%d sec", threshold, maintenanceInterval/time.Second)
 	}
 
-	stats.RegisterGauge(&stats.MetricDesc{
+	metric.RegisterGauge(&metric.MetricDesc{
 		Name:      antispamActive,
 		Subsystem: subsystemName,
 		Help:      "Gauge indicates whether the antispam is enabled",
 	})
 	// not enabled by default
-	stats.GetGauge(subsystemName, antispamActive).Set(0)
+	metric.GetGauge(subsystemName, antispamActive).Set(0)
 
-	stats.RegisterCounter(&stats.MetricDesc{
+	metric.RegisterCounter(&metric.MetricDesc{
 		Name:      antispamBanCount,
 		Subsystem: subsystemName,
 		Help:      "How many times a source was banned",
@@ -77,8 +77,8 @@ func (p *antispamer) isSpam(id SourceID, name string, isNewSource bool) bool {
 	x := value.Inc()
 	if x == int32(p.threshold) {
 		value.Swap(int32(p.unbanIterations * p.threshold))
-		stats.GetGauge(subsystemName, antispamActive).Set(1)
-		stats.GetCounter(subsystemName, antispamBanCount).Inc()
+		metric.GetGauge(subsystemName, antispamActive).Set(1)
+		metric.GetCounter(subsystemName, antispamBanCount).Inc()
 		logger.Warnf("antispam: source has been banned id=%d, name=%s", id, name)
 	}
 
@@ -119,7 +119,7 @@ func (p *antispamer) maintenance() {
 	}
 
 	if allUnbanned {
-		stats.GetGauge(subsystemName, antispamActive).Set(0)
+		metric.GetGauge(subsystemName, antispamActive).Set(0)
 	} else {
 		logger.Info("antispam: there are banned sources")
 	}

diff --git a/pipeline/delta_wrapper.go b/pipeline/delta_wrapper.go
@@ -9,7 +9,7 @@ import (
 // DeltaWrapper acts as a wrapper around int64
 // and returns the difference between
 // the new and the old value when a new value is set.
-// This is useful for metrics (see the maintenance function).
+// This is useful for metric (see the maintenance function).
 type DeltaWrapper struct {
 	val int64
 }

diff --git a/pipeline/metrics_holder.go b/pipeline/metrics_holder.go
@@ -2,11 +2,11 @@ package pipeline
 
 import (
 	"fmt"
-	"github.com/ozontech/file.d/buildinfo"
 	"strconv"
 	"sync"
 	"time"
 
+	"github.com/ozontech/file.d/buildinfo"
 	"github.com/prometheus/client_golang/prometheus"
 	"go.uber.org/atomic"
 )