TraceQL Metrics: Fix incorrect optimization for some queries

CHANGELOG.md

@@ -56,6 +56,7 @@ configurable via the throughput_bytes_slo field, and it will populate op="traces
 * [BUGFIX] Fix behavior for queries like {.foo && true} and {.foo || false} [#4855](https://github.com/grafana/tempo/pull/4855) (@stoewer)
 * [BUGFIX] Fix performance bottleneck and file cleanup in block builder [#4550](https://github.com/grafana/tempo/pull/4550) (@mdisibio)
 * [BUGFIX] TraceQL incorrect results for additional spanset filters after a select operation [#4600](https://github.com/grafana/tempo/pull/4600) (@mdisibio)
+* [BUGFIX] TraceQL metrics incorrect results for queries with multiple filters that reside in non-dedicated columns that also group by the same variable [#4887](https://github.com/grafana/tempo/pull/4887) (@mdisibio)
 * [BUGFIX] TraceQL results caching bug for floats ending in .0 [#4539](https://github.com/grafana/tempo/pull/4539) (@carles-grafana)
 * [BUGFIX] Rhythm: fix sorting order for partition consumption [#4747](https://github.com/grafana/tempo/pull/4747) (@javiermolinar)
 * [BUGFIX] Rhythm: fix block builder to not reuse a block ID if it was already flushed, to prevent read errors [#4872](https://github.com/grafana/tempo/pull/4872) (@mdisibio)

pkg/traceql/engine_metrics.go

@@ -919,32 +919,51 @@ func optimize(req *FetchSpansRequest) {
 	// layer and doesn't alter the correctness of the query.
 	// This can't be done for plain attributes or in all cases.
 	if len(req.SecondPassConditions) > 0 {
-		secondLayerAlwaysPresent := true
-		for _, cond := range req.SecondPassConditions {
-			if cond.Attribute.Intrinsic != IntrinsicNone {
-				continue
-			}
+		if !allIntrinsics(req.SecondPassConditions) {
+			// Cannot move them.
+			return
+		}
 
-			// This is a very special case. resource.service.name is also always present
-			// (required by spec) so it can be moved too.
-			if cond.Attribute.Scope == AttributeScopeResource && cond.Attribute.Name == "service.name" {
-				continue
-			}
+		// Move all to first pass
+		req.Conditions = append(req.Conditions, req.SecondPassConditions...)
+		req.SecondPassConditions = nil
+	}
+
+	// If the remaining first layer is a single condition, then we can
+	// eliminate the callback.
+	if len(req.Conditions) == 1 {
+		req.SecondPass = nil
+		return
+	}
 
-			secondLayerAlwaysPresent = false
+	// Finally if the entire remaining first layer is intrinsics, then we can
+	// elmininate the callback.
+	// TODO(mdisibio): We can't eliminate the callback if other conditions exist, despite AllConditions, because the storage layer
+	// doesn't strictly honor AllConditions for multiple attributes of the same type in the shared columns.
+	// For example: { span.string_a = "foo" && span.string_b = "bar" } | rate()
+	// This query requires the callback to assert the attributes correctly.
+	// Dedicated columns aren't affected but we don't have the information to distinguish that here.
+	if len(req.Conditions) > 0 && allIntrinsics(req.Conditions) {
+		req.SecondPass = nil
+	}
+}
+
+func allIntrinsics(conds []Condition) bool {
+	for _, cond := range conds {
+		if cond.Attribute.Intrinsic != IntrinsicNone {
+			continue
 		}
 
-		if secondLayerAlwaysPresent {
-			// Move all to first pass
-			req.Conditions = append(req.Conditions, req.SecondPassConditions...)
-			req.SecondPass = nil
-			req.SecondPassConditions = nil
+		// This is a very special case. resource.service.name is also always present
+		// (required by spec) so it can be moved too.
+		if cond.Attribute.Scope == AttributeScopeResource && cond.Attribute.Name == "service.name" {
+			continue
 		}
-	}
 
-	if len(req.SecondPassConditions) == 0 {
-		req.SecondPass = nil
+		// Anything else is not an intrinsic
+		return false
 	}
+	return true
 }
 
 func lookup(needles []Attribute, haystack Span) Static {

pkg/traceql/engine_metrics_test.go

@@ -320,6 +320,107 @@ func TestCompileMetricsQueryRangeFetchSpansRequest(t *testing.T) {
 	}
 }
 
+func TestOptimizeFetchSpansRequest(t *testing.T) {
+	secondPass := func(_ *Spanset) ([]*Spanset, error) {
+		return nil, nil
+	}
+
+	tc := []struct {
+		name     string
+		input    FetchSpansRequest
+		expected FetchSpansRequest
+	}{
+		{
+			name: "Not able to be optimized because not all conditions",
+			input: FetchSpansRequest{
+				SecondPass: secondPass,
+				SecondPassConditions: []Condition{
+					{Attribute: IntrinsicNameAttribute},
+					{Attribute: NewScopedAttribute(AttributeScopeResource, false, "service.name")},
+				},
+			},
+			expected: FetchSpansRequest{
+				SecondPass: secondPass,
+				SecondPassConditions: []Condition{
+					{Attribute: IntrinsicNameAttribute},
+					{Attribute: NewScopedAttribute(AttributeScopeResource, false, "service.name")},
+				},
+			},
+		},
+		{
+			name: "Intrinsics moved to first pass and second pass eliminated",
+			input: FetchSpansRequest{
+				AllConditions: true,
+				SecondPass:    secondPass,
+				SecondPassConditions: []Condition{
+					{Attribute: IntrinsicNameAttribute},
+					{Attribute: NewScopedAttribute(AttributeScopeResource, false, "service.name")},
+				},
+			},
+			expected: FetchSpansRequest{
+				AllConditions: true,
+				Conditions: []Condition{
+					{Attribute: IntrinsicNameAttribute},
+					{Attribute: NewScopedAttribute(AttributeScopeResource, false, "service.name")},
+				},
+			},
+		},
+		{
+			name: "Unscoped cannot be optimized",
+			input: FetchSpansRequest{
+				AllConditions: true,
+				Conditions: []Condition{
+					{Attribute: NewScopedAttribute(AttributeScopeNone, false, "http.status_code")},
+				},
+				SecondPass: secondPass,
+			},
+			expected: FetchSpansRequest{
+				AllConditions: true,
+				Conditions: []Condition{
+					{Attribute: NewScopedAttribute(AttributeScopeNone, false, "http.status_code")},
+				},
+				SecondPass: secondPass,
+			},
+		},
+		{
+			name: "Single scoped non-intrinsic can still elminiate second pass",
+			input: FetchSpansRequest{
+				AllConditions: true,
+				Conditions: []Condition{
+					{Attribute: NewScopedAttribute(AttributeScopeSpan, false, "http.status_code")},
+				},
+				SecondPass: secondPass,
+			},
+			expected: FetchSpansRequest{
+				AllConditions: true,
+				Conditions: []Condition{
+					{Attribute: NewScopedAttribute(AttributeScopeSpan, false, "http.status_code")},
+				},
+			},
+		},
+	}
+
+	for _, c := range tc {
+		t.Run(c.name, func(t *testing.T) {
+			// Make a copy
+			actual := &c.input
+			optimize(actual)
+
+			// Instead of comparing func pointers, check if they are both set or not
+			if c.expected.SecondPass != nil {
+				require.NotNil(t, actual.SecondPass)
+			} else {
+				require.Nil(t, actual.SecondPass)
+			}
+
+			// Now nil out func and compare the rest
+			c.expected.SecondPass = nil
+			actual.SecondPass = nil
+			require.Equal(t, c.expected, *actual)
+		})
+	}
+}
+
 func TestQuantileOverTime(t *testing.T) {
 	req := &tempopb.QueryRangeRequest{
 		Start: uint64(1 * time.Second),

tempodb/encoding/vparquet4/block_traceql_test.go

@@ -1101,6 +1101,7 @@ func BenchmarkBackendBlockQueryRange(b *testing.B) {
 		"{} | rate() by (resource.service.name)",
 		"{} | rate() by (span.http.url)", // High cardinality attribute
 		"{resource.service.name=`loki-ingester`} | rate()",
+		"{span.http.host != `` && span.http.flavor=`2`} | rate() by (span.http.flavor)", // Multiple conditions
 		"{status=error} | rate()",
 	}
 
@@ -1119,7 +1120,7 @@ func BenchmarkBackendBlockQueryRange(b *testing.B) {
 
 	for _, tc := range testCases {
 		b.Run(tc, func(b *testing.B) {
-			for _, minutes := range []int{5, 7} {
+			for _, minutes := range []int{5} {
 				b.Run(strconv.Itoa(minutes), func(b *testing.B) {
 					st := block.meta.StartTime
 					end := st.Add(time.Duration(minutes) * time.Minute)