Skip to content

insights: Spike PR for graphql api changes

Warren Gifford requested to merge insights/api-chart-spike-2 into main

Created by: CristinaBirkel

Pertains to https://github.com/sourcegraph/sourcegraph/issues/33933 and referenced in RFC 690. Nothing here should be merged, and is for illustrative purposes and discussion only.

I broke this into 2 commits.

  1. Adds a mutation (meant to replace the existing mutation) CreateLineChartInsight. a. This decouples the line chart from the type of series it can hold. For now it can only hold search series, but we can add to that as needed. b. There's also a refactor in the resolvers to highlight this decoupling, where the CreateLineChartInsight resolver handles just the insight_view creation and then passes off to a more generic insight creation helper method.
  2. Adds a mutation for CreateBarChartInsight. a. This is meant to highlight what it might look like to add a new chart type. The set of allowed data series types (again, just search for now, but we can add others,) needs to be defined, but otherwise the series definitions remain separate.

I also added a PieChartDataSeriesInput (not being used anywhere) type to demonstrate more than one type of series on an insight. In this way we could support both langstats and search series on a pie chart without needing separate mutations.

Misc thoughts/concerns/questions:

  • Adding to an enum value is possible, but we can't then remove from it. (Unless I'm missing how to do that.) This might be a little tricky for the down migration.
  • How important is it that we enforce valid chart type <-> data generation type pairings in the graphql schema? This may be a bit of a pain to implement. Totally doable, but if it's not an important consideration it would be easier to just have one type with more fields than needed for any given generation method. I am leaning towards it being somewhat important actually, but I'm curious to hear what others think there.
  • I did not implement anything with the ordering right now. This is needed in order to maintain the ordering of the series definitions though, (when there are series of different types on the same chart.)
    • We could punt on this for now even, and just say that only one array of series can be filled in per insight. That's all we support on the webapp, and it could be something to add in later as needed. I think I would lean towards punting on this actually.
  • Did not do anything with updates in this PR. I suspect it will follow a very similar methodology though. Any concerns here?
  • What other options might there be for series presentation besides color and label? I'm leaning towards suggesting just one type here if we can get away with it. Otherwise this might need to be pulled out. I'm realizing this needs more thought.
  • I also didn't modify the query types at all, when adding bar charts. I think it should be straight forward, but will investigate that more.

Merge request reports

Loading