Explore: Allow saving to Query Library from query row

[gelicia]

What is this feature?

Query library will be a way for anyone to save their queries in a central source that is assessable to the organization. For tis first pass, we are focusing on making it available in Explore and in a separate app, but it will eventually be implemented anywhere in core grafana where people save queries.

We want the ability to save queries from the query row, as that would be the most assessable place for the user who is writing the query. This PR attempts to balance this implementation with the lightest touch on core components.

Dashboards - there is a change to a core component, to add a Map that holds QueryActionComponents alongside the existing array that stores them. This is because the current only use of this is in Enterprise, to add the recorded queries button. The function that adds this action is fired once when the app is initialized. For core Grafana, we tend to prefer things for a specific part of Grafana to be isolated in that code, so I moved similar logic inside ExplorePage, the top level component for Explore. However, if you leave Explore, then return, it would fire the logic to add the component again, and result in redundant actions. The newly added map for these actions makes sure the action is rewritten instead of concatenated into the array, so we do not get redundant actions.

I could see this having more use if other areas inside of core Grafana want to add actions.

Why do we need this feature?

To continue to build out query library.

Which issue(s) does this PR fix?:

Fixes #84902

Special notes for your reviewer:

Please check that:

• It works as expected from a user's perspective.
• If this is a pre-GA feature, it is behind a feature toggle.
• The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

[gelicia]

This does add one modal per query, but the previous method had one modal per "add to library" button. Should this simply be in Explore and be controled via state?

[aocenas]

If I understand it right now, the button to add to QL is inside the QueryEditorRows component and it controls state of the parent via a prop function that then triggers the modal here, right? Is there a benefit in that? Making the modal part of the QL save button seems like it would package this more nicely, so that any need to move it around would be easier, and also you would see all the logic in one place. Is there any need for the QueryRows component to know about the QL save button?

[gelicia]

Nope, I was just wondering if we would have a preference to do it another way - seems how I have it is good :D

[gelicia]

Actually with some changes in how we need to initiate the extra query action, it made sense to move everything up to the main Explore page. We still don't have extra stuff in the app state though, we can control it all in the component state.

[torkelo]

Is there be some indication that query is already in library?

[gelicia]

Not at the moment. We have the ability to save to query library from query history in Explore, but all it does is immediately hide the "save to query library" button after the save goes through in that component's state without persistence. If that's good enough we can absolutely do that here, but I don't know a good, scalable way to know one query is the same as one that already exists in the library. The query text isn't guaranteed to save properties in the same order, and searching the library off the query text would get really slow. There is a key for a query run but running the same query multiple times results in a different key every time, I believe.

Is there something I'm missing?

[torkelo]

confused, so the saved query is not named or referenced by some name/id? or is that coming later?

[gelicia]

We have IDs for the queries that are saved in the query library. So if a query library entry exists for SELECT * FROM table , but then a couple weeks later someone goes into the same datasource and writes SELECT * FROM table, there is no great way to know that query is equivalent to one that is already saved. We can have something in app state where the button for that query row disappears immediately after saving, but the button would come back on refresh.

[ifrost]

Adding works nicely 👍 I've tested with and without the toggle, and checked that the button doesn't show up in Dashboards yet.

In Figma it was designed with a blue "new" button, and the form opens as a drawer. Are we okay with these changes? (cc @diegoadams). To me using a modal makes sense as it would probably look good outside of Explore. For the "new" button, I wonder for how long we should display it as highlighted - until the first usage?

[diegoadams]

To me using a modal makes sense as it would probably look good outside of Explore.

@ifrost Using the modal is also fine. The drawer was intended to keep the context of the current query being saved, but since all the query information is repeated anyway, it’s okay to use the modal.

[diegoadams]

For the "new" button, I wonder for how long we should display it as highlighted

@ifrost The highlighted blue should remain long enough for users to notice and understand the new saving feature. Generally, keeping it for the first 2-3 interactions should be effective.

fwd.mov

[gelicia]

