Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

Commit d29091a

Browse files
author
leo
authored
insights: allow to save the default number of series samples (#47329)
1 parent e2ede87 commit d29091a

File tree

14 files changed

+165
-39
lines changed

14 files changed

+165
-39
lines changed

‎dev/gqltest/code_insights_test.go‎

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -559,6 +559,75 @@ func TestUpdateInsight(t *testing.T) {
559559
}
560560
})
561561

562+
t.Run("default filters are saved on update", func(t *testing.T) {
563+
repos := []string{"repo1"}
564+
intervalUnit := "MONTH"
565+
intervalValue := 4
566+
dataSeries := map[string]any{
567+
"query": "lang:css",
568+
"options": map[string]string{
569+
"label": "insights",
570+
"lineColor": "#6495ED",
571+
},
572+
}
573+
repoScope := map[string]any{
574+
"repositories": repos,
575+
}
576+
timeScope := map[string]any{
577+
"stepInterval": map[string]any{
578+
"unit": intervalUnit,
579+
"value": intervalValue,
580+
},
581+
}
582+
insight, err := client.CreateSearchInsight("my gqltest insight", dataSeries, repoScope, timeScope)
583+
if err != nil {
584+
t.Fatal(err)
585+
}
586+
if insight.InsightViewId == "" {
587+
t.Fatal("Did not get an insight view ID")
588+
}
589+
defer func() {
590+
if err := client.DeleteInsightView(insight.InsightViewId); err != nil {
591+
t.Fatalf("couldn't disable insight series: %v", err)
592+
}
593+
}()
594+
595+
if insight.Label != "insights" {
596+
t.Errorf("wrong label: %v", insight.Label)
597+
}
598+
if insight.Color != "#6495ED" {
599+
t.Errorf("wrong color: %v", insight.Color)
600+
}
601+
602+
dataSeries["seriesId"] = insight.SeriesId
603+
dataSeries["options"] = map[string]any{
604+
"label": "insights 2",
605+
"lineColor": "green",
606+
}
607+
608+
var numSamples int32 = 32
609+
updatedInsight, err := client.UpdateSearchInsight(insight.InsightViewId, map[string]any{
610+
"dataSeries": []any{
611+
dataSeries,
612+
},
613+
"presentationOptions": map[string]string{},
614+
"viewControls": map[string]any{
615+
"filters": struct{}{},
616+
"seriesDisplayOptions": map[string]int32{
617+
"numSamples": numSamples,
618+
},
619+
},
620+
"repositoryScope": repoScope,
621+
"timeScope": timeScope,
622+
})
623+
if err != nil {
624+
t.Fatal(err)
625+
}
626+
627+
if updatedInsight.NumSamples != numSamples {
628+
t.Errorf("wrong number of samples: %d", updatedInsight.NumSamples)
629+
}
630+
})
562631
}
563632

