insights: update gql schema to allow errors for some insightviews by chwarwick · Pull Request #44491 · sourcegraph/sourcegraph-public-snapshot

chwarwick · 2022-11-16T15:21:10Z

Updates the schema to allow for some insight views to error while still returning the rest of the insights. Previously with the schema as [InsightView!]! any error would propagate up until it found a nullable field which would remove all insightviews and make the insights UI unusable.

closes https://github.com/sourcegraph/sourcegraph/issues/42301

Test plan

With an error forced on the 2nd insight:

{
  "errors": [
    {
      "message": "simulated error",
      "path": ["insightViews", "nodes", 1, "presentation"]
    }
  ],
  "data": {
    "insightViews": {
      "nodes": [
        {
          "id": "aW5zaWdodF92aWV3OiIySEhORjFUU29oazNnM0drNUxEaDM3UnYyMEgi",
          "defaultSeriesDisplayOptions": {
            "limit": 20,
            "sortOptions": {
              "mode": "RESULT_COUNT",
              "direction": "DESC",
              "__typename": "SeriesSortOptions"
            },
            "__typename": "SeriesDisplayOptions"
          },
          "appliedSeriesDisplayOptions": {
            "limit": 20,
            "sortOptions": {
              "mode": "RESULT_COUNT",
              "direction": "DESC",
              "__typename": "SeriesSortOptions"
            },
            "__typename": "SeriesDisplayOptions"
          },
          "isFrozen": false,
          "appliedFilters": {
            "includeRepoRegex": "",
            "excludeRepoRegex": "",
            "searchContexts": [],
            "__typename": "InsightViewFilters"
          },
          "dashboardReferenceCount": 0,
          "dashboards": {
            "nodes": [],
            "__typename": "InsightsDashboardConnection"
          },
          "presentation": {
            "__typename": "LineChartInsightViewPresentation",
            "title": "TODO FIXME",
            "seriesPresentation": [
              {
                "seriesId": "2HXcICBeRFSkdZl1okp5Adbqzqz",
                "label": "FIXME",
                "color": "var(--oc-grape-7)",
                "__typename": "LineChartDataSeriesPresentation"
              },
              {
                "seriesId": "2HXcIDyGCbux9SvdM7DcqcbDuUY",
                "label": "TODO",
                "color": "var(--oc-grape-7)",
                "__typename": "LineChartDataSeriesPresentation"
              }
            ]
          },
          "dataSeriesDefinitions": [
            {
              "seriesId": "2HXcICBeRFSkdZl1okp5Adbqzqz",
              "query": "FIXME     ",
              "repositoryScope": {
                "repositories": [],
                "__typename": "InsightRepositoryScope"
              },
              "timeScope": {
                "unit": "DAY",
                "value": 9,
                "__typename": "InsightIntervalTimeScope"
              },
              "isCalculated": true,
              "generatedFromCaptureGroups": false,
              "groupBy": null,
              "__typename": "SearchInsightDataSeriesDefinition"
            },
            {
              "seriesId": "2HXcIDyGCbux9SvdM7DcqcbDuUY",
              "query": "TODO     ",
              "repositoryScope": {
                "repositories": [],
                "__typename": "InsightRepositoryScope"
              },
              "timeScope": {
                "unit": "DAY",
                "value": 9,
                "__typename": "InsightIntervalTimeScope"
              },
              "isCalculated": true,
              "generatedFromCaptureGroups": false,
              "groupBy": null,
              "__typename": "SearchInsightDataSeriesDefinition"
            }
          ],
          "__typename": "InsightView"
        },
        null
      ],
      "__typename": "InsightViewConnection"
    }
  }
}

The dashboard still renders non-erroring insights:

sg-e2e-regression-test-bob · 2022-11-16T15:28:46Z

Bundle size report 📦

Initial size	Total size	Async size	Modules
0.00% (0.00 kb)	0.00% (+0.30 kb)	0.00% (+0.30 kb)	0.00% (0)

Look at the Statoscope report for a full comparison between the commits 0d6b1e9 and 77009d4 or learn more.

Open explanation

Initial size is the size of the initial bundle (the one that is loaded when you open the page)
Total size is the size of the initial bundle + all the async loaded chunks
Async size is the size of all the async loaded chunks
Modules is the number of modules in the initial bundle

chwarwick · 2022-11-16T15:37:06Z

Attempting to navigate directly to a broken insight returns

vovakulikov

@chwarwick left a few comments

vovakulikov · 2022-11-16T15:41:37Z

