Separate all-in-one integration tests for v1 and v2 (jaegertracing#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 <[email protected]>

.codecov.yml

@@ -11,6 +11,7 @@ ignore:
   - "thrift-gen/*/*"
   - "**/thrift-0.9.2/*"
   - "swagger-gen/*/*"
+  - "**/main.go"
 
 coverage:
   precision: 2

.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 }}

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

cmd/all-in-one/.nocover

@@ -0,0 +1 @@
+main is not testable

cmd/all-in-one/all_in_one_test.go

@@ -13,16 +13,14 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-//go:build integration
-// +build integration
-
 package main
 
 import (
 	"bytes"
 	"encoding/json"
 	"io"
 	"net/http"
+	"os"
 	"strings"
 	"testing"
 	"time"
@@ -37,58 +35,100 @@ 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), `<div id="jaeger-ui-root"></div>`)
+	})
+}
+
 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 {
 	Data []*ui.Trace `json:"data"`
 }
 
 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")
 }