564633
func TestSaveInsightAsNewView(t *testing.T) {

‎enterprise/cmd/frontend/internal/insights/resolvers/insight_series_resolver.go‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -324,7 +324,13 @@ func getRecordedSeriesPointOpts(ctx context.Context, db database.DB, timeseriesS
324324
opts.ID = &definition.InsightSeriesID
325325
opts.SupportsAugmentation = definition.SupportsAugmentation
326326

327-
oldest, err := timeseriesStore.GetOffsetNRecordingTime(ctx, definition.InsightSeriesID, options.NumSamples, false)
327+
// by this point the numSamples option should be set correctly but we're reusing the same struct across functions
328+
// so set max again.
329+
numSamples := 90
330+
if options.NumSamples != nil && *options.NumSamples < 90 && *options.NumSamples > 0 {
331+
numSamples = int(*options.NumSamples)
332+
}
333+
oldest, err := timeseriesStore.GetOffsetNRecordingTime(ctx, definition.InsightSeriesID, numSamples, false)
328334
if err != nil {
329335
return nil, errors.Wrap(err, "GetOffsetNRecordingTime")
330336
}

‎enterprise/cmd/frontend/internal/insights/resolvers/insight_series_resolver_test.go‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111
"github.com/stretchr/testify/require"
1212

1313
"github.com/sourcegraph/log/logtest"
14+
1415
"github.com/sourcegraph/sourcegraph/internal/api"
1516

1617
"github.com/sourcegraph/sourcegraph/cmd/frontend/graphqlbackend"
@@ -359,7 +360,7 @@ func Test_NumSamplesFiltering(t *testing.T) {
359360

360361
tests := []struct {
361362
name string
362-
numSamples int
363+
numSamples int32
363364
}{
364365
{
365366
name: "one",
@@ -380,10 +381,10 @@ func Test_NumSamplesFiltering(t *testing.T) {
380381
}
381382
for _, test := range tests {
382383
t.Run(test.name, func(t *testing.T) {
383-
points, err := fetchSeries(ctx, types.InsightViewSeries{SeriesID: series.SeriesID, InsightSeriesID: series.ID}, types.InsightViewFilters{}, types.SeriesDisplayOptions{NumSamples: test.numSamples}, &base)
384+
points, err := fetchSeries(ctx, types.InsightViewSeries{SeriesID: series.SeriesID, InsightSeriesID: series.ID}, types.InsightViewFilters{}, types.SeriesDisplayOptions{NumSamples: &test.numSamples}, &base)
384385
require.NoError(t, err)
385386

386-
assert.Equal(t, test.numSamples, len(points))
387+
assert.Equal(t, int(test.numSamples), len(points))
387388
t.Log(points)
388389
for i := range points {
389390
assert.Equal(t, times[len(points)-i-1].Timestamp, points[i].Time)

‎enterprise/cmd/frontend/internal/insights/resolvers/insight_view_resolvers.go‎

Lines changed: 8 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -103,9 +103,7 @@ func (i *insightViewSeriesDisplayOptionsResolver) SortOptions(ctx context.Contex
103103
}
104104

105105
func (i *insightViewSeriesDisplayOptionsResolver) NumSamples() *int32 {
106-
v := int32(i.seriesDisplayOptions.NumSamples)
107-
return &v
108-
106+
return i.seriesDisplayOptions.NumSamples
109107
}
110108

111109
type insightViewSeriesSortOptionsResolver struct {
@@ -589,6 +587,7 @@ func (r *Resolver) UpdateLineChartSearchInsight(ctx context.Context, args *graph
589587
SeriesSortMode: seriesSortMode,
590588
SeriesSortDirection: seriesSortDirection,
591589
SeriesLimit: args.Input.ViewControls.SeriesDisplayOptions.Limit,
590+
SeriesNumSamples: args.Input.ViewControls.SeriesDisplayOptions.NumSamples,
592591
})
593592
if err != nil {
594593
return nil, errors.Wrap(err, "UpdateView")
@@ -1094,12 +1093,10 @@ func (d *InsightViewQueryConnectionResolver) Nodes(ctx context.Context) ([]graph
10941093
Direction: types.SeriesSortDirection(d.args.SeriesDisplayOptions.SortOptions.Direction),
10951094
}
10961095
}
1097-
numSamples := 90
1098-
if d.args.SeriesDisplayOptions.NumSamples != nil {
1099-
numSamples = int(*d.args.SeriesDisplayOptions.NumSamples)
1100-
if numSamples > 90 {
1101-
numSamples = 90
1102-
}
1096+
numSamples := d.args.SeriesDisplayOptions.NumSamples
1097+
if numSamples != nil && *numSamples > 90 {
1098+
var maxNumSamples int32 = 90
1099+
numSamples = &maxNumSamples
11031100
}
11041101
resolver.overrideSeriesOptions = &types.SeriesDisplayOptions{
11051102
SortOptions: sortOptions,
@@ -1303,8 +1300,8 @@ func createAndAttachSeries(ctx context.Context, tx *store.InsightStore, startSer
13031300
}
13041301

13051302
// Don't try to match on non-global series, since they are always replaced
1306-
// Also don't try to match on series that use repo critieria
1307-
// TODO: Reconsider matching on on criteria based series. If so the edit case would need work to ensure other insights remain the same.
1303+
// Also don't try to match on series that use repo criteria
1304+
// TODO: Reconsider matching on criteria based series. If so the edit case would need work to ensure other insights remain the same.
13081305
if len(series.RepositoryScope.Repositories) == 0 && series.RepositoryScope.RepositoryCriteria == nil {
13091306
matchingSeries, foundSeries, err = tx.FindMatchingSeries(ctx, store.MatchSeriesArgs{
13101307
Query: series.Query,

‎enterprise/internal/insights/store/insight_store.go‎

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -262,6 +262,7 @@ func (s *InsightStore) GroupByView(ctx context.Context, viewSeries []types.Insig
262262
SeriesOptions: types.SeriesDisplayOptions{
263263
SortOptions: sortOptions,
264264
Limit: seriesSet[0].SeriesLimit,
265+
NumSamples: seriesSet[0].SeriesNumSamples,
265266
},
266267
})
267268
}
@@ -482,6 +483,7 @@ func scanInsightViewSeries(rows *sql.Rows, queryErr error) (_ []types.InsightVie
482483
&temp.SeriesSortMode,
483484
&temp.SeriesSortDirection,
484485
&temp.SeriesLimit,
486+
&temp.SeriesNumSamples,
485487
&temp.GroupBy,
486488
&temp.BackfillAttempts,
487489
&temp.SupportsAugmentation,
@@ -615,6 +617,7 @@ func (s *InsightStore) UpdateView(ctx context.Context, view types.InsightView) (
615617
view.SeriesSortMode,
616618
view.SeriesSortDirection,
617619
view.SeriesLimit,
620+
view.SeriesNumSamples,
618621
view.UniqueID,
619622
))
620623
var id int
@@ -958,7 +961,7 @@ returning id;`
958961
const updateInsightViewSql = `
959962
UPDATE insight_view SET title = %s, description = %s, default_filter_include_repo_regex = %s, default_filter_exclude_repo_regex = %s,
960963
default_filter_search_contexts = %s, other_threshold = %s, presentation_type = %s, series_sort_mode = %s, series_sort_direction = %s,
961-
series_limit = %s
964+
series_limit = %s, series_num_samples = %s
962965
WHERE unique_id = %s
963966
RETURNING id;`
964967

@@ -976,8 +979,8 @@ i.id, i.series_id, i.query, i.created_at, i.oldest_historical_at, i.last_recorde
976979
i.next_recording_after, i.backfill_queued_at, i.last_snapshot_at, i.next_snapshot_after, i.repositories,
977980
i.sample_interval_unit, i.sample_interval_value, iv.default_filter_include_repo_regex, iv.default_filter_exclude_repo_regex,
978981
iv.other_threshold, iv.presentation_type, i.generated_from_capture_groups, i.just_in_time, i.generation_method, iv.is_frozen,
979-
default_filter_search_contexts, iv.series_sort_mode, iv.series_sort_direction, iv.series_limit, i.group_by, i.backfill_attempts,
980-
i.supports_augmentation, i.repository_criteria
982+
default_filter_search_contexts, iv.series_sort_mode, iv.series_sort_direction, iv.series_limit, iv.series_num_samples,
983+
i.group_by, i.backfill_attempts, i.supports_augmentation, i.repository_criteria
981984
FROM (%s) iv
982985
JOIN insight_view_series ivs ON iv.id = ivs.insight_view_id
983986
JOIN insight_series i ON ivs.insight_series_id = i.id
@@ -991,8 +994,8 @@ i.id, i.series_id, i.query, i.created_at, i.oldest_historical_at, i.last_recorde
991994
i.next_recording_after, i.backfill_queued_at, i.last_snapshot_at, i.next_snapshot_after, i.repositories,
992995
i.sample_interval_unit, i.sample_interval_value, iv.default_filter_include_repo_regex, iv.default_filter_exclude_repo_regex,
993996
iv.other_threshold, iv.presentation_type, i.generated_from_capture_groups, i.just_in_time, i.generation_method, iv.is_frozen,
994-
default_filter_search_contexts, iv.series_sort_mode, iv.series_sort_direction, iv.series_limit, i.group_by, i.backfill_attempts,
995-
i.supports_augmentation, i.repository_criteria
997+
default_filter_search_contexts, iv.series_sort_mode, iv.series_sort_direction, iv.series_limit, iv.series_num_samples,
998+
i.group_by, i.backfill_attempts, i.supports_augmentation, i.repository_criteria
996999
FROM dashboard_insight_view as dbiv
9971000
JOIN insight_view iv ON iv.id = dbiv.insight_view_id
9981001
JOIN insight_view_series ivs ON iv.id = ivs.insight_view_id
@@ -1032,8 +1035,8 @@ SELECT iv.id, 0 as dashboard_insight_id, iv.unique_id, iv.title, iv.description,
10321035
i.next_recording_after, i.backfill_queued_at, i.last_snapshot_at, i.next_snapshot_after, i.repositories,
10331036
i.sample_interval_unit, i.sample_interval_value, iv.default_filter_include_repo_regex, iv.default_filter_exclude_repo_regex,
10341037
iv.other_threshold, iv.presentation_type, i.generated_from_capture_groups, i.just_in_time, i.generation_method, iv.is_frozen,
1035-
default_filter_search_contexts, iv.series_sort_mode, iv.series_sort_direction, iv.series_limit, i.group_by, i.backfill_attempts,
1036-
i.supports_augmentation, i.repository_criteria
1038+
default_filter_search_contexts, iv.series_sort_mode, iv.series_sort_direction, iv.series_limit, iv.series_num_samples,
1039+
i.group_by, i.backfill_attempts, i.supports_augmentation, i.repository_criteria
10371040
10381041
FROM insight_view iv
10391042
JOIN insight_view_series ivs ON iv.id = ivs.insight_view_id

‎enterprise/internal/insights/store/insight_store_test.go‎

Lines changed: 34 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -287,9 +287,10 @@ func TestGetAll(t *testing.T) {
287287
groupByRepo := "repo"
288288
ctx := context.Background()
289289

290+
store := NewInsightStore(insightsDB)
291+
290292
// First test the method on an empty database.
291293
t.Run("test empty database", func(t *testing.T) {
292-
store := NewInsightStore(insightsDB)
293294
got, err := store.GetAll(ctx, InsightQueryArgs{})
294295
if err != nil {
295296
t.Fatal(err)
@@ -348,8 +349,7 @@ func TestGetAll(t *testing.T) {
348349
t.Fatal(err)
349350
}
350351

351-
t.Run("test all results", func(t *testing.T) {
352-
store := NewInsightStore(insightsDB)
352+
t.Run("all results", func(t *testing.T) {
353353
got, err := store.GetAll(ctx, InsightQueryArgs{})
354354
if err != nil {
355355
t.Fatal(err)
@@ -476,7 +476,7 @@ func TestGetAll(t *testing.T) {
476476
t.Errorf("unexpected insight view series want/got: %s", diff)
477477
}
478478
})
479-
t.Run("test first result", func(t *testing.T) {
479+
t.Run("first result", func(t *testing.T) {
480480
store := NewInsightStore(insightsDB)
481481
got, err := store.GetAll(ctx, InsightQueryArgs{Limit: 1})
482482
if err != nil {
@@ -535,8 +535,7 @@ func TestGetAll(t *testing.T) {
535535
t.Errorf("unexpected insight view series want/got: %s", diff)
536536
}
537537
})
538-
t.Run("test second result", func(t *testing.T) {
539-
store := NewInsightStore(insightsDB)
538+
t.Run("second result", func(t *testing.T) {
540539
got, err := store.GetAll(ctx, InsightQueryArgs{Limit: 1, After: "b"})
541540
if err != nil {
542541
t.Fatal(err)
@@ -594,8 +593,7 @@ func TestGetAll(t *testing.T) {
594593
t.Errorf("unexpected insight view series want/got: %s", diff)
595594
}
596595
})
597-
t.Run("test last 2 results", func(t *testing.T) {
598-
store := NewInsightStore(insightsDB)
596+
t.Run("last 2 results", func(t *testing.T) {
599597
got, err := store.GetAll(ctx, InsightQueryArgs{After: "b"})
600598
if err != nil {
601599
t.Fatal(err)
@@ -676,8 +674,7 @@ func TestGetAll(t *testing.T) {
676674
t.Errorf("unexpected insight view series want/got: %s", diff)
677675
}
678676
})
679-
t.Run("test find by title results ", func(*testing.T) {
680-
store := NewInsightStore(insightsDB)
677+
t.Run("find by title results", func(*testing.T) {
681678
got, err := store.GetAll(ctx, InsightQueryArgs{Find: "view 3"})
682679
if err != nil {
683680
t.Fatal(err)
@@ -735,9 +732,7 @@ func TestGetAll(t *testing.T) {
735732
t.Errorf("unexpected insight view series want/got: %s", diff)
736733
}
737734
})
738-
739-
t.Run("test find by series label results ", func(*testing.T) {
740-
store := NewInsightStore(insightsDB)
735+
t.Run("find by series label results", func(*testing.T) {
741736
got, err := store.GetAll(ctx, InsightQueryArgs{Find: "label5-1"})
742737
if err != nil {
743738
t.Fatal(err)
@@ -796,7 +791,6 @@ func TestGetAll(t *testing.T) {
796791
}
797792
})
798793
t.Run("exclude insight ids from results", func(t *testing.T) {
799-
store := NewInsightStore(insightsDB)
800794
got, err := store.GetAll(ctx, InsightQueryArgs{ExcludeIDs: []string{"b", "e"}})
801795
if err != nil {
802796
t.Fatal(err)
@@ -854,6 +848,32 @@ func TestGetAll(t *testing.T) {
854848
t.Errorf("unexpected insight view series want/got: %s", diff)
855849
}
856850
})
851+
t.Run("returns expected number of samples", func(t *testing.T) {
852+
// Set the series_num_samples value
853+
numSamples := int32(50)
854+
view, err := store.UpdateView(ctx, types.InsightView{
855+
UniqueID: "d",
856+
PresentationType: types.Line, // setting for null constraint
857+
SeriesNumSamples: &numSamples,
858+
})
859+
if err != nil {
860+
t.Fatal(err)
861+
}
862+
if diff := cmp.Diff(&numSamples, view.SeriesNumSamples); diff != "" {
863+
t.Errorf("unexpected insight view series num samples want/got: %s", diff)
864+
}
865+
866+
series, err := store.GetAll(ctx, InsightQueryArgs{UniqueIDs: []string{"d"}})
867+
if err != nil {
868+
t.Fatal(err)
869+
}
870+
// we're only testing the number of samples in this test cases
871+
for _, s := range series {
872+
if diff := cmp.Diff(&numSamples, s.SeriesNumSamples); diff != "" {
873+
t.Errorf("unexpected insight view series num samples want/got: %s", diff)
874+
}
875+
}
876+
})
857877
}
858878

859879
func TestGetAllOnDashboard(t *testing.T) {
@@ -1196,7 +1216,6 @@ func TestCreateSeries(t *testing.T) {
11961216
}
11971217
})
11981218
t.Run("test create and get capture groups series", func(t *testing.T) {
1199-
store := NewInsightStore(insightsDB)
12001219
sampleIntervalUnit := "MONTH"
12011220
repoCriteria := "repo:a"
12021221
_, err := store.CreateSeries(ctx, types.InsightSeries{

‎enterprise/internal/insights/types/types.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ type InsightViewSeries struct {
4242
BackfillAttempts int32
4343
SupportsAugmentation bool
4444
RepositoryCriteria *string
45+
SeriesNumSamples *int32
4546
}
4647

4748
type Insight struct {
@@ -83,6 +84,7 @@ type InsightView struct {
8384
SeriesSortMode *SeriesSortMode
8485
SeriesSortDirection *SeriesSortDirection
8586
SeriesLimit *int32
87+
SeriesNumSamples *int32
8688
}
8789

8890
// InsightSeries is a single data series for a Code Insight. This contains some metadata about the data series, as well
@@ -186,7 +188,7 @@ const (
186188
type SeriesDisplayOptions struct {
187189
SortOptions *SeriesSortOptions
188190
Limit *int32
189-
NumSamples int
191+
NumSamples *int32
190192
}
191193

192194
type SeriesSortOptions struct {

‎internal/database/schema.codeinsights.json‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1606,6 +1606,19 @@
16061606
"GenerationExpression": "",
16071607
"Comment": ""
16081608
},
1609+
{
1610+
"Name": "series_num_samples",
1611+
"Index": 14,
1612+
"TypeName": "integer",
1613+
"IsNullable": true,
1614+
"Default": "",
1615+
"CharacterMaximumLength": 0,
1616+
"IsIdentity": false,
1617+
"IdentityGeneration": "",
1618+
"IsGenerated": "NEVER",
1619+
"GenerationExpression": "",
1620+
"Comment": ""
1621+
},
16091622
{
16101623
"Name": "series_sort_direction",
16111624
"Index": 12,

0 commit comments

Comments
 (0)