Merge branch 'main' into deprecate-spanmetrics-processor

.codecov.yml

@@ -14,6 +14,7 @@ ignore:
   - "examples/hotrod"
   - "plugin/storage/integration"
   - "cmd/jaeger/internal/integration"
+  - "internal/tools"
 
 coverage:
   precision: 2

.github/dependabot.yml

@@ -19,6 +19,12 @@ updates:
         patterns:
           - "go.opentelemetry.io/contrib/instrumentation/*"
 
+  - package-ecosystem: gomod
+    directory: "/internal/tools"
+    schedule:
+      interval: daily
+    labels: [ "changelog:dependencies" ]
+
   - package-ecosystem: "github-actions"
     directory: "/"
     schedule:
@@ -129,4 +135,11 @@ updates:
       interval: daily
     allow:
       - dependency-name: "bitnami/kafka"
-        update-types: ["version-update:semver-minor"]
+        update-types: ["version-update:semver-minor"]
+  - package-ecosystem: docker
+    directory: /docker-compose/cassandra
+    schedule:
+      interval: daily
+    allow:
+      - dependency-name: "cassandra"
+        update-types: ["version-update:semver-minor"]

.github/workflows/ci-cassandra.yml

@@ -24,11 +24,9 @@ jobs:
         version:
         - distribution: cassandra
           major: 3.x
-          image: 3.11
           schema: v003
         - distribution: cassandra
           major: 4.x
-          image: 4.0
           schema: v004
     name: ${{ matrix.version.distribution }} ${{ matrix.version.major }} ${{ matrix.jaeger-version }}
     steps:
@@ -45,10 +43,10 @@ jobs:
 
     - name: Run cassandra integration tests
       id: test-execution
-      run: bash scripts/cassandra-integration-test.sh ${{ matrix.version.image }} ${{ matrix.version.schema }} ${{ matrix.jaeger-version }}
+      run: bash scripts/cassandra-integration-test.sh ${{ matrix.version.major }} ${{ matrix.version.schema }} ${{ matrix.jaeger-version }}
 
     - name: Output Cassandra logs
-      run: docker logs ${{ steps.test-execution.outputs.cid }}
+      run: docker compose -f ${{ steps.test-execution.outputs.docker_compose_file }} logs
       if: ${{ failure() }}
 
     - name: Upload coverage to codecov

.github/workflows/ci-lint-checks.yaml

@@ -41,3 +41,13 @@ jobs:
 
     - name: Ensure PR is not on main branch
       uses: ./.github/actions/block-pr-not-on-main
+
+    - name: Set up Python 3.x for DCO check
+      uses: actions/setup-python@v2
+      with:
+        python-version: '3.x'
+
+    - name: Run DCO check
+      run: python3 scripts/dco_check.py -b main -v
+      env:
+          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}

.gitignore

@@ -1,6 +1,8 @@
 go.work
 go.work.sum
 
+.tools/
+
 *.out
 *.test
 *.xml
@@ -15,6 +17,8 @@ cover.html
 .tmp/
 .mkdocs-virtual-env/
 vendor/
+
+# Jaeger binaries
 examples/hotrod/hotrod
 examples/hotrod/hotrod-*
 cmd/all-in-one/all-in-one-*
@@ -36,6 +40,7 @@ cmd/query/query-*
 cmd/tracegen/tracegen
 cmd/tracegen/tracegen-*
 crossdock/crossdock-*
+
 run-crossdock.log
 proto-gen/.patched-otel-proto/
 __pycache__

.golangci.yml

@@ -171,9 +171,6 @@ linters-settings:
         # do a clean-up and enable
         - name: unused-parameter
           disabled: true
-        # do a clean-up and enable
-        - name: unused-receiver
-          disabled: true
         # we use storage_v2, so...
         - name: var-naming
           disabled: true
@@ -189,9 +186,6 @@ linters-settings:
         # TBD - often triggered in tests
         - name: unhandled-error
           disabled: true
-        # this one looks like it's catching real errors, need to enable it
-        - name: modifies-value-receiver
-          disabled: true
         # often looks like a red herring, needs investigation
         - name: flag-parameter
           disabled: true
@@ -210,48 +204,18 @@ linters-settings:
         # maybe enable, needs invesitgation of the impact
         - name: get-return
           disabled: true
-        # enable after cleanup
-        - name: early-return
-          disabled: true
-        # enable after cleanup
-        - name: bare-return
-          disabled: true
-        # enable after cleanup
-        - name: empty-lines
-          disabled: true
         # investigate, could be real bugs. But didn't recent Go version changed loop variables semantics?
         - name: range-val-address
           disabled: true