@ifrost In standup I mentioned seeing scaffolding for being able to add in action buttons without needing to modify the core component. On a second look, I realized this exists for Recorded Queries, and it is only in use in Grafana Enterprise (which I forgot to check the first time). That works super well here and can be implemented as needed until we are ready for Query Library to be a real core component of Grafana.

@aocenas - You mentioned this being a useful extension point for the Sidecar project - the good news there is it already fully works without issue.

[ifrost]

This will cause adding new entries every time the component is rendered.

Screen.Recording.2024-07-11.at.12.51.23.mov

[gelicia]

Thank you for this, sorry I missed it. I checked out how Enterprise does it, and they just have an init function go off when you start the app that adds the action. See initRecordedQueries inside the Enterprise repository for more information.

Unfortunately, there's not an easy equivalent, because I still would like to encapsulate this inside Explore as much as possible. I moved the logic up to ExplorePage, the top level component for Explore, but it still would create redundant actions when you left Explore and came back. I decided to improve on the QueryActionComponent paradigm by adding another variable, a map, that you could set actions to. This way when Explore re-loads, it will re-add the action, but only by replacing the action with the same key. This is done alongside the existing array for actions, so it will not impact Enterprise and recorded queries. It only fires once whenever you enter Explore after navigating out of Explore.

As it is without this change, you can get the list of actions and iterate through it, but it's a list of JSX elements - I couldn't find an easy way to establish uniqueness on that list.

It does mean that this PR is no longer isolated to just explore, but I think the keyed list of actions will have a lot of use if we build out extensions on query rows like we talked a bit about. Having an init function fire once on app load can work well for Enterprise, but for more complicated cases inside core Grafana, we should let components that may load more than once be responsible for adding query actions without the risk of redundancy.

Lmk if you think this is a decent thing to add and I can ping Dashboards about it (unless someone sees it without the ping)

[ifrost]

One problem is that the save button will show up in all query rows (e.g. in Dashboards), but only after loading Explore page and will not do antyhing. Example: If I start with a dashboard - no button shows up. I go to Explore - button shows up and works. I go back to a dashbaord - the button shows up but does nothing.

Screen.Recording.2024-07-12.at.10.59.14.mov

I think we should scope it down so the button is shown only in Explore. Once we expand it to other places we should use AppPlugin Extension and have the save modal being provided by the app (if installed).

One way of scoping it down would be to unregister the item on unmount, but definitely check the API with dashboards. It feels a bit hacky that there's a global singleton to inject additional options.

[gelicia]

It feels a bit hacky that there's a global singleton to inject additional options.

Unfortunately, theres not a lot of other options..

I really do like using QueryActionComponent for this and would like to continue this direction, because we are able to keep the add to QueryEditorRow to where it is relevant. I added the concept of scopes to the Map as well, so the QueryEditorRow will only add in the actions relevant to the app that is passed in. We're getting a little deep into adding stuff to this aspect of Grafana, but I think it's highly likely if other parts of core Grafana would want to add actions, they would run into the same problems needing scopes and uniqueness.

Let me know what you think about all this, and if youre okay with it, I can move forward with asking dashboards for their blessing

[ifrost]

Explore changes LGTM 👍 We'll need to rethink how to register query row action when it becomes available in other places but we'll cross that bridge.

[harisrozajac]

works like a charm 🚀

[axelavargas]

Hey @gelicia 👋🏾, I took a look at the code relevant to dashboards squad, and it looks good to me 🙌🏾 .

I also gave it a quick test locally and the only thing I found was the issue of not being able to save when the description field contained spaces(the video I shared in the slack thread).

I know you are working on a fix, so I am approving the PR :)

[gelicia]

only thing I found was the issue of not being able to save when the description field contained spaces

Ah, just to have this documented somewhere - we put the description in a field called generateName which looks like was changed to not allow spaces. I did an easy space to dash swap for that so we can move forward, but will evaluate with @ifrost if that's the solution we want to be long term. Thank you for the review!