-
Notifications
You must be signed in to change notification settings - Fork 11
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
Patterns: Use runtime datasource for queries #672
Patterns: Use runtime datasource for queries #672
Conversation
…0/patterns-runtime-resource-call
…0/patterns-runtime-resource-call
getQueryRunner( | ||
buildDataQuery(getTimeSeriesExpr(this, optionValue), { legendFormat: `{{${optionValue}}}` }) | ||
) | ||
getQueryRunner([ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need multiple queries anymore, and I would highly suggest we don't mix queries that call different API endpoints, but if we wanted to include multiple queries that call the same API endpoint this should work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first glance, I like this ability of running more than one query. What was the original purpose of this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initially I tried to smoosh all the queries into $data, which was a bad idea as queries would end at different times, but if we have multiple data queries, that should work fine as they are all returned from the backend at the same time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense for "combined panels", i.e. the ones at the service selection scene, where we do one logs and one metrics query for each service 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, that's probably worth opening in another issue. Might be good for the lazy-loaded panels as well to at least batch things up a bit.
src/Components/ServiceScene/Breakdowns/Patterns/PatternsBreakdownScene.tsx
Outdated
Show resolved
Hide resolved
I haven't tested this yet, so I plan to do a more in-depth review tomorrow, but took a good look at the changes and didn't find anything that stands out, aka, looks pretty good 👌 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great, works very good. Left a couple of comments, but nothing too big. Good job!
getQueryRunner( | ||
buildDataQuery(getTimeSeriesExpr(this, optionValue), { legendFormat: `{{${optionValue}}}` }) | ||
) | ||
getQueryRunner([ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense for "combined panels", i.e. the ones at the service selection scene, where we do one logs and one metrics query for each service 👍
src/Components/ServiceScene/Breakdowns/Patterns/PatternsBreakdownScene.tsx
Outdated
Show resolved
Hide resolved
src/Components/ServiceScene/Breakdowns/Patterns/PatternsBreakdownScene.tsx
Outdated
Show resolved
Hide resolved
src/Components/ServiceScene/Breakdowns/Patterns/PatternsBreakdownScene.tsx
Outdated
Show resolved
Hide resolved
src/Components/ServiceScene/Breakdowns/PatternsLogsSampleScene.tsx
Outdated
Show resolved
Hide resolved
So far it works very well, but I found this case where the query is not re-run after removing the last filter. Can consistently reproduce with any field. Test.mov |
I can no longer reproduce ☝️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To the moon 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Refactoring the patterns call to use runtime datasource.
Fixes: #670
This is a pretty big refactor, but the end goal should be that we don't re-query the patterns on every route change, just when:
Ok, I've tried a few different approaches with this, and while this "requires" (not really required, but it would be good to avoid the hack to call a private method) a scenes core change (grafana/scenes#866), I think this is the cleanest approach so far.
Some history: Initially I saw that the special $data provider didn't allow multiple query runners, but did allow multiple queries, so I tried stuffing the patterns resource query in with the typical data api call. But this broke a bunch of assumptions that scenes has about the data being returned by a query runner, primarily we had multiple queries that called different api endpoints, so the results would come in at different times. I could work around this by holding the "done" state until all queries had resolved, but then if you had one query taking longer then the other, you'd either need to change the assumption that you wait for a query state to be "done" before doing stuff with the response, but this would cause unnecessary re-rendering in panelBuilders that directly consumed data providers, as sometimes the dataframes would be undefined when it was actually loading.
Anyway, that was a terrible idea.
So instead of trying to use $data, in this current attempt we're just adding more custom data providers to the state of the ServiceScene
$patternsData
. This works much better, but if I activated the query provider in the serviceScene we'd still get unnecessary executions of the patterns call, for example on every route change. But we just want to run the patterns call on specific route activations, or when the time range changes, or the labels variable has been updated, or the user is entering the application for the first time. But we can't move the data provider down the scene tree as we expose the count of the patterns in the tabs which are visible across all routes.So currently we're never activating the patternsData query runner, but we call runQueries in the specific cases where we want it to update. This works fine except we need to extend the
SceneQueryRunner
class and preventrunQueries
from callingsubscribeToTimeRangeChanges
, as this will make the exact same request twice (but only the first time a scene is activated).This should give us the desired functionality to only request the patterns when we need it, instead of on every route change that has ServiceScene as a parent (which is the entire application, except the service selection scene), and still take advantage of query caching caching within scenes.