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

perf(api): batch APIs queries to request for maximum number of datapo… #310

Merged
merged 1 commit into from
May 6, 2024

Conversation

chejimmy
Copy link
Contributor

@chejimmy chejimmy commented May 4, 2024

…ints

What this PR does / why we need it:

Grafana has a default behavior to set query's maxDataPoints to a somewhat low number (compares to the SiteWise APIs maximum), and causes unnecessary pagination for queries with high response data points. It creates unnecessary latency and request (TPS) load to the server.

For example, for 6 hour time range - the maxDataPoints is auto set to ~500, and requires ~50 request for 6 hours of 1 second data points = 6hr60min/hr60sec/min / 432maxDataPoints/request.

This PR updates the batch APIs queries to request for maximum number of datapoints the SiteWise APIs can support for a better UX.

  • BatchGetAssetPropertyValueHistoryMaxResults of 20000
  • BatchGetAssetPropertyAggregatesMaxResults of 4000

Before this PR - GetPropertyValueHistory for 6 hr took ~10 seconds:

Screen.Recording.2024-05-04.at.1.19.07.AM.1080p.mov

After this PR - GetPropertyValueHistory for 6 hr took ~1.5 seconds:

Screen.Recording.2024-05-04.at.1.16.41.AM.mov

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

@chejimmy chejimmy marked this pull request as ready for review May 4, 2024 08:26
@chejimmy chejimmy requested a review from a team as a code owner May 4, 2024 08:26
@chejimmy chejimmy requested review from iwysiu and njvrzm and removed request for a team May 4, 2024 08:26
Copy link
Contributor

@iwysiu iwysiu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @chejimmy ! Overall looks good and makes sense to me, but I think you need to add "datapoints" to the words section in cspell.config.json to pass the spell check (and the ci). You can double check it locally by running yarn spellcheck

Copy link
Collaborator

@tracy-french tracy-french left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! This looks like a huge performance boost!

@chejimmy
Copy link
Contributor Author

chejimmy commented May 6, 2024

Hi @chejimmy ! Overall looks good and makes sense to me, but I think you need to add "datapoints" to the words section in cspell.config.json to pass the spell check (and the ci). You can double check it locally by running yarn spellcheck

I see; updated to data points instead to conform to the existing spelling 👍

@iwysiu iwysiu merged commit 174b7ea into grafana:main May 6, 2024
3 of 11 checks passed
@chejimmy chejimmy deleted the batch-api-performance branch May 6, 2024 22:42
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.

3 participants