-        # enable after cleanup
-        - name: confusing-results
-          disabled: true
-        # we do use dot imports, but not a bad idea to make it explicit
-        - name: dot-imports
-          disabled: true
-        # enable after cleanup: "tag on not-exported field"
-        - name: struct-tag
-          disabled: true
-        # enable after cleanup
-        - name: receiver-naming
-          disabled: true
         # this is idiocy, promotes less readable code. Don't enable.
         - name: var-declaration
           disabled: true
-        # enable after cleanup: warns of stutter names
-        - name: exported
-          disabled: true
         # enable after cleanup
         - name: redefines-builtin-id
           disabled: true
         # "no nested structs are allowed" - don't enable, doesn't make sense
         - name: nested-structs
           disabled: true
-        # enable after cleanup
-        - name: indent-error-flow
-          disabled: true
-        # enable after cleanup
-        - name: unexported-return
-          disabled: true
         # looks useful, but requires refactoring: "calls to log.Fatal only in main() or init() functions"
         - name: deep-exit
           disabled: true

Makefile

@@ -10,6 +10,9 @@ JAEGER_V2_STORAGE_PKGS = ./cmd/jaeger/internal/integration
 DOCKER_NAMESPACE?=jaegertracing
 DOCKER_TAG?=latest
 
+# SRC_ROOT is the top of the source tree.
+SRC_ROOT := $(shell git rev-parse --show-toplevel)
+
 # TODO we can compartmentalize this Makefile better, by separting:
 #  - integration tests
 #  - all the binary building targets
