Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Selecting top-most hierarchy value by default in filters #1018

Merged
merged 7 commits into from
Apr 19, 2023

Conversation

bprusinowski
Copy link
Collaborator

@bprusinowski bprusinowski commented Apr 6, 2023

Fixes #1017.

The main problem with #1016 was that I only updated the types in client.readQuery in configurator-state.tsx

DataCubeMetadataWithComponentValuesQueryDataCubeMetadataWithComponentValuesQueryVariables
DataCubeMetadataWithComponentValuesQueryVariablesDataCubeMetadataWithComponentValuesAndHierarchiesQueryVariables

but missed updating the document (DataCubeMetadataWithComponentValuesDocumentDataCubeMetadataWithComponentValuesAndHierarchiesDocument), so we were accessing dimensions without hierarchies in this place.

This PR fixes that (and also adds a unit test to make sure we select top-most hierarchy value by default in filters).

TODO

  • Remove DataCubeMetadataWithComponentValues & DataCubeMetadataWithComponentsValuesAndHierarchies in favor of using DataCubeMetadata & DimensionValues (so we can avoid fetching the dimension data twice in the editor mode in some cases)

@bprusinowski bprusinowski requested a review from ptbrowne as a code owner April 6, 2023 09:08
@vercel
Copy link

vercel bot commented Apr 6, 2023

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 14, 2023 8:58am

@bprusinowski bprusinowski changed the title Fix/load hierarchy where necessary fix: Selecting top-most hierarchy value by default in filters Apr 6, 2023
@Rdataflow
Copy link
Contributor

Rdataflow commented Apr 6, 2023

good to see the functionality is recovering - OTOH loading times for published charts double by this PR. 👁️
i.e
https://visualization-tool-nsubpeats-ixt1.vercel.app/v/y_66ZxOF-Mox?dataSource=Test ~5s
vs
https://test.visualize.admin.ch/v/G3Plp_XPARAI?dataSource=Test ~2s
image

why not for published charts rely on the list and ordering in the chartConfig?
i.e. the order stored in chartConfig.filters.$iri.values from the DB?

@bprusinowski
Copy link
Collaborator Author

Hi @Rdataflow, thank you for taking the time to do the testing 🙇‍♂️

I am fairly confident that there were no changes introduced in this pull request that could affect loading times of published charts or dataset preview, as there were only changes related to components used in the editing mode 👀

It's true that the data on TEST seems to loads faster, but – if we look at the preview deployment where the initial performance improvements were introduced (#1016, link to the same chart created using that deployment: https://visualization-tool-git-perf-published-chart-ixt1.vercel.app/en/v/4HVZl08NN58Y?dataSource=Test) it looks like loading time is comparable between the old preview deployment and the chart created by you using preview deployment from this PR.

I am not 100% sure, but my assumption is that it should work faster on the TEST environment, @ptbrowne do you see any potential explanations here?

PS. I also tested the performance manually by refreshing the pages with dataset preview for this PR (link) and for #1016 (link) and it looked quite random; sometimes this deployment was faster and sometimes it was the old deployment.

@Rdataflow
Copy link
Contributor

@ptbrowne in case you feel confident the observed bad performance is not related to the code you may go on and double check again after this PR is merged 👍 we just need to ensure there is no bad impact

@Rdataflow
Copy link
Contributor

performance on test as of now is updated in above comments

@ptbrowne
Copy link
Collaborator

LGTM, thanks Bartosz.

@bprusinowski bprusinowski merged commit 166c2c1 into main Apr 19, 2023
@bprusinowski bprusinowski deleted the fix/load-hierarchy-where-necessary branch April 19, 2023 06:53
@Rdataflow
Copy link
Contributor

@bprusinowski unfortunately https://test.visualize.admin.ch/de/browse?dataset=https%3A%2F%2Fenvironment.ld.admin.ch%2Ffoen%2Fnfi%2FJstat_fullHierarchy_C-94%2Fcube%2F2023-1&dataSource=Test now takes much longer than 2s previously, rather 4s+. can you check and fix this regression?

