grafana / iot-sitewise-datasource

IoT Sitewise
Apache License 2.0
18 stars 9 forks source link

chore: added cache to store/refresh relative range queries. #318

Closed chejimmy closed 3 months ago

chejimmy commented 4 months ago

What this PR does / why we need it:

Loading a huge amount of data takes a lot of time and also, if the dashboard is set to auto refresh then most of the times the dashboard refreshes while the data is still getting loaded. This is not a good user experience and makes the dashboard unusable.

This PR added a cache to the DataSource.ts on the frontend. The cache RelativeRangeCache.ts tracks a history of the relative range requests and their data frame responses. This cache allows the DataSource to reuse the cached data frame responses and refreshes only the most recent data, reducing the request time range; hence, bringing a snappy refresh experience for customers.

After this PR:

https://github.com/grafana/iot-sitewise-datasource/assets/50635800/9998ad8e-2296-47da-8e59-6085247e705d

Before this PR:

https://github.com/grafana/iot-sitewise-datasource/assets/50635800/35c8adb5-95a7-4f6d-b9a0-15f68f7ed4d6

Which issue(s) this PR fixes: N/A

Fixes https://github.com/grafana/iot-sitewise-datasource/issues/321

Special notes for your reviewer:

chejimmy commented 3 months ago

Great work! A couple of questions:

  • Do we need a way to toggle this feature on/off? For example would it make sense to add a toggle to the UI in someway for turning off the cache? I suppose they could always refresh the page? I'm not sure if it's a common usecase for example for queries results to change over time. It looks like we always refresh data if it's within the last 15 minutes and I assume it is for this reason, that maybe the latest data might come in with a delay and may not be up to date. But presumably 15 minutes may not work for all users? Could be nice to have a toggle in the query editor to turn it off or make the amount of time configurable? What do you think?
  • Just a reminder this won't do anything for alerting, though I could see it improving user's experiences of viewing the dashboard. Have you looked into using grafana's enterprise query caching feature? It may also help a bit for situations where multiple people are making the same request at the same time in multiple windows but I don't know that it caches relative time ranges like this, so it makes sense to me that this could be added in addition: https://grafana.com/blog/2021/09/02/reduce-costs-and-increase-performance-with-query-caching-in-grafana-cloud/ just wanted to make sure it was on your radar as well.
  1. That's a good callout. I think customer would like to have this control šŸ‘ I will add a toggle so customer can have the option to turn off the cache if it doesn't work for their usecases.
  2. We are aware of the limitations. However, we just want this to serve as a stopgap until SiteWise service have a better way to deliver data to the clients šŸ˜‰

getting a bit late here, I will read the in code comments tmrw; TY for reviewing the code šŸ™

chejimmy commented 3 months ago

Added a toggle so customer can have the option to turn off the cache.

Demo (notice the second time refresh with cache disabled takes significantly longer):

https://github.com/grafana/iot-sitewise-datasource/assets/50635800/a703d205-dae4-44da-a627-471df9960655

sarahzinger commented 3 months ago

oh also i think the e2e tests are failing because we don't run them on forks if I remember correctly... I ran them here and they pass.

However QueryEditor ā€ŗ QueryEditor with awsDatasourcesNewFormStyling feature toggle disabled ā€ŗ should display correct fields for query type PropertyValueHistory does not pass

chejimmy commented 3 months ago

I like it with the toggle! One more suggestion on wording (assuming you think it's accurate!)

Also what's the reason again to do this only for relative ranges? I suppose we could start with a relative range and then expand for all time ranges later if it seems appropriate. Then again I wonder if non-relative ranges might clash more with our enterprise query caching feature.

We are working on this in an incremental manner to keep the logic simple. We will consider expanding the feature to all time ranges in the future šŸ™

chejimmy commented 3 months ago

oh also i think the e2e tests are failing because we don't run them on forks if I remember correctly... I ran them here and they pass.

However QueryEditor ā€ŗ QueryEditor with awsDatasourcesNewFormStyling feature toggle disabled ā€ŗ should display correct fields for query type PropertyValueHistory does not pass

I think it was a silly mistake I added multiple (2) CacheRows šŸ˜…; should be fixed in fix: removed duplicate clientCacheRow from QueryEditor

chejimmy commented 3 months ago

Looks good! Just as a heads up I created an issue related to this, in the future if you want to make sure you create an issue for each pr it helps with our tracking! Thanks!

Thanks for contributing šŸ†

:pray: TY for the review and helping along the journey!