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

WaitingForPanels: Change waiting logic for Scenes #496

Merged
merged 3 commits into from Feb 16, 2024

Conversation

torkelo
Copy link
Member

@torkelo torkelo commented Feb 11, 2024

This improves and simplifies the waiting for panel queries and panel rendering logic.

Instead of scanning for how many panels there are using css selectors and comparing this to panel content (which is a bit unreliable / fragile) we just wait for all network requests to be completed and then an additional 100ms for data rendering (this step should be blocking so maybe we can lower this).

  • Now the waiting for panels step also works in scenes
  • Tested with single panels
  • Tested with full dashboard images (using height=-1) (With many slow queries)
  • Tested with new and angular panels

Copy link
Contributor

@AgnesToulet AgnesToulet left a comment

Choose a reason for hiding this comment

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

I think this doesn't cover async queries (I discovered them recently and I'd say this is the only reason why we need this complicated code). An example of data source using them is the Athena DS.

As the new PDF feature relying on scenes is working though, maybe it would be possible to use only the full page condition for everything.

},
options.fullPageImage || isPDF
);
await page.waitForNetworkIdle({ timeout: options.timeout * 1000 });
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is the same as what we do here:

return page.goto(options.url, { waitUntil: 'networkidle0', timeout: options.timeout * 1000 });

Copy link
Member Author

@torkelo torkelo Feb 12, 2024

Choose a reason for hiding this comment

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

@AgnesToulet not 100% sure this is the same, that page.goto is just for the initial page load, before we scroll down and trigger more queries. That is why I do a second call to waitForNetworkIdle here as this is after the scroll

@torkelo
Copy link
Member Author

torkelo commented Feb 12, 2024

I think this doesn't cover async queries (I discovered them recently and I'd say this is the only reason why we need this complicated code). An example of data source using them is the Athena DS.

@AgnesToulet All queries are async, what is special about the Athena DS?

@AgnesToulet
Copy link
Contributor

@AgnesToulet All queries are async, what is special about the Athena DS?

When you use the TestData data source with the "Slow query" scenario, you can see the query request in a "pending" state for a while and returning after a specific time. This is caught by waitForNetworkIdle because it's waiting for all pending requests to end.

With the Athena DS, a first query call is made, but it returns pretty quickly. Then, when the results are ready, another query call is made to retrieve them. If the time between these two requests is greater than the idleTime option, waitForNetworkIdle will return before this second request is made.

@torkelo
Copy link
Member Author

torkelo commented Feb 12, 2024

I see

Then, when the results are ready, another query call is made to retrieve

@AgnesToulet how does know when the results are ready?

@torkelo
Copy link
Member Author

torkelo commented Feb 12, 2024

@AgnesToulet is this some closed source athena data source? this ones does nothing special https://github.com/grafana/athena-datasource/blob/main/src/datasource.ts#L110C18-L110C23

@torkelo
Copy link
Member Author

torkelo commented Feb 12, 2024

@torkelo
Copy link
Member Author

torkelo commented Feb 12, 2024

@AgnesToulet I see this, https://github.com/grafana/grafana-async-query-data-js/blob/main/src/requestLooper.ts#L79

hm... dammit. why is there always some stupid edge case ruining everything :)

I think I have a way fix it but will require a change in main repo

Comment on lines +570 to +572
if (window.__grafanaSceneContext) {
return window.__grafanaRunningQueryCount === 0;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

@AgnesToulet pushed an update that restores the old waiting logic, except for scenes it will use this window query counter (That is not related to network query state but actual data source requests observable state and responses). So for Athena this will work as they are returning observable loading states (And their observable does not complete until they they are done)

Copy link
Contributor

Choose a reason for hiding this comment

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

@torkelo This looks good but doesn't support the TestData slow query scenario. I get this screenshot:
image

Is it expected? Does it work with slow queries from other DS?

Copy link
Member Author

Choose a reason for hiding this comment

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

@AgnesToulet it works for every slow query I have tested for any data source, in both new and old dashboard arch.

What version of Grafana are you using and what is the url? is the feature toggle __feature.dashboardSceneSolo enabled (or in url)?

Copy link
Contributor

Choose a reason for hiding this comment

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

@torkelo I'm on latest main, I have the dashboardSceneSolo feature toggle enabled, and it happens when rendering a panel from a Scene dashboard. The URL is this one: http://localhost:3000/render/d-solo/b17337d4-f4b7-40e2-b1a0-d76deee4d6f5?orgId=1&scenes&var-myVar=b&from=2024-02-16T07:33:09.973Z&to=2024-02-16T13:33:09.973Z&panelId=panel-11-clone-0&__feature.dashboardSceneSolo&width=1000&height=500&tz=Europe%2FParis

What's weird is that I see the same behavior with the current version of the image renderer, while all other panels (also using the TestData DS but with the Random Walk scenario) are timing out...

Copy link
Contributor

Choose a reason for hiding this comment

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

@torkelo I tried to access the associated /d-solo page, and I'd say the issue is from there. The panel using the TestData DS with the slow query scenario always shows the loading page (the query is supposed to run for 15 seconds, but even after that time, I still see the loading page), and I don't see any query pending.

So, I will approve the PR and will work on releasing it along with the PDF option next week.

Copy link
Member Author

Choose a reason for hiding this comment

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

@AgnesToulet strange, d-solo route works well for me no matter the panel or query

Copy link
Member Author

Choose a reason for hiding this comment

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

@AgnesToulet you you using latest main?

Copy link
Contributor

Choose a reason for hiding this comment

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

@torkelo Actually, I tried again with a simple slow panel, and it's working. The panel that doesn't work is a repeating panel (using a custom variable).

Copy link
Member Author

Choose a reason for hiding this comment

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

@torkelo for repeated panels you need use a new syntax for the panelId , example panel-2-clone-2 , it's one of the few breaking changes in scenes

@torkelo
Copy link
Member Author

torkelo commented Feb 14, 2024

@AgnesToulet
after grafana/grafana#82441 is merged this should now work for old and scenes dashboards & panels

Copy link
Contributor

@AgnesToulet AgnesToulet left a comment

Choose a reason for hiding this comment

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

LGTM!

@torkelo torkelo merged commit 6a876f4 into master Feb 16, 2024
4 checks passed
@torkelo torkelo deleted the fix-waiting-for-panels branch February 16, 2024 14:14
@AgnesToulet AgnesToulet changed the title WaitingForPanels: Change waiting logic WaitingForPanels: Change waiting logic to support Scenes Feb 20, 2024
@AgnesToulet AgnesToulet changed the title WaitingForPanels: Change waiting logic to support Scenes WaitingForPanels: Change waiting logic for Scenes Feb 20, 2024
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.

None yet

2 participants