search

visualize-admin / visualization-tool

The tool for visualizing Swiss Open Government Data. Project ownership: Federal Office for the Environment FOEN
https://visualize.admin.ch
BSD 3-Clause "New" or "Revised" License
29 stars 3 forks source link

perf: Fire DataCubesObservations query as soon as possible #1464

Closed bprusinowski closed 3 months ago

bprusinowski commented 3 months ago

Closes #1423

This PR removes a need to pass dimensions to get query filters, as it looks like this behavior is not needed. It was used to filter out interactive data filters that are a part of dimensions, which should always be true (except when a dimension was completely removed from a cube, which would most probably break the chart anyway).

With this change we fire DataCubesObservations query in parallel to DataCubesComponents fetching, not waiting for it to finish first.

How to test

PR

  1. Go to this link.
  2. Open Developer Tools (e.g. see how to do so on Chrome).
  3. Switch to the Network tab.
  4. Reload the page.
  5. ✅ See that the graphql queries were fired (almost) at the same time.
Screenshot 2024-04-19 at 13 30 54

TEST

  1. Go to this link.
  2. Open Developer Tools (e.g. see how to do so on Chrome).
  3. Switch to the Network tab.
  4. Reload the page.
  5. ❌ See that some graphql queries were fired sequentially (waited for other queries to finish first).
Screenshot 2024-04-19 at 13 31 11
vercel[bot] commented 3 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
visualization-tool ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 19, 2024 2:07pm
ptbrowne commented 3 months ago

I see that you added this code in https://github.com/visualize-admin/visualization-tool/pull/1273/files, so I think you added that to only apply the filters to the correct cube in case of multi cubes. So I think you still need to know which cubes has which filters.

Maybe we should add the cube iri to each filter, so that this would be instant ? But then we'd have the problem of versioned/unversioned iri ? Maybe hear we need a query that returns only dimension iri and measure iri for a cube (without getting unit, values, description etcc...), that would not prevent the waterfall query though.

bprusinowski commented 3 months ago

Thanks for catching this @ptbrowne. I dug a little deeper and you're right that we still need to narrow down interactive data filters to a given cube – interestingly, this is not the case with current logic on the main branch (see that queryFilters contain mixed values from two cubes). I think it works before we check if a given dimension iri is a part of cube dimensions on the server side, inside the query.

Screenshot 2024-04-19 at 15 12 07

This is due to the fact that we simply pass dimensions which come from both cubes, and the logic in its current form would in fact always return all dimension unless one of them disappeared from a cube.

But to answer the need of querying dimensions to get this information, I think that's not the case, as interactive data filters are only enabled for filters from the left panel and we keep their IRIs in the chart config – so we can already narrow them down correctly per cube without filters (see https://github.com/visualize-admin/visualization-tool/pull/1464/commits/ae44d6b4eb03edade5d77515dedc74beff1a2f3e).

There still could be a problem if two cubes contain dimension with the same IRI, for it I think we should refactor interactive filters store to be cube-based. But this PR should already improve performance and fix wrong assignment of filters in most of the cases compared to actual implementation 🤞

Screenshot 2024-04-19 at 15 24 36
ptbrowne commented 3 months ago

Cool to have seen this request waterfall 🚀 🚀 Code looks good, thanks again for walking me through it :)

Rdataflow commented 3 months ago

@bprusinowski well done :rocket: :rocket: :rocket: a clear speed gain for published charts

bprusinowski commented 3 months ago

Thanks for catching this issue @Rdataflow 💯