NB: the other examples (see above) look fine and had no impact 👍

cc @ZellerSabine

@bprusinowski
Copy link
Collaborator Author

Hi @Rdataflow, I used an old deployment preview coming from March 31 (#1005), before the changes in this PR (and any recent changes to data fetching) to check the loading times before / after this PR. It seems that it also used to take about 4s to load the dataset (see link).

Can you share more details on how you tested the regression in this example?

@Rdataflow
Copy link
Contributor

@bprusinowski I used what was deployed on TEST as of Apr 6 at 3PM...

@bprusinowski
Copy link
Collaborator Author

Hi, thanks for the info! Taking a look right now at TEST, it seems to load quick https://test.visualize.admin.ch/de/browse?dataset=https%3A%2F%2Fenvironment.ld.admin.ch%2Ffoen%2Fnfi%2FJstat_fullHierarchy_C-94%2Fcube%2F2023-1&dataSource=Test. Maybe the server was busy last time when testing? Is it also loading fast for you @Rdataflow now?

@adintegra
Copy link
Contributor

Thanks for looking into this @bprusinowski 🙇‍♂️ Agree that there may have been some transient issues upstream unrelated to the code changes in this PR. The shared URLs are responding fine for me as well.

@Rdataflow
Copy link
Contributor

@bprusinowski the cube switched to a new url https://test.visualize.admin.ch/de/browse?dataset=https%3A%2F%2Fenvironment.ld.admin.ch%2Ffoen%2Fnfi%2Fnfi_C-94%2Fcube%2F2023-1&dataSource=Test

the total page loading time is still the same (5s+)
this GraphQL query alone takes 4s typically while before none of the graphql took so long (see above)

 time curl -s 'https://test.visualize.admin.ch/api/graphql' -H 'x-visualize-cache-control: no-cache'   --data-raw $'{"query":"query DataCubePreview($iri: String\u0021, $sourceType: String\u0021, $sourceUrl: String\u0021, $locale: String\u0021, $latest: Boolean, $filters: Filters) {\\n  dataCubeByIri(\\n    iri: $iri\\n    sourceType: $sourceType\\n    sourceUrl: $sourceUrl\\n    locale: $locale\\n    latest: $latest\\n  ) {\\n    iri\\n    title\\n    description\\n    publicationStatus\\n    dimensions(sourceType: $sourceType, sourceUrl: $sourceUrl) {\\n      ...dimensionMetadata\\n      __typename\\n    }\\n    measures(sourceType: $sourceType, sourceUrl: $sourceUrl) {\\n      ...dimensionMetadata\\n      __typename\\n    }\\n    observations(sourceType: $sourceType, sourceUrl: $sourceUrl, limit: 10) {\\n      data\\n      sparql\\n      sparqlEditorUrl\\n      __typename\\n    }\\n    __typename\\n  }\\n}\\n\\nfragment dimensionMetadata on Dimension {\\n  iri\\n  label\\n  description\\n  isNumerical\\n  isKeyDimension\\n  dataType\\n  order\\n  values(sourceType: $sourceType, sourceUrl: $sourceUrl, filters: $filters)\\n  unit\\n  related {\\n    iri\\n    type\\n    __typename\\n  }\\n  ... on TemporalDimension {\\n    timeUnit\\n    timeFormat\\n    __typename\\n  }\\n  ... on NumericalMeasure {\\n    isCurrency\\n    currencyExponent\\n    resolution\\n    isDecimal\\n    __typename\\n  }\\n}\\n","operationName":"DataCubePreview","variables":{"iri":"https://environment.ld.admin.ch/foen/nfi/nfi_C-94/cube/2023-1","sourceType":"sparql","sourceUrl":"https://test.lindas.admin.ch/query","locale":"de"}}'   -o /dev/null

real 0m4.054s
user 0m0.015s
sys 0m0.046s

@adintegra can you repro page loading time of 2s - with serverside cache: disabled and share how to achieve that? I would be interested...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

regression: default topmost parent selection broken on TEST
4 participants