Fix span deduplication via correct ordering of adjusters (jaegertraci…

…ng#6116)

## Which problem is this PR solving?
- Resolves jaegertracing#6001 (again). If ClockSkew adjuster is run before
DedupeBySpanHash, it will add warnings to spans with duplicate span IDs,
which affect the hash code, which means that you "dedupe" to two copies
of each span, not one.

## Description of the changes
- Change the order of Adjusters so that DedupeBySpanHash runs before
ClockSkew

## How was this change tested?
- In production, lightly
- Also unit tested

## Checklist
- [x] I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [x] I have added unit tests for the new functionality
- [x] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `yarn lint` and `yarn test`

---------

Signed-off-by: Chris Danis <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Co-authored-by: Yuri Shkuro <[email protected]>
Co-authored-by: Yuri Shkuro <[email protected]>

cmd/query/app/http_handler_test.go

@@ -162,6 +162,38 @@ func TestGetTraceSuccess(t *testing.T) {
 	assert.Empty(t, response.Errors)
 }
 
+func extractTraces(t *testing.T, response *structuredResponse) []ui.Trace {
+	var traces []ui.Trace
+	b, err := json.Marshal(response.Data)
+	require.NoError(t, err)
+	err = json.Unmarshal(b, &traces)
+	require.NoError(t, err)
+	return traces
+}
+
+// TestGetTraceDedupeSuccess partially verifies that the standard adjusteres
+// are defined in correct order.
+func TestGetTraceDedupeSuccess(t *testing.T) {
+	dupedMockTrace := &model.Trace{
+		Spans:    append(mockTrace.Spans, mockTrace.Spans...),
+		Warnings: []string{},
+	}
+
+	ts := initializeTestServer(t)
+	ts.spanReader.On("GetTrace", mock.AnythingOfType("*context.valueCtx"), mock.AnythingOfType("model.TraceID")).
+		Return(dupedMockTrace, nil).Once()
+
+	var response structuredResponse
+	err := getJSON(ts.server.URL+`/api/traces/123456`, &response)
+	require.NoError(t, err)
+	assert.Empty(t, response.Errors)
+	traces := extractTraces(t, &response)
+	assert.Len(t, traces[0].Spans, 2)
+	for _, span := range traces[0].Spans {
+		assert.Empty(t, span.Warnings, "clock skew adjuster would've complained about dupes")
+	}
+}
+
 func TestLogOnServerError(t *testing.T) {
 	zapCore, logs := observer.New(zap.InfoLevel)
 	logger := zap.New(zapCore)
@@ -276,15 +308,6 @@ func TestGetTrace(t *testing.T) {
 		return &trace
 	}
 
-	extractTraces := func(t *testing.T, response *structuredResponse) []ui.Trace {
-		var traces []ui.Trace
-		resBytes, err := json.Marshal(response.Data)
-		require.NoError(t, err)
-		err = json.Unmarshal(resBytes, &traces)
-		require.NoError(t, err)
-		return traces
-	}
-
 	for _, tc := range testCases {
 		testCase := tc // capture loop var
 		t.Run(testCase.suffix, func(t *testing.T) {

cmd/query/app/querysvc/adjusters.go

@@ -15,11 +15,11 @@ import (
 func StandardAdjusters(maxClockSkewAdjust time.Duration) []adjuster.Adjuster {
 	return []adjuster.Adjuster{
 		adjuster.ZipkinSpanIDUniquifier(),
-		adjuster.ClockSkew(maxClockSkewAdjust),
+		adjuster.SortTagsAndLogFields(),
+		adjuster.DedupeBySpanHash(),            // requires SortTagsAndLogFields for deterministic results
+		adjuster.ClockSkew(maxClockSkewAdjust), // adds warnings (which affect SpanHash) on dupe span IDs
 		adjuster.IPTagAdjuster(),
 		adjuster.OTelTagAdjuster(),
-		adjuster.SortTagsAndLogFields(),
-		adjuster.DedupeBySpanHash(), // requires SortTagsAndLogFields for deterministic results
 		adjuster.SpanReferences(),
 		adjuster.ParentReference(),
 	}