@@ -32,6 +35,7 @@ ALL_SRC = $(shell find . -name '*.go' \
 				   -not -name 'mocks*' \
 				   -not -name '*.pb.go' \
 				   -not -path './vendor/*' \
+				   -not -path './internal/tools/*' \
 				   -not -path '*/mocks/*' \
 				   -not -path '*/*-gen/*' \
 				   -not -path '*/thrift-0.9.2/*' \
@@ -59,7 +63,6 @@ GOTEST_QUIET=$(GO) test $(RACE)
 GOTEST=$(GOTEST_QUIET) -v
 COVEROUT=cover.out
 GOFMT=gofmt
-GOFUMPT=gofumpt
 FMT_LOG=.fmt.log
 IMPORT_LOG=.import.log
 COLORIZE ?= | $(SED) 's/PASS/✅ PASS/g' | $(SED) 's/FAIL/❌ FAIL/g' | $(SED) 's/SKIP/🔕 SKIP/g'
@@ -78,11 +81,10 @@ DATE=$(shell TZ=UTC0 git show --quiet --date='format-local:%Y-%m-%dT%H:%M:%SZ' -
 BUILD_INFO_IMPORT_PATH=$(JAEGER_IMPORT_PATH)/pkg/version
 BUILD_INFO=-ldflags "-X $(BUILD_INFO_IMPORT_PATH).commitSHA=$(GIT_SHA) -X $(BUILD_INFO_IMPORT_PATH).latestVersion=$(GIT_CLOSEST_TAG) -X $(BUILD_INFO_IMPORT_PATH).date=$(DATE)"
 
-MOCKERY=mockery
-GOVERSIONINFO=goversioninfo
 SYSOFILE=resource.syso
 
 # import other Makefiles after the variables are defined
+include Makefile.Tools.mk
 include docker/Makefile
 include Makefile.Protobuf.mk
 include Makefile.Thrift.mk
@@ -187,7 +189,7 @@ fmt:
 
 .PHONY: lint
 lint: goleak
-	golangci-lint -v run
+	$(LINT) -v run
 	@./scripts/updateLicense.py $(ALL_SRC) > $(FMT_LOG)
 	@./scripts/import-order-cleanup.py -o stdout -t $(ALL_SRC) > $(IMPORT_LOG)
 	@[ ! -s "$(FMT_LOG)" -a ! -s "$(IMPORT_LOG)" ] || (echo "License check or import ordering failures, run 'make fmt'" | cat - $(FMT_LOG) $(IMPORT_LOG) && false)
@@ -452,22 +454,6 @@ changelog:
 draft-release:
 	./scripts/draft-release.py
 
-.PHONY: install-test-tools
-install-test-tools:
-	$(GO) install github.com/golangci/golangci-lint/cmd/golangci-lint@v1.55.2
-	$(GO) install mvdan.cc/gofumpt@latest
-
-.PHONY: install-build-tools
-install-build-tools:
-	$(GO) install github.com/josephspurrier/goversioninfo/cmd/goversioninfo@v1.4.0
-
-.PHONY: install-tools
-install-tools: install-test-tools install-build-tools
-	$(GO) install github.com/vektra/mockery/v2@v2.42.3
-
-.PHONY: install-ci
-install-ci: install-test-tools install-build-tools
-
 .PHONY: test-ci
 test-ci: GOTEST := $(GOTEST_QUIET)
 test-ci: build-examples cover
@@ -477,8 +463,8 @@ init-submodules:
 	git submodule update --init --recursive
 
 .PHONY: generate-mocks
-generate-mocks: install-tools
-	$(MOCKERY) --all --dir ./pkg/es/ --output ./pkg/es/mocks && rm pkg/es/mocks/ClientBuilder.go
+generate-mocks: $(MOCKERY)
+	$(MOCKERY) --all --dir ./pkg/es/ --output ./pkg/es/mocks
 	$(MOCKERY) --all --dir ./storage/spanstore/ --output ./storage/spanstore/mocks
 	$(MOCKERY) --all --dir ./proto-gen/storage_v1/ --output ./proto-gen/storage_v1/mocks
 

Makefile.Tools.mk

@@ -0,0 +1,39 @@
+# Copyright (c) 2024 The Jaeger Authors.
+# Copyright The OpenTelemetry Authors
+# SPDX-License-Identifier: Apache-2.0
+
+TOOLS_MOD_DIR   := $(SRC_ROOT)/internal/tools
+TOOLS_BIN_DIR   := $(SRC_ROOT)/.tools
+TOOLS_MOD_REGEX := "\s+_\s+\".*\""
+TOOLS_PKG_NAMES := $(shell grep -E $(TOOLS_MOD_REGEX) < $(TOOLS_MOD_DIR)/tools.go | tr -d " _\"")
+TOOLS_BIN_NAMES := $(addprefix $(TOOLS_BIN_DIR)/, $(notdir $(shell echo $(TOOLS_PKG_NAMES) | sed 's|/v[0-9]||g')))
+
+GOFUMPT       := $(TOOLS_BIN_DIR)/gofumpt
+GOVERSIONINFO := $(TOOLS_BIN_DIR)/goversioninfo
+LINT          := $(TOOLS_BIN_DIR)/golangci-lint
+MOCKERY       := $(TOOLS_BIN_DIR)/mockery
+
+# this target is useful for setting up local workspace, but from CI we want to call more specific ones
+.PHONY: install-tools
+install-tools: $(TOOLS_BIN_NAMES)
+
+.PHONY: install-test-tools
+install-test-tools: $(LINT) $(GOFUMPT)
+
+.PHONY: install-build-tools
+install-build-tools: $(GOVERSIONINFO)
+
+.PHONY: install-ci
+install-ci: install-test-tools install-build-tools
+
+list-internal-tools:
+	@echo Third party tool modules:
+	@echo $(TOOLS_PKG_NAMES) | tr ' ' '\n' | sed 's/^/- /g'
+	@echo Third party tool binaries:
+	@echo $(TOOLS_BIN_NAMES) | tr ' ' '\n' | sed 's/^/- /g'
+
+$(TOOLS_BIN_DIR):
+	mkdir -p $@
+
+$(TOOLS_BIN_NAMES): $(TOOLS_BIN_DIR) $(TOOLS_MOD_DIR)/go.mod
+	cd $(TOOLS_MOD_DIR) && $(GO) build -o $@ -trimpath $(shell echo $(TOOLS_PKG_NAMES) | tr ' ' '\n' | grep $(notdir $@))

cmd/agent/app/builder.go

@@ -167,10 +167,11 @@ func (b *Builder) getProcessors(rep reporter.Reporter, mFactory metrics.Factory,
 
 // GetHTTPServer creates an HTTP server that provides sampling strategies and baggage restrictions to client libraries.
 func (c HTTPServerConfiguration) getHTTPServer(manager configmanager.ClientConfigManager, mFactory metrics.Factory, logger *zap.Logger) *http.Server {
-	if c.HostPort == "" {
-		c.HostPort = defaultHTTPServerHostPort
+	hostPort := c.HostPort
+	if hostPort == "" {
+		hostPort = defaultHTTPServerHostPort
 	}
-	return httpserver.NewHTTPServer(c.HostPort, manager, mFactory, logger)
+	return httpserver.NewHTTPServer(hostPort, manager, mFactory, logger)
 }
 
 // GetThriftProcessor gets a TBufferedServer backed Processor using the collector configuration

cmd/agent/app/builder_test.go

@@ -182,11 +182,11 @@ func TestMultipleCollectorProxies(t *testing.T) {
 
 type fakeCollectorProxy struct{}
 
-func (f fakeCollectorProxy) GetReporter() reporter.Reporter {
+func (fakeCollectorProxy) GetReporter() reporter.Reporter {
 	return fakeCollectorProxy{}
 }
 
-func (f fakeCollectorProxy) GetManager() configmanager.ClientConfigManager {
+func (fakeCollectorProxy) GetManager() configmanager.ClientConfigManager {
 	return fakeCollectorProxy{}
 }
 
@@ -202,7 +202,7 @@ func (fakeCollectorProxy) Close() error {
 	return nil
 }
 
-func (f fakeCollectorProxy) GetSamplingStrategy(_ context.Context, _ string) (*api_v2.SamplingStrategyResponse, error) {
+func (fakeCollectorProxy) GetSamplingStrategy(_ context.Context, _ string) (*api_v2.SamplingStrategyResponse, error) {
 	return nil, errors.New("no peers available")
 }
 

cmd/agent/app/configmanager/grpc/manager.go

@@ -47,6 +47,6 @@ func (s *ConfigManagerProxy) GetSamplingStrategy(ctx context.Context, serviceNam
 }
 
 // GetBaggageRestrictions returns baggage restrictions from collector.
-func (s *ConfigManagerProxy) GetBaggageRestrictions(_ context.Context, _ string) ([]*baggage.BaggageRestriction, error) {
+func (*ConfigManagerProxy) GetBaggageRestrictions(_ context.Context, _ string) ([]*baggage.BaggageRestriction, error) {
 	return nil, errors.New("baggage not implemented")
 }

cmd/agent/app/customtransport/buffered_read_transport.go

@@ -36,19 +36,19 @@ func NewTBufferedReadTransport(readBuf *bytes.Buffer) (*TBufferedReadTransport,
 
 // IsOpen does nothing as transport is not maintaining the connection
 // Required to maintain thrift.TTransport interface
-func (p *TBufferedReadTransport) IsOpen() bool {
+func (*TBufferedReadTransport) IsOpen() bool {
 	return true
 }
 
 // Open does nothing as transport is not maintaining the connection
 // Required to maintain thrift.TTransport interface
-func (p *TBufferedReadTransport) Open() error {
+func (*TBufferedReadTransport) Open() error {
 	return nil
 }
 
 // Close does nothing as transport is not maintaining the connection
 // Required to maintain thrift.TTransport interface
-func (p *TBufferedReadTransport) Close() error {
+func (*TBufferedReadTransport) Close() error {
 	return nil
 }
 
@@ -72,6 +72,6 @@ func (p *TBufferedReadTransport) Write(buf []byte) (int, error) {
 
 // Flush does nothing as udp server does not write responses back
 // Required to maintain thrift.TTransport interface
-func (p *TBufferedReadTransport) Flush(_ context.Context) error {
+func (*TBufferedReadTransport) Flush(_ context.Context) error {
 	return nil
 }

cmd/agent/app/reporter/grpc/flags.go

@@ -51,11 +51,11 @@ func (b *ConnBuilder) InitFromViper(v *viper.Viper) (*ConnBuilder, error) {
 		b.CollectorHostPorts = strings.Split(hostPorts, ",")
 	}
 	b.MaxRetry = uint(v.GetInt(retry))
-	if tls, err := tlsFlagsConfig.InitFromViper(v); err == nil {
-		b.TLS = tls
-	} else {
+	tls, err := tlsFlagsConfig.InitFromViper(v)
+	if err != nil {
 		return b, fmt.Errorf("failed to process TLS options: %w", err)
 	}
+	b.TLS = tls
 	b.DiscoveryMinPeers = v.GetInt(discoveryMinPeers)
 	return b, nil
 }

cmd/agent/app/reporter/grpc/reporter_test.go

@@ -183,7 +183,7 @@ func TestReporter_MakeModelKeyValue(t *testing.T) {
 
 type mockMultitenantSpanHandler struct{}
 
-func (h *mockMultitenantSpanHandler) PostSpans(ctx context.Context, r *api_v2.PostSpansRequest) (*api_v2.PostSpansResponse, error) {
+func (*mockMultitenantSpanHandler) PostSpans(ctx context.Context, r *api_v2.PostSpansRequest) (*api_v2.PostSpansResponse, error) {
 	md, ok := metadata.FromIncomingContext(ctx)
 	if !ok {
 		return &api_v2.PostSpansResponse{}, status.Errorf(codes.PermissionDenied, "missing tenant header")