perses icon indicating copy to clipboard operation
perses copied to clipboard

[FEATURE] Add Test button to datasource creation

Open sinkingpoint opened this issue 2 years ago • 5 comments

Description

This is the UI work for utilising the previously created Unsaved Datasources framework to be able to test datasources before they're saved.

There's a few components to this:

  • Work in the datasource provider to take two different types of selector. A "Saved" selector, which functions as previously - loading the datasource from the API and using that, and an "Unsaved" selector which generates datasource clients from the spec included in the selector.

  • This is the one I'm least sure about. A change to the datasource client api to allow us to handle both saved and unsaved datasources. Because unsaved datasources have more complicated semantics than unsaved ones (they need to package the spec, method, and request body into the body of the request), we need to be able to process the requests sent by datasource clients. To do this, we add in another argument - fetch to the query options of datasources. For saved datasources, again, this is the same. For unsaved ones, it's a wrapper around globals.fetch that allows us to intercept and repackage requests.

  • Adding the button and wiring it into the DatasourceStoreProvider

Screenshots

Global Datasources

https://github.com/perses/perses/assets/4386405/3ed021a9-3a08-4977-b7e2-7abcb5b17c99

Project Datasources

https://github.com/perses/perses/assets/4386405/829b740f-d4dd-4dd4-bb13-e46ae16816b7

Dashboard datasources ( + requests to saved datasources still working)

https://github.com/perses/perses/assets/4386405/83a7a293-6428-479d-b505-5b7b8a25b6dd

Checklist

  • [x] Pull request has a descriptive title and context useful to a reviewer.
  • [x] Pull request title follows the [<catalog_entry>] <commit message> naming convention using one of the following catalog_entry values: FEATURE, ENHANCEMENT, BUGFIX, BREAKINGCHANGE, DOC,IGNORE.
  • [x] All commits have DCO signoffs.

UI Changes

  • [x] Changes that impact the UI include screenshots and/or screencasts of the relevant changes.
  • [ ] Code follows the UI guidelines.
  • [ ] Visual tests are stable and unlikely to be flaky. See Storybook and e2e docs for more details. Common issues include:
    • Is the data inconsistent? You need to mock API requests.
    • Does the time change? You need to use consistent time values or mock time utilities.
    • Does it have loading states? You need to wait for loading to complete.

sinkingpoint avatar Dec 30 '23 21:12 sinkingpoint

For example I forgot why we need a custom fetch

To perhaps clarify a bit: Requests to saved datasources get proxied as is to the backing datasource, because we already have all the required metadata (datasource url etc) on the server side. This means that a request to a saved datasource is exactly as if you're fetching the datasource directly.

For unsaved datasources, we don't have that metadata already on the server side. So we need to package the spec up and send it along with the request. Because we have to send a body, this means that we have to use a POST (or at least, not a get), so we have to also include the requested method, and actual request body in the request body.

In order to make that transparent from the plugins point of view, I have a custom fetch wrapper that checks if we're calling an unsaved datasources and transparently wraps the request in the format that an unsaved datasource requires. That way, plugins can continue to make fetchs as usual and not worry about handling the case of saved vs unsaved.

Does that sort of make sense?

sinkingpoint avatar Jan 04 '24 04:01 sinkingpoint

@sjcobb I note that we already have a fetch wrapper of sorts in https://github.com/perses/perses/blob/main/ui/core/src/utils/fetch.ts - I could piggy back off that? WDYT?

sinkingpoint avatar Jan 04 '24 05:01 sinkingpoint

@sjcobb I note that we already have a fetch wrapper of sorts in https://github.com/perses/perses/blob/main/ui/core/src/utils/fetch.ts - I could piggy back off that? WDYT?

You need to consider that we already created a wrapper that handles the refresh of the token: https://github.com/perses/perses/blob/main/ui/app/src/model/fetch.ts .

Nexucis avatar Jan 08 '24 08:01 Nexucis

For example I forgot why we need a custom fetch

To perhaps clarify a bit: Requests to saved datasources get proxied as is to the backing datasource, because we already have all the required metadata (datasource url etc) on the server side. This means that a request to a saved datasource is exactly as if you're fetching the datasource directly.

For unsaved datasources, we don't have that metadata already on the server side. So we need to package the spec up and send it along with the request. Because we have to send a body, this means that we have to use a POST (or at least, not a get), so we have to also include the requested method, and actual request body in the request body.

In order to make that transparent from the plugins point of view, I have a custom fetch wrapper that checks if we're calling an unsaved datasources and transparently wraps the request in the format that an unsaved datasource requires. That way, plugins can continue to make fetchs as usual and not worry about handling the case of saved vs unsaved.

Does that sort of make sense?

Thank you for the explanations ! I was wondering too why we need to have another fetch wrapper.

Now I'm wondering if it is worth it. The whole point initially was just to test the connection to the datasource, not really to use an unsaved datasource to make a timeseries query. That's definitely interesting, I'm just worried it creates more difficulties to create a datasource plugin. But maybe I'm worried for nothing.

Nexucis avatar Jan 08 '24 08:01 Nexucis

I think there's still value here. Working within limiting it to just health checks, and piggy-backing off of https://github.com/perses/perses/blob/main/ui/app/src/model/fetch.ts I think I could put something together

sinkingpoint avatar Jan 08 '24 09:01 sinkingpoint