From 738d3126b716d9fefcef0b5723cdc8349f089cab Mon Sep 17 00:00:00 2001 From: Alex Santamaura Date: Wed, 30 Aug 2023 11:56:20 -0400 Subject: [PATCH] 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 #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. --- pkg/ts/query.go | 16 ++++++---- pkg/ts/server_test.go | 72 +++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 79 insertions(+), 9 deletions(-) diff --git a/pkg/ts/query.go b/pkg/ts/query.go index 9fe0fd1308d6..bd3f10fdb2b3 100644 --- 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 index 80858c46ea8a..1c8bbe02018d 100644 --- 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,6 +417,7 @@ func TestServerQueryTenant(t *testing.T) { Sources: []string{"1"}, }, { + // Not providing a source (nodeID or storeID) will aggregate across all sources. Name: "test.metric", }, }, @@ -424,6 +425,70 @@ func TestServerQueryTenant(t *testing.T) { 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", }, },