insights: update gql to allow control over number of series samples#46284
Conversation
|
Codenotify: Notifying subscribers in CODENOTIFY files for diff fc2652e...ca0ef93.
|
chwarwick
left a comment
There was a problem hiding this comment.
Is the plan to persist this value in the future? I think there 2 potential impacts from this.
- hourly/daily/weekly insights could start showing fewer points by default - I think this is ok
- Monthly insights start showing more points - I also think this is ok
I do think we should remove it from the resolver if it is just returning user input.
| """ | ||
| Max number of samples to return for all the series on the view. | ||
| """ | ||
| numSamples: Int |
There was a problem hiding this comment.
It feels wrong to include this is the options of the series if this isn't persisted and is just returning whatever was sent in the input options.
There was a problem hiding this comment.
It does allow us to "soft validate" by replacing invalid (too big) values with a max. I'm not strongly opinionated either way though.
There was a problem hiding this comment.
I can remove it until I work on this issue if you'd rather: https://github.com/sourcegraph/sourcegraph/issues/46283
There was a problem hiding this comment.
leaving this in, added soft-validation (max 90 samples). that issue linked will take care on allowing to actually set a default number of samples (which will need frontend work to actually work...)
yes that is the plan (https://github.com/sourcegraph/sourcegraph/issues/46283) it's just a lot of boilerplate to implement that functionality, so I didn't want to detract from the actual logic being implemented here |
closes https://github.com/sourcegraph/sourcegraph/issues/45750
open https://github.com/sourcegraph/sourcegraph/issues/46283 as follow up, frontend changes still need to be done
Test plan
Added unit tests
Manually tested:
variables
{"id":"aW5zaWdodF92aWV3OiIySjA3YTE4bGZTZEZzenYxOXp4QjhrZWQ5RVki","filters":{"includeRepoRegex":"","excludeRepoRegex":"","searchContexts":[""]},"seriesDisplayOptions":{"limit":20,"sortOptions":{"direction":"DESC","mode":"RESULT_COUNT"}, "numSamples": 1}}result
{ "data": { "insightViews": { "nodes": [ { "id": "aW5zaWdodF92aWV3OiIySjA3YTE4bGZTZEZzenYxOXp4QjhrZWQ5RVki", "dataSeries": [ { "seriesId": "2J07a7NrF1cNQjEGfFF8XU2ygmR", "label": "I hate", "points": [ { "dateTime": "2023-01-10T13:44:42Z", "value": 2, "diffQuery": "repo:leonore before:2023-01-10T13:44:42Z type:diff I hate", "__typename": "InsightDataPoint" } ], "__typename": "InsightsSeries" } ], "__typename": "InsightView" } ], "__typename": "InsightViewConnection" } } }