ts: update server queries to account for system tenant id

Previously, ts queries would consider providing no tenant id and the system tenant id as the same and would return all the aggregated datapoints. This was likely due to the original implementation considering that the system tenant would always want to view all the aggregated data. This is not the case anymore since the system tenant has the ability to view all the data, system tenant specific data or other tenants data. Therefore this commit adjusts the server query code so that if a system tenant id is provided, it returns data for only the system tenant. Fixes cockroachdb#108929 Release note (bug fix): adjust ts server queries to be able to return system tenant only metrics if tenant id is provided, this will fix an issue where some metrics graphs appear to double count.
renatolabs · Aug 31, 2023 · 738d312 · 738d312
1 parent 8b7fb0c
commit 738d312
Show file tree

Hide file tree

Showing 2 changed files with 79 additions and 9 deletions.
diff --git a/pkg/ts/query.go b/pkg/ts/query.go
@@ -845,10 +845,14 @@ func (db *DB) readFromDatabase(
 	kd := diskResolution.SlabDuration()
 	for currentTimestamp := startTimestamp; currentTimestamp <= timespan.EndNanos; currentTimestamp += kd {
 		for _, source := range sources {
-			// If a TenantID is specified and is not the system tenant, only query
-			// data for that tenant source.
-			if tenantID.IsSet() && !tenantID.IsSystem() {
-				source = tsutil.MakeTenantSource(source, tenantID.String())
+			// If a TenantID is specified we may need to format the source in order to retrieve the correct data.
+			// e.g. if not system tenant we need the source to be of format nodeID-tenantID but if it is the
+			// system tenant we need the source to be of format nodeID. Otherwise we get all the data via the
+			// format nodeID-
+			if tenantID.IsSet() {
+				if !tenantID.IsSystem() {
+					source = tsutil.MakeTenantSource(source, tenantID.String())
+				}
 				key := MakeDataKey(seriesName, source, diskResolution, currentTimestamp)
 				b.Get(key)
 			} else {
@@ -905,7 +909,7 @@ func (db *DB) readAllSourcesFromDatabase(
 		return nil, err
 	}
 
-	if !tenantID.IsSet() || tenantID.IsSystem() {
+	if !tenantID.IsSet() {
 		return b.Results[0].Rows, nil
 	}
 
@@ -917,7 +921,7 @@ func (db *DB) readAllSourcesFromDatabase(
 			return nil, err
 		}
 		_, tenantSource := tsutil.DecodeSource(source)
-		if tenantSource == tenantID.String() {
+		if tenantID.IsSystem() && tenantSource == "" || tenantSource == tenantID.String() {
 			rows = append(rows, row)
 		}
 	}

diff --git a/pkg/ts/server_test.go b/pkg/ts/server_test.go
@@ -368,8 +368,8 @@ func TestServerQueryTenant(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	// System tenant should aggregate across all tenants.
-	expectedSystemResult := &tspb.TimeSeriesQueryResponse{
+	// Undefined tenant ID should aggregate across all tenants.
+	expectedAggregatedResult := &tspb.TimeSeriesQueryResponse{
 		Results: []tspb.TimeSeriesQueryResponse_Result{
 			{
 				Query: tspb.Query{
@@ -408,7 +408,7 @@ func TestServerQueryTenant(t *testing.T) {
 
 	conn := s.RPCClientConn(t, username.RootUserName())
 	client := tspb.NewTimeSeriesClient(conn)
-	systemResponse, err := client.Query(context.Background(), &tspb.TimeSeriesQueryRequest{
+	aggregatedResponse, err := client.Query(context.Background(), &tspb.TimeSeriesQueryRequest{
 		StartNanos: 400 * 1e9,
 		EndNanos:   500 * 1e9,
 		Queries: []tspb.Query{
@@ -417,13 +417,78 @@ func TestServerQueryTenant(t *testing.T) {
 				Sources: []string{"1"},
 			},
 			{
+				// Not providing a source (nodeID or storeID) will aggregate across all sources.
 				Name: "test.metric",
 			},
 		},
 	})
 	if err != nil {
 		t.Fatal(err)
 	}
+	for _, r := range aggregatedResponse.Results {
+		sort.Strings(r.Sources)
+	}
+	require.Equal(t, expectedAggregatedResult, aggregatedResponse)
+
+	// System tenant ID should provide system tenant ts data.
+	systemID := roachpb.MustMakeTenantID(1)
+	expectedSystemResult := &tspb.TimeSeriesQueryResponse{
+		Results: []tspb.TimeSeriesQueryResponse_Result{
+			{
+				Query: tspb.Query{
+					Name:     "test.metric",
+					Sources:  []string{"1"},
+					TenantID: systemID,
+				},
+				Datapoints: []tspb.TimeSeriesDatapoint{
+					{
+						TimestampNanos: 400 * 1e9,
+						Value:          100.0,
+					},
+					{
+						TimestampNanos: 500 * 1e9,
+						Value:          200.0,
+					},
+				},
+			},
+			{
+				Query: tspb.Query{
+					Name:     "test.metric",
+					Sources:  []string{"1", "10"},
+					TenantID: systemID,
+				},
+				Datapoints: []tspb.TimeSeriesDatapoint{
+					{
+						TimestampNanos: 400 * 1e9,
+						Value:          300.0,
+					},
+					{
+						TimestampNanos: 500 * 1e9,
+						Value:          600.0,
+					},
+				},
+			},
+		},
+	}
+
+	systemResponse, err := client.Query(context.Background(), &tspb.TimeSeriesQueryRequest{
+		StartNanos: 400 * 1e9,
+		EndNanos:   500 * 1e9,
+		Queries: []tspb.Query{
+			{
+				Name:     "test.metric",
+				Sources:  []string{"1"},
+				TenantID: systemID,
+			},
+			{
+				Name:     "test.metric",
+				TenantID: systemID,
+			},
+		},
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
 	for _, r := range systemResponse.Results {
 		sort.Strings(r.Sources)
 	}
@@ -489,6 +554,7 @@ func TestServerQueryTenant(t *testing.T) {
 				Sources: []string{"1"},
 			},
 			{
+				// Not providing a source (nodeID or storeID) will aggregate across all sources.
 				Name: "test.metric",
 			},
 		},