ts: update server queries to account for system tenant id #109727
Conversation
|
This change is |
Santamaura
force-pushed
the
ts-doublecount-fix
branch
from
eda7f70 to
32c4331
Compare
Santamaura
requested review from
a team and
ericharmeling
and removed request for
a team
There was a problem hiding this comment.
Nice! Just have a few minor comments
Reviewed 1 of 2 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @Santamaura)
pkg/ts/query.go line 928 at r1 (raw file):
} else if tenantSource == tenantID.String() { rows = append(rows, row) }
nit
Suggestion:
if tenantID.IsSystem() && tenantSource == "" || tenantSource == tenantID.String() {
rows = append(rows, row)
}pkg/ts/server_test.go line 416 at r1 (raw file):
Queries: []tspb.Query{ { Name: "test.metric",
Specifying no source aggregates across all sources too. This is a bit confusing and probably worth adding a comment on this test (and the one below).
I don't know what a "source" means in this context, but I'm guessing it's a node? Also worth clarifying.
pkg/ts/server_test.go line 432 at r1 (raw file):
require.Equal(t, expectedAggregatedResult, aggregatedResponse) // Ssytem tenant ID should provide system tenant ts data.
nit
Suggestion:
System
Santamaura
force-pushed
the
ts-doublecount-fix
branch
from
32c4331 to
56bdd16
Compare
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.
Santamaura
force-pushed
the
ts-doublecount-fix
branch
from
56bdd16 to
738d312
Compare
There was a problem hiding this comment.
TFTR
Reviewable status:
pkg/ts/query.go line 928 at r1 (raw file):
Previously, ericharmeling (Eric Harmeling) wrote…
nit
D
pkg/ts/server_test.go line 432 at r1 (raw file):
Previously, ericharmeling (Eric Harmeling) wrote…
nit
Done
There was a problem hiding this comment.
Weird reviewable did not post some of my comments, redid those ones.
Reviewable status:
pkg/ts/query.go line 928 at r1 (raw file):
Previously, Santamaura (Alex Santamaura) wrote…
D
Done
pkg/ts/server_test.go line 416 at r1 (raw file):
Previously, ericharmeling (Eric Harmeling) wrote…
Specifying no source aggregates across all sources too. This is a bit confusing and probably worth adding a comment on this test (and the one below).
I don't know what a "source" means in this context, but I'm guessing it's a node? Also worth clarifying.
Added comments to the tests, source is nodeID or storeID
|
bors r+ |
|
Build succeeded: |
|
blathers backport 23.1 |
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.
Some screenshots after the change:
All
System
Tenant