-                seriesToggleState.setSelectedSeriesIds([])
-                setInsightData(parsedData)
+                const node = data.insightViews.nodes[0]
+                if (node !== null) {


@chwarwick should we throw an error here if we didn't find insight here? Or this wouldn't be the case here because we filter out the null insights at the query before (where we fetch insight configurations)?

Thought: Even if we filter out the null insights at the beginning it still should be possible to fetch insight configuration without a problem but encounter a problem and get a null value here in the backend insight card. Therefore I think we should handle error case here as well.

Also, I noticed that in the standalone insight page gql handler, we stopped polling manually in case of insight null value. Should we also handle this in this component?

At first I was thinking that if we have a null insight it would always have an error, which is true for our current implementation but the schema does allow that to be null and have no error so I should handle that.

The polling I don't think is an issue because it will get stoped by isFetchingHistoricalData being undefined

@vovakulikov after looking at this more I do not believe there needs to be any additional changes.

If there was an error in 1 more of the resolvers the insight is returned null and the apollo error is populated. This display the error in the card.

Polling will be stoped because a null/undefined insight will produce a falsey isFetchingHistoricalData

vovakulikov · 2022-11-16T15:43:05Z

@@ -0,0 +1,3 @@
+export function isNotNull<T>(value: T | null): value is T {


I think we can use here isDefined method

nice i was thinking something like this had to exist somewhere I just couldn't find it.

felixfbecker · 2022-11-21T18:51:25Z

So this seems to always ignore the errored insights (hide them in lists and treat as a "not found" error for single view). That's an improvement to before, but I think ideally we would still render a card for those insights and pluck the error out of the errors array (where path matches that index) and display it as an alert in the card/in place of the "not found" error. Is that feasible @vovakulikov ?

vovakulikov · 2022-11-21T19:17:16Z

@felixfbecker this is what we discussed with @chwarwick last week when we were having a conversation about this PR. I think it should be technically possible, we would have to change our typings about insight configuration and add there a union type like:

type Insight = InsightConfiguration | Error

Instead of just filtering out the null insights at the top level.
In this case in the card UI we would do a error checking like

isError(insightConfiguration) ? <render the errored card> : <render a plain insight card>

This idea doesn't block this PR though. I think this might be a good follow up issue for this handling errored insight story. @chwarwick if you're agree can you fill a follow up issue for this. (I will make a quick investigation about how difficult it would be to have more verbose error handling before our next planning meeting)

felixfbecker · 2022-11-21T19:36:20Z

Agree that it doesn't block.
Why do we need to add a union type though? Does Apollo not give us access to the existing errors array returned by the GraphQL backend?

The GraphQL schema would quickly become a mess if we added an Error union type for every object. For expected conditions it makes sense to design data into the schema (but not a generic "Error" type, because the fact that they're expected makes them not really an error, but just part of the expected possible states). The goal of this is to handle unexpected errors though, for which GraphQL has the errors array mechanism (and that's where graphql-go will automatically put unexpected exceptions).

vovakulikov · 2022-11-22T02:18:36Z

@felixfbecker I didn't mean the schema, I was talking about FE types and components API.

chwarwick · 2022-11-22T12:34:01Z

So this seems to always ignore the errored insights (hide them in lists and treat as a "not found" error for single view). That's an improvement to before, but I think ideally we would still render a card for those insights and pluck the error out of the errors array (where path matches that index) and display it as an alert in the card/in place of the "not found" error. Is that feasible @vovakulikov ?

@felixfbecker There is a follow up issue for that, this issue is meant to eliminate the worst case scenario of 1 bad insight making the entire insights section unusable.

felixfbecker · 2022-11-22T21:49:54Z

Thanks, but noting that follow-up issue goes much further than what I was suggesting – I'm just suggesting displaying the error, not returning enough data to allow mutation. Even if https://github.com/sourcegraph/sourcegraph/issues/38951 is implemented, there can still be unexpected error cases in which it would probably be good to not just make those insights "disappear" but surface the error at least so the user knows something is wrong and is not just wondering "where did my insights go?".

Chris Warwick added 2 commits November 16, 2022 10:03

allow errors within insight views

a874917

add a null insight to mock test result

b407b88

chwarwick requested a review from vovakulikov November 16, 2022 15:21

cla-bot Bot added the cla-signed label Nov 16, 2022

vovakulikov reviewed Nov 16, 2022

View reviewed changes

Chris Warwick added 2 commits November 16, 2022 11:22

use existing isDefined

dde8494

add comment about err/null case

f92dcff

vovakulikov approved these changes Nov 17, 2022

View reviewed changes

Chris Warwick added 2 commits November 22, 2022 07:52

add change log

e755e6d

Merge branch 'main' into cw/nullable-insightview

0d6b1e9

chwarwick merged commit db011dd into main Nov 22, 2022

chwarwick deleted the cw/nullable-insightview branch November 22, 2022 18:58

		@@ -0,0 +1,3 @@
		export function isNotNull<T>(value: T \| null): value is T {

Conversation

chwarwick commented Nov 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test plan

Uh oh!

sg-e2e-regression-test-bob commented Nov 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bundle size report 📦

Uh oh!

chwarwick commented Nov 16, 2022

Uh oh!

vovakulikov left a comment

Choose a reason for hiding this comment

Uh oh!

vovakulikov Nov 16, 2022

Choose a reason for hiding this comment

Uh oh!

vovakulikov Nov 16, 2022

Choose a reason for hiding this comment

Uh oh!

chwarwick Nov 16, 2022

Choose a reason for hiding this comment

Uh oh!

chwarwick Nov 16, 2022

Choose a reason for hiding this comment

Uh oh!

vovakulikov Nov 16, 2022

Choose a reason for hiding this comment

Uh oh!

chwarwick Nov 16, 2022

Choose a reason for hiding this comment

Uh oh!

felixfbecker commented Nov 21, 2022

Uh oh!

vovakulikov commented Nov 21, 2022

Uh oh!

felixfbecker commented Nov 21, 2022

Uh oh!

vovakulikov commented Nov 22, 2022

Uh oh!

chwarwick commented Nov 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

felixfbecker commented Nov 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

chwarwick commented Nov 16, 2022 •

edited

Loading

sg-e2e-regression-test-bob commented Nov 16, 2022 •

edited

Loading

chwarwick commented Nov 22, 2022 •

edited

Loading