Skip unneeded query when getting CVE chart (#45813)
<!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **Related issue:** Resolves #45720 # Details When requesting CVE chart data, we were making a call to get the set of tracked CVEs to filter the data by. Currently we're only _collecting_ data for the tracked CVEs, so there's no reason to make this call at all. When we add more filtering options and start collecting more data, we'll need a call like this again, and will likely need to start caching the results. Otherwise it's a multi-second cost per query on large deployments. # Checklist for submitter If some of the following don't apply, delete the relevant line. - [X] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. ## Testing - [X] Added/updated automated tests - removed some outdated tests - replaced with a test that checks that when an entity filter returns no items (an empty, rather than nil slice) we get empty buckets returned rather than getting data for all entities. This is a regression test for when we add filtering back. - [X] QA'd all new/changed functionality manually - validated that chart still loads and shows the same data. - tried it on a load test env and saw dramatic API request time improvement <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **Bug Fixes** * Optimized CVE chart data retrieval by eliminating redundant queries, reducing unnecessary database operations and improving performance. * Fixed entity ID filtering logic to correctly handle edge cases and prevent unintended filter interactions across metrics. * **Tests** * Added test coverage for chart data queries with empty entity filters. * Improved test isolation to ensure metric-specific filtering behavior is properly separated. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/fleetdm/fleet/pull/45813?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This commit is contained in:
@@ -0,0 +1 @@
|
||||
- Remove unneeded call to get tracked CVEs when reading CVE chart data
|
||||
@@ -328,6 +328,33 @@ func TestGetSCDDataMixedEncoding(t *testing.T) {
|
||||
assert.Equal(t, 5, pts[0].Value, "union of dense {1,2,3} and roaring {3,4,5} = {1,2,3,4,5}")
|
||||
}
|
||||
|
||||
// TestGetSCDDataEmptyEntityIDsReturnsZeroBuckets pins the non-nil empty
|
||||
// entityIDs contract: a caller signaling "filter requested but resolved to
|
||||
// nothing" must get zero-valued buckets across the date range — not an error
|
||||
// from `IN ()` and not an empty slice. Rows are seeded that would match if the
|
||||
// filter were nil; the empty-slice filter must exclude them.
|
||||
func TestGetSCDDataEmptyEntityIDsReturnsZeroBuckets(t *testing.T) {
|
||||
tdb := testutils.SetupTestDB(t, "chart_mysql")
|
||||
ds := NewDatastore(tdb.Conns(), tdb.Logger)
|
||||
|
||||
startDate := time.Date(2026, 4, 21, 0, 0, 0, 0, time.UTC)
|
||||
bucketSize := 24 * time.Hour
|
||||
endDate := startDate.Add(3 * bucketSize)
|
||||
validFrom := startDate.Add(-time.Hour)
|
||||
|
||||
tdb.InsertSCDRowWithHostIDs(t, "cve", "CVE-A", []uint{1, 2, 3}, validFrom, scdOpenSentinel)
|
||||
tdb.InsertSCDRowWithHostIDs(t, "cve", "CVE-B", []uint{4, 5}, validFrom, scdOpenSentinel)
|
||||
|
||||
pts, err := ds.GetSCDData(t.Context(), "cve",
|
||||
startDate, endDate, bucketSize,
|
||||
api.SampleStrategySnapshot, nil, []string{})
|
||||
require.NoError(t, err)
|
||||
require.Len(t, pts, 3, "one bucket per slot across the date range, not an empty slice")
|
||||
for i, dp := range pts {
|
||||
assert.Zero(t, dp.Value, "bucket %d must be zero — empty entityIDs filter excludes all rows", i)
|
||||
}
|
||||
}
|
||||
|
||||
func testScrubOtherDatasetUnaffected(t *testing.T, tdb *testutils.TestDB, ds *Datastore) {
|
||||
now := time.Now().UTC()
|
||||
|
||||
|
||||
@@ -139,15 +139,6 @@ func (s *Service) GetChartData(ctx context.Context, metric string, opts api.Requ
|
||||
}
|
||||
|
||||
var entityIDs []string
|
||||
if metric == "cve" {
|
||||
// TODO(iteration-2): replace with user-configurable filter from
|
||||
// RequestOpts when dynamic CVE filtering ships.
|
||||
entityIDs, err = s.store.TrackedCriticalCVEs(ctx)
|
||||
if err != nil {
|
||||
return nil, ctxerr.Wrap(ctx, err, "resolve tracked critical CVEs")
|
||||
}
|
||||
}
|
||||
|
||||
// entityIDs semantics at the storage layer: nil = no filter; non-nil empty
|
||||
// = match nothing (produces zero-valued buckets). Do NOT convert empty to
|
||||
// nil here.
|
||||
|
||||
@@ -294,59 +294,6 @@ func TestGetChartDataCVEResolution(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
func TestGetChartDataCVEUsesCuratedFilter(t *testing.T) {
|
||||
ds := &mockDatastore{}
|
||||
svc := NewService(&mockAuthorizer{}, ds, globalViewer(), nil)
|
||||
svc.RegisterDataset(&chart.CVEDataset{})
|
||||
|
||||
ds.trackedCriticalCVEsFn = func(_ context.Context) ([]string, error) {
|
||||
return []string{"CVE-A", "CVE-B"}, nil
|
||||
}
|
||||
var gotEntityIDs []string
|
||||
ds.getSCDDataFunc = func(_ context.Context, _ string, _, _ time.Time, _ time.Duration, _ api.SampleStrategy, _ *roaring.Bitmap, entityIDs []string) ([]api.DataPoint, error) {
|
||||
gotEntityIDs = entityIDs
|
||||
return nil, nil
|
||||
}
|
||||
|
||||
_, err := svc.GetChartData(t.Context(), "cve", api.RequestOpts{Days: 7})
|
||||
require.NoError(t, err)
|
||||
assert.Equal(t, []string{"CVE-A", "CVE-B"}, gotEntityIDs)
|
||||
}
|
||||
|
||||
func TestGetChartDataCVEEmptySetReturnsZeros(t *testing.T) {
|
||||
ds := &mockDatastore{}
|
||||
svc := NewService(&mockAuthorizer{}, ds, globalViewer(), nil)
|
||||
svc.RegisterDataset(&chart.CVEDataset{})
|
||||
|
||||
// Non-nil empty slice — the resolver produced no matches but a filter
|
||||
// was requested. The service MUST pass this through verbatim so the
|
||||
// storage layer's "AND 1=0" path fires.
|
||||
ds.trackedCriticalCVEsFn = func(_ context.Context) ([]string, error) {
|
||||
return []string{}, nil
|
||||
}
|
||||
var gotEntityIDs []string
|
||||
gotEntityIDsIsNil := true
|
||||
ds.getSCDDataFunc = func(_ context.Context, _ string, startDate, endDate time.Time, bucketSize time.Duration, _ api.SampleStrategy, _ *roaring.Bitmap, entityIDs []string) ([]api.DataPoint, error) {
|
||||
gotEntityIDs = entityIDs
|
||||
gotEntityIDsIsNil = entityIDs == nil
|
||||
numBuckets := int(endDate.Sub(startDate) / bucketSize)
|
||||
points := make([]api.DataPoint, numBuckets)
|
||||
for i := range points {
|
||||
points[i] = api.DataPoint{Timestamp: startDate.Add(time.Duration(i+1) * bucketSize), Value: 0}
|
||||
}
|
||||
return points, nil
|
||||
}
|
||||
|
||||
resp, err := svc.GetChartData(t.Context(), "cve", api.RequestOpts{Days: 7})
|
||||
require.NoError(t, err)
|
||||
assert.False(t, gotEntityIDsIsNil, "service must pass non-nil empty slice so storage layer emits AND 1=0")
|
||||
assert.Empty(t, gotEntityIDs)
|
||||
require.NotEmpty(t, resp.Data)
|
||||
for _, dp := range resp.Data {
|
||||
assert.Zero(t, dp.Value)
|
||||
}
|
||||
}
|
||||
|
||||
func TestGetChartDataUptimePassesNilEntityIDs(t *testing.T) {
|
||||
ds := &mockDatastore{}
|
||||
svc := NewService(&mockAuthorizer{}, ds, globalViewer(), nil)
|
||||
|
||||
Reference in New Issue
Block a user