From 076cd87f020bc737bd29f6f408db343b3407aea7 Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Sun, 26 Nov 2023 22:45:42 -0500 Subject: [PATCH] Separate all-in-one integration tests for v1 and v2 (#4968) ## Which problem is this PR solving? - Turns out integration test for jaeger-v2 was never working correctly because v1 all-in-one was still running and tests were succeeding against that ## Description of the changes - build-all-in-one workflow - Skip into two jobs to avoid cross interference - DRY by using PR_ONLY variable - all-in-one.go test - Replace build tag with a check for env var, to allow the test to compile in the IDE - Allow skipping sampling strategy test which is not working yet in v2 - remove unused `cmd/jaeger/internal/all-in-one/` package - use our standard Tracer constructor to pass a tracer to query-service in v2 (OTEL-coll doesn't support internal tracer yet) ## How was this change tested? - CI --------- Signed-off-by: Yuri Shkuro --- .codecov.yml | 1 + .github/workflows/ci-all-in-one-build.yml | 40 +++--- Makefile | 15 +- cmd/agent/.nocover | 0 cmd/all-in-one/.nocover | 1 + cmd/all-in-one/all_in_one_test.go | 117 ++++++++------- cmd/jaeger/internal/all-in-one/.nocover | 1 - cmd/jaeger/internal/all-in-one/config.go | 133 ------------------ .../internal/extension/jaegerquery/server.go | 10 +- scripts/check-test-files.sh | 11 +- 10 files changed, 118 insertions(+), 211 deletions(-) delete mode 100644 cmd/agent/.nocover create mode 100644 cmd/all-in-one/.nocover delete mode 100644 cmd/jaeger/internal/all-in-one/.nocover delete mode 100644 cmd/jaeger/internal/all-in-one/config.go diff --git a/.codecov.yml b/.codecov.yml index d1a3cbdcb49..f203175966f 100644 --- a/.codecov.yml +++ b/.codecov.yml @@ -11,6 +11,7 @@ ignore: - "thrift-gen/*/*" - "**/thrift-0.9.2/*" - "swagger-gen/*/*" + - "**/main.go" coverage: precision: 2 diff --git a/.github/workflows/ci-all-in-one-build.yml b/.github/workflows/ci-all-in-one-build.yml index 17443e21c26..c02c7ef4c85 100644 --- a/.github/workflows/ci-all-in-one-build.yml +++ b/.github/workflows/ci-all-in-one-build.yml @@ -18,6 +18,16 @@ permissions: jobs: all-in-one: runs-on: ubuntu-latest + strategy: + matrix: + mode: + - name: v1 + binary: all-in-one + skip_sampling: false + - name: v2 + binary: jaeger + skip_sampling: true + steps: - name: Harden Runner uses: step-security/harden-runner@eb238b55efaa70779f274895e782ed17c84f2895 @@ -47,24 +57,22 @@ jobs: - uses: docker/setup-qemu-action@68827325e0b33c7199eb31dd4e31fbe9023e06e3 - - name: Build only linux/amd64 docker image for Pull Request - if: github.ref_name != 'main' - run: bash scripts/build-all-in-one-image.sh pr-only - - - name: Build and test jaeger (v2) as all-in-one - if: github.ref_name != 'main' - run: BINARY=jaeger bash scripts/build-all-in-one-image.sh pr-only + - name: Define PR_ONLY var if running on a Pull Request + run: | + case ${GITHUB_EVENT_NAME} in + pull_request) + echo "PR_ONLY=pr-only" >> ${GITHUB_ENV} + ;; + *) + echo "PR_ONLY=" >> ${GITHUB_ENV} + ;; + esac - name: Build, test, and publish all-in-one image - if: github.ref_name == 'main' - run: bash scripts/build-all-in-one-image.sh - env: - DOCKERHUB_TOKEN: ${{ secrets.DOCKERHUB_TOKEN }} - QUAY_TOKEN: ${{ secrets.QUAY_TOKEN }} - - - name: Build, test, and publish jaeger (v2) image - if: github.ref_name == 'main' - run: BINARY=jaeger bash scripts/build-all-in-one-image.sh + run: | + BINARY=${{ matrix.mode.binary }} \ + SKIP_SAMPLING=${{ matrix.mode.skip_sampling }} \ + bash scripts/build-all-in-one-image.sh ${{ env.PR_ONLY }} env: DOCKERHUB_TOKEN: ${{ secrets.DOCKERHUB_TOKEN }} QUAY_TOKEN: ${{ secrets.QUAY_TOKEN }} diff --git a/Makefile b/Makefile index 22900966367..ee5974ba8b2 100644 --- a/Makefile +++ b/Makefile @@ -9,7 +9,7 @@ GO = go include docker/Makefile include crossdock/rules.mk -# TODO we can comparmentalize this Makefile better, by separting: +# TODO we can compartmentalize this Makefile better, by separting: # - thrift and proto builds # - integration tests # - all the binary building targets @@ -107,11 +107,6 @@ echo-all-pkgs: echo-all-srcs: @echo $(ALL_SRC) | tr ' ' '\n' | sort -# TODO: no files actually use this right now -.PHONY: go-gen -go-gen: - @echo skipping go generate ./... - .PHONY: clean clean: rm -rf cover*.out .cover/ cover.html $(FMT_LOG) $(IMPORT_LOG) \ @@ -121,15 +116,15 @@ clean: find cmd -type f -executable | xargs -I{} sh -c '(git ls-files --error-unmatch {} 2>/dev/null || rm -v {})' .PHONY: test -test: go-gen +test: bash -c "set -e; set -o pipefail; $(GOTEST) -tags=memory_storage_integration ./... $(COLORIZE)" .PHONY: all-in-one-integration-test -all-in-one-integration-test: go-gen - $(GOTEST) -tags=integration ./cmd/all-in-one/... +all-in-one-integration-test: + TEST_MODE=integration $(GOTEST) ./cmd/all-in-one/ .PHONY: storage-integration-test -storage-integration-test: go-gen +storage-integration-test: # Expire tests results for storage integration tests since the environment might change # even though the code remains the same. go clean -testcache diff --git a/cmd/agent/.nocover b/cmd/agent/.nocover deleted file mode 100644 index e69de29bb2d..00000000000 diff --git a/cmd/all-in-one/.nocover b/cmd/all-in-one/.nocover new file mode 100644 index 00000000000..8ab06f28b67 --- /dev/null +++ b/cmd/all-in-one/.nocover @@ -0,0 +1 @@ +main is not testable diff --git a/cmd/all-in-one/all_in_one_test.go b/cmd/all-in-one/all_in_one_test.go index 6bb20c12560..c6852ef7b98 100644 --- a/cmd/all-in-one/all_in_one_test.go +++ b/cmd/all-in-one/all_in_one_test.go @@ -13,9 +13,6 @@ // See the License for the specific language governing permissions and // limitations under the License. -//go:build integration -// +build integration - package main import ( @@ -23,6 +20,7 @@ import ( "encoding/json" "io" "net/http" + "os" "strings" "testing" "time" @@ -37,50 +35,92 @@ import ( "github.com/jaegertracing/jaeger/proto-gen/api_v3" ) +// These tests are only run when the environment variable TEST_MODE=integration is set. +// An optional SKIP_SAMPLING=true environment variable can be used to skip sampling checks (for jaeger-v2). + const ( - host = "0.0.0.0" - queryPort = "16686" - agentPort = "5778" - queryHostPort = host + ":" + queryPort - queryURL = "http://" + queryHostPort - agentHostPort = host + ":" + agentPort - agentURL = "http://" + agentHostPort - - getServicesURL = queryURL + "/api/services" - getSamplingStrategyURL = agentURL + "/sampling?service=whatever" - - getServicesAPIV3URL = queryURL + "/api/v3/services" + host = "0.0.0.0" + queryPort = "16686" + agentPort = "5778" + queryAddr = "http://" + host + ":" + queryPort + agentAddr = "http://" + host + ":" + agentPort + + getServicesURL = "/api/services" + getTraceURL = "/api/traces/" + getServicesAPIV3URL = "/api/v3/services" + getSamplingStrategyURL = "/sampling?service=whatever" ) -var getTraceURL = queryURL + "/api/traces/" +var traceID string // stores state exchanged between createTrace and getAPITrace var httpClient = &http.Client{ Timeout: time.Second, } func TestAllInOne(t *testing.T) { + if os.Getenv("TEST_MODE") != "integration" { + t.Skip("Integration test for all-in-one skipped; set environment variable TEST_MODE=integration to enable") + } + // Check if the query service is available healthCheck(t) - t.Run("Check if the favicon icon is available", jaegerLogoCheck) + t.Run("checkWebUI", checkWebUI) t.Run("createTrace", createTrace) t.Run("getAPITrace", getAPITrace) t.Run("getSamplingStrategy", getSamplingStrategy) t.Run("getServicesAPIV3", getServicesAPIV3) } +func healthCheck(t *testing.T) { + require.Eventuallyf( + t, + func() bool { + _, err := http.Get(queryAddr + "/") + return err == nil + }, + 10*time.Second, + time.Second, + "expecting query endpoint to be healhty", + ) + t.Logf("Server detected at %s", queryAddr) +} + +func checkWebUI(t *testing.T) { + t.Run("logo", func(t *testing.T) { + resp, err := http.Get(queryAddr + "/static/jaeger-logo-ab11f618.svg") + require.NoError(t, err) + require.NotNil(t, resp) + defer resp.Body.Close() + assert.Equal(t, http.StatusOK, resp.StatusCode) + }) + t.Run("React app", func(t *testing.T) { + resp, err := http.Get(queryAddr + "/") + require.NoError(t, err) + require.NotNil(t, resp) + defer resp.Body.Close() + assert.Equal(t, http.StatusOK, resp.StatusCode) + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + assert.Contains(t, string(body), `
`) + }) +} + func createTrace(t *testing.T) { - req, err := http.NewRequest(http.MethodGet, getServicesURL, nil) + // Since all requests to query service are traces, creating a new trace + // is simply a matter of querying one of the endpoints. + req, err := http.NewRequest(http.MethodGet, queryAddr+getServicesURL, nil) require.NoError(t, err) resp, err := httpClient.Do(req) require.NoError(t, err) defer resp.Body.Close() traceResponse := resp.Header.Get("traceresponse") + // Expecting: [version] [trace-id] [child-id] [trace-flags] parts := strings.Split(traceResponse, "-") - require.Len(t, parts, 4) // [version] [trace-id] [child-id] [trace-flags] - traceID := parts[1] - getTraceURL += traceID + require.Len(t, parts, 4, "traceResponse=%s", traceResponse) + traceID = parts[1] + t.Logf("Created trace %s", traceID) } type response struct { @@ -88,7 +128,7 @@ type response struct { } func getAPITrace(t *testing.T) { - req, err := http.NewRequest(http.MethodGet, getTraceURL, nil) + req, err := http.NewRequest(http.MethodGet, queryAddr+getTraceURL+traceID, nil) require.NoError(t, err) var queryResponse response @@ -113,7 +153,11 @@ func getAPITrace(t *testing.T) { } func getSamplingStrategy(t *testing.T) { - req, err := http.NewRequest(http.MethodGet, getSamplingStrategyURL, nil) + // TODO once jaeger-v2 can pass this test, remove from .github/workflows/ci-all-in-one-build.yml + if os.Getenv("SKIP_SAMPLING") == "true" { + t.Skip("skipping sampling strategy check because SKIP_SAMPLING=true is set") + } + req, err := http.NewRequest(http.MethodGet, agentAddr+getSamplingStrategyURL, nil) require.NoError(t, err) resp, err := httpClient.Do(req) @@ -130,30 +174,8 @@ func getSamplingStrategy(t *testing.T) { assert.EqualValues(t, 1.0, queryResponse.ProbabilisticSampling.SamplingRate) } -func healthCheck(t *testing.T) { - t.Log("Health-checking all-in-one...") - require.Eventuallyf( - t, - func() bool { - _, err := http.Get(queryURL) - return err == nil - }, - 10*time.Second, - time.Second, - "expecting query endpoint to be healhty", - ) -} - -func jaegerLogoCheck(t *testing.T) { - t.Log("Checking favicon...") - resp, err := http.Get(queryURL + "/static/jaeger-logo-ab11f618.svg") - require.NoError(t, err) - require.NotNil(t, resp) - assert.Equal(t, http.StatusOK, resp.StatusCode) -} - func getServicesAPIV3(t *testing.T) { - req, err := http.NewRequest(http.MethodGet, getServicesAPIV3URL, nil) + req, err := http.NewRequest(http.MethodGet, queryAddr+getServicesAPIV3URL, nil) require.NoError(t, err) resp, err := httpClient.Do(req) require.NoError(t, err) @@ -163,5 +185,6 @@ func getServicesAPIV3(t *testing.T) { jsonpb := runtime.JSONPb{} err = jsonpb.Unmarshal(body, &servicesResponse) require.NoError(t, err) - assert.Equal(t, []string{"jaeger-all-in-one"}, servicesResponse.GetServices()) + require.Len(t, servicesResponse.GetServices(), 1) + assert.Contains(t, servicesResponse.GetServices()[0], "jaeger") } diff --git a/cmd/jaeger/internal/all-in-one/.nocover b/cmd/jaeger/internal/all-in-one/.nocover deleted file mode 100644 index 9d6cf4b7fb6..00000000000 --- a/cmd/jaeger/internal/all-in-one/.nocover +++ /dev/null @@ -1 +0,0 @@ -FIXME diff --git a/cmd/jaeger/internal/all-in-one/config.go b/cmd/jaeger/internal/all-in-one/config.go deleted file mode 100644 index d97aa729372..00000000000 --- a/cmd/jaeger/internal/all-in-one/config.go +++ /dev/null @@ -1,133 +0,0 @@ -// Copyright (c) 2023 The Jaeger Authors. -// SPDX-License-Identifier: Apache-2.0 - -package allinone - -import ( - "context" - "fmt" - "time" - - "go.opentelemetry.io/collector/component" - "go.opentelemetry.io/collector/config/configtelemetry" - "go.opentelemetry.io/collector/otelcol" - "go.opentelemetry.io/collector/service" - "go.opentelemetry.io/collector/service/extensions" - "go.opentelemetry.io/collector/service/pipelines" - "go.opentelemetry.io/collector/service/telemetry" - "go.uber.org/zap/zapcore" - - "github.com/jaegertracing/jaeger/cmd/jaeger/internal/exporters/storageexporter" - "github.com/jaegertracing/jaeger/cmd/jaeger/internal/extension/jaegerquery" - "github.com/jaegertracing/jaeger/cmd/jaeger/internal/extension/jaegerstorage" -) - -type configProvider struct { - watcher chan error -} - -var _ otelcol.ConfigProvider = (*configProvider)(nil) - -// NewConfigProvider creates a new ConfigProvider. -func NewConfigProvider() *configProvider { - return &configProvider{ - watcher: make(chan error, 1), - } -} - -func (cp *configProvider) Get(ctx context.Context, factories otelcol.Factories) (*otelcol.Config, error) { - cfg := &otelcol.Config{ - Service: cp.makeServiceConfig(), - Extensions: make(map[component.ID]component.Config), - Receivers: make(map[component.ID]component.Config), - Processors: make(map[component.ID]component.Config), - Exporters: make(map[component.ID]component.Config), - } - defaultConfigs("extension", cfg.Service.Extensions, cfg.Extensions, factories.Extensions) - for _, pipeCfg := range cfg.Service.Pipelines { - defaultConfigs("receiver", pipeCfg.Receivers, cfg.Receivers, factories.Receivers) - defaultConfigs("processor", pipeCfg.Processors, cfg.Processors, factories.Processors) - defaultConfigs("exporter", pipeCfg.Exporters, cfg.Exporters, factories.Exporters) - } - return cfg, nil -} - -func defaultConfigs[TFactory component.Factory]( - componentType string, - comps []component.ID, - outCfg map[component.ID]component.Config, - factories map[component.Type]TFactory, -) error { - for _, compID := range comps { - f, ok := factories[compID.Type()] - if !ok { - return fmt.Errorf("no factory registered for %s %v", componentType, compID) - } - cfg := f.CreateDefaultConfig() - outCfg[compID] = cfg - } - return nil -} - -// makeServiceConfig creates default config that contains -// all standard all-in-one extensions and pipelines. -func (cp *configProvider) makeServiceConfig() service.Config { - return service.Config{ - Extensions: extensions.Config([]component.ID{ - jaegerstorage.ID, - jaegerquery.ID, - }), - Pipelines: pipelines.Config(map[component.ID]*pipelines.PipelineConfig{ - component.NewID("traces"): { - Receivers: []component.ID{ - component.NewID("otlp"), - component.NewID("jaeger"), - component.NewID("zipkin"), - }, - Processors: []component.ID{ - component.NewID("batch"), - }, - Exporters: []component.ID{ - storageexporter.ID, - }, - }, - }), - // TODO: OTel Collector currently (v0.87) hardcodes telemetry settings, this is a copy. - // https://github.com/open-telemetry/opentelemetry-collector/blob/35512c466577036b0cc306673d2d4ad039c77f1c/otelcol/unmarshaler.go#L43 - Telemetry: telemetry.Config{ - Logs: telemetry.LogsConfig{ - Level: zapcore.InfoLevel, - Development: false, - Encoding: "console", - Sampling: &telemetry.LogsSamplingConfig{ - Enabled: true, - Tick: 10 * time.Second, - Initial: 10, - Thereafter: 100, - }, - OutputPaths: []string{"stderr"}, - ErrorOutputPaths: []string{"stderr"}, - DisableCaller: false, - DisableStacktrace: false, - InitialFields: map[string]any(nil), - }, - Metrics: telemetry.MetricsConfig{ - Level: configtelemetry.LevelNone, - // Address: ":8888", - }, - // TODO initialize tracer - }, - } -} - -// Watch implements otelcol.ConfigProvider. -// The returned channel is never written to, as there is no configuration to watch. -func (cp *configProvider) Watch() <-chan error { - return cp.watcher -} - -// Shutdown implements otelcol.ConfigProvider. -func (cp *configProvider) Shutdown(ctx context.Context) error { - close(cp.watcher) - return nil -} diff --git a/cmd/jaeger/internal/extension/jaegerquery/server.go b/cmd/jaeger/internal/extension/jaegerquery/server.go index 470a02ea868..bc83ba06ef6 100644 --- a/cmd/jaeger/internal/extension/jaegerquery/server.go +++ b/cmd/jaeger/internal/extension/jaegerquery/server.go @@ -69,6 +69,14 @@ func (s *server) Start(ctx context.Context, host component.Host) error { metricsQueryService, _ := disabled.NewMetricsReader() tm := tenancy.NewManager(&s.config.Tenancy) + // TODO OTel-collector does not initialize the tracer currently + // https://github.com/open-telemetry/opentelemetry-collector/issues/7532 + //nolint + jtracer, err := jtracer.New("jaeger") + if err != nil { + return fmt.Errorf("could not initialize a tracer: %w", err) + } + // TODO contextcheck linter complains about next line that context is not passed. It is not wrong. //nolint s.server, err = queryApp.NewServer( @@ -77,7 +85,7 @@ func (s *server) Start(ctx context.Context, host component.Host) error { metricsQueryService, s.makeQueryOptions(), tm, - jtracer.NoOp(), + jtracer, ) if err != nil { return fmt.Errorf("could not create jaeger-query: %w", err) diff --git a/scripts/check-test-files.sh b/scripts/check-test-files.sh index 0724cb56ba8..7ffc896688c 100755 --- a/scripts/check-test-files.sh +++ b/scripts/check-test-files.sh @@ -1,10 +1,15 @@ #!/bin/bash -set -euo pipefail +# Copyright (c) 2023 The Jaeger Authors. +# SPDX-License-Identifier: Apache-2.0 + +# This script checks that all directories with go files +# have at least one *_test.go file or a .nocover file. -COLOR_FIXME=$(printf "\033[31mFIXME\033[0m") +set -euo pipefail NO_TEST_FILE_DIRS="" + # shellcheck disable=SC2048 for dir in $*; do mainFile=$(find "${dir}" -maxdepth 1 -name 'main.go') @@ -20,7 +25,7 @@ for dir in $*; do exit 1 fi echo "Package excluded from coverage: ${dir}" - echo " reason: ${reason}" | sed "/FIXME/s//${COLOR_FIXME}/" + echo " reason: ${reason}" | sed "s/FIXME/🔴 FIXME/" continue fi if [ -z "${NO_TEST_FILE_DIRS}" ]; then