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

Draft: add apiVersion to datasource ref #87961

Closed
wants to merge 3 commits into from
Closed

Conversation

andresmgot
Copy link
Contributor

@andresmgot andresmgot commented May 16, 2024

What is this feature?

Adds the plugin (and data source) apiVersion to a query DatasourceRef and in DatasourceSettings so panels and alerts (among other requests), include a reference to the target apiVersion.

Query:

Screenshot from 2024-05-16 13-32-50

Alert:

Screenshot from 2024-05-16 13-35-20

It's still not clear to me if we should add this field elsewhere for example, in the DataQuery object. If the apiVersion is not in the query, the plugin won't receive this information in the request (req.PluginContext.DatasourceSettings is populated with the value in storage, not with the DatasourceRef from the query) so it won't work out of the box for example for running query migrations.

TODO:

  • Clean up PR
  • (won't do this in this PR) Add apiVersion when saving a data source

Why do we need this feature?

We can add now a validation step that checks that for a given query, the datasource referenced matches in apiVersion with that it's stored.

Who is this feature for?

Which issue(s) does this PR fix?:

Fixes #

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.

@grafana-delivery-bot grafana-delivery-bot bot added this to the 11.1.x milestone May 16, 2024
@andresmgot
Copy link
Contributor Author

@marefr @ryantxu @wbrowne pinging you here for an early review to make sure we are on the same page. Please take a look to the description for more details.

@marefr
Copy link
Member

marefr commented May 16, 2024

Think this is going in the direction I was expecting, but might have been outside the loop recently.

It's still not clear to me if we should add this field elsewhere for example, in the DataQuery object.

Based on plugin context is expected to have the request api version, https://github.com/grafana/grafana-plugin-sdk-go/blob/36b8fe32821b1740d9fb32a6472287058aee5304/proto/backend.proto#L86, that dictates that all the data queries should use the same api version and hence the datasource ref api version would need to be extracted and put into pluginContext.ApiVersion I guess?

@andresmgot
Copy link
Contributor Author

Based on plugin context is expected to have the request api version, https://github.com/grafana/grafana-plugin-sdk-go/blob/36b8fe32821b1740d9fb32a6472287058aee5304/proto/backend.proto#L86, that dictates that all the data queries should use the same api version and hence the datasource ref api version would need to be extracted and put into pluginContext.ApiVersion I guess?

This could be a bit confusing, but the apiVersion means different things depending on where it's set:

  • The request pluginContext.APIVersion is the apiVersion currently running in the backend (Grafana / DS API Server)
  • The apiVersion of the datasource reference is the apiVersion that was running when the query was saved.
  • The apiVersion of the datasource config (settings) is the apiVersion that was running when the data source was saved.

Because of that, I think plugins should have access to both the pluginContext API version and the query API version, so in case of a mistmatch, it could potentially run migration code based on that field. Right now, the datasource ref is only used in the Grafana backend to retrieve the proper datasource info and it's that info what's forwarded to the plugin (the original datasource ref gets lost).

@marefr
Copy link
Member

marefr commented May 16, 2024

Yes, its very confusing 😄

  • The request pluginContext.APIVersion is the apiVersion currently running in the backend (Grafana / DS API Server)

Not sure that make sense. Shouldn't the frontend be able to decide which api version to use?

In the context of a datasource API server I would expect the version being used when hitting the API will populate the pluginContext.APIVersion.

@andresmgot
Copy link
Contributor Author

andresmgot commented May 16, 2024

Not sure that make sense. Shouldn't the frontend be able to decide which api version to use?

Correct, in the future, the api version will be part of the URL (e.g. /apis/testdata.datasource.grafana.app/v0alpha1) but in this PR, the way to "choose" an API version is setting it in the DatasourceRef. It's up to the backend to process the query or reject it (right now there is only one api version running in the backend so it won't be able to process it if the version doesn't match).

In the context of a datasource API server I would expect the version being used when hitting the API will populate the pluginContext.APIVersion.

As I understand it, it's not the same. For example, the API server may be able to accept multiple version (e.g. v0alpha1, v0alpha2). In that context, pluginContext.APIVersion would be the latest one (v0alpha2) but would still be able to receive requests for v0alpha1 and migrate those. In any case, this PR doesn't make any decission about this.

addQuery(queries, query, {
type: dsSettings?.type,
uid: dsSettings?.uid,
apiVersion: dsSettings?.meta.apiVersion,
Copy link
Member

Choose a reason for hiding this comment

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

hymm -- I though we were not going to add this to the swagger yet?

I don't think we want that form locked in yet

@@ -16,7 +16,7 @@ import {
* @public
*/
export function getDataSourceRef(ds: DataSourceInstanceSettings): DataSourceRef {
return { uid: ds.uid, type: ds.type };
return { uid: ds.uid, type: ds.type, apiVersion: ds.meta?.apiVersion };
Copy link
Member

Choose a reason for hiding this comment

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

Hymm -- do we we want to set this automaticly yet? I see two big issues:

  1. this assumes frontend and backend are in sync
  2. This does not let the frontend say what apiVersion is is actually supporting

@ryantxu
Copy link
Member

ryantxu commented May 20, 2024

I think this is generally fine with the big exception that I do not think we are ready to have it automatically filled up from the settings. If a frontend sets it that is fine, but lets not depend on settings.meta.apiVersion -- since that is just is a placeholder now and needs to be updated. Also it seems like the frontend should explicitly know what it is writing if that is the intention of this setting.

See also: grafana/grafana-plugin-sdk-go#991

@andresmgot
Copy link
Contributor Author

If a frontend sets it that is fine, but lets not depend on settings.meta.apiVersion -- since that is just is a placeholder now and needs to be updated

Okay, let's block this until #88041 is solved

Also it seems like the frontend should explicitly know what it is writing if that is the intention of this setting

The question is where should the frontend get this from if not from the datasource settings?

@ryantxu
Copy link
Member

ryantxu commented May 20, 2024

The question is where should the frontend get this from if not from the datasource settings?

IIUC, it is the frontend client that should know what api it is using -- so it would be in the client.

@andresmgot
Copy link
Contributor Author

IIUC, it is the frontend client that should know what api it is using -- so it would be in the client.

yes but I mean the client, in this case Grafana UI should get the version from somewhere. Would that be the current value in the plugin.json? If that's the case, maybe we should not encode the apiVersion in the DatasourceRef but in the DataQuery itself.

@marefr
Copy link
Member

marefr commented May 21, 2024

IIUC, it is the frontend client that should know what api it is using -- so it would be in the client.

yes but I mean the client, in this case Grafana UI should get the version from somewhere. Would that be the current value in the plugin.json? If that's the case, maybe we should not encode the apiVersion in the DatasourceRef but in the DataQuery itself.

@andresmgot I think I understand where you're going/how you're thinking. Let me see if this is correctly understood. So when a user selects a datasource to make a query for either explore, alerting or dashboards the datasource ref is supposed to automatically populate the panel datasource prop, right? Is one potential problem for using the datasource ref that the datasource don't have any dynamic way to influence the api version picked besides currently putting it into plugin.json? That's why you suggest putting it in data query instead because the query editor can control such prop?

@andresmgot
Copy link
Contributor Author

So when a user selects a datasource to make a query for either explore, alerting or dashboards the datasource ref is supposed to automatically populate the panel datasource prop, right?

Correct.

Is one potential problem for using the datasource ref that the datasource don't have any dynamic way to influence the api version picked besides currently putting it into plugin.json? That's why you suggest putting it in data query instead because the query editor can control such prop?

First of all, I am thinking from the assumption that the final user won't need to care about the apiVersion that the plugin is using and that this info will be transparent to them so the client (in this case the Grafana UI), will need to make automatic decissions about how to handle apiVersions.

My confusion (and correct me if I am wrong here @ryantxu), comes from the fact that an API server directly owns data source configs but also queries. In other words, a specific apiVersion is tied to two different schemas: the data source config and the query.

From that point, what I am questioning is if a query should be tied to an apiVersion because it's using a data source with a certain apiVersion or because it's using a plugin with a certain apiVersion.

For the former, imagine a query that is using a datasource "A". At the moment of creating the datasource, the API server is using the version "v0alpha1" (this version is read from the plugin.json). Therefore, when saving the query, it will contain: {datasourceRef: {uid: A, apiVersion: v0alpha1}}. If later, the API server gets upgraded to v0alpha2, the datasource may be automatically migrated to the new version. From that point, the query will still refer to the version v0alpha1, which is not the version running in the server. In this situation, we'd need to either: fail in the server if it doesn't support v0alpha1 or migrate the query too to the new apiVersion.

For the later, the query would store the apiVersion that the plugin (backend) is running at the time of writing the query, regardless of the apiVersion of the datasource (which should match anyway, at least at the moment of writing the query): {datasourceRef: {uid: A}, apiVersion: v0alpha1}. If later on the version gets upgraded in the server, we are in the same situation, the query will need to be either rejected or migrated.

So in the end, I guess the question is, does an apiVersion also refers to a specific version of a query schema or not. If it does, I think it makes sense to store that information in the "root" of the query. If not and the relation between an apiVersion and a query is indirect (query -> datasource -> apiVersion), then we are good storing just the apiVersion in the datasourceRef.

Hope all that long comment makes sense 😞

@ryantxu
Copy link
Member

ryantxu commented May 22, 2024

does an apiVersion also refers to a specific version of a query schema or not.

Yes -- though not really a schema -- it points to the server that can answer that query payload. The server can (and eventually should!) have a schema that can help validate the query target.


If later, the API server gets upgraded to v0alpha2, the datasource may be automatically migrated to the new version. From that point, the query will still refer to the version v0alpha1, which is not the version running in the server.

I think this comment points to the biggest confusion -- every apiVersion that we publish should keep working until we explicitly want to break or stop supporting only APIs. When you publish "v2", something must still be running at "v1" that continues to support the payloads that were valid at that time -- behind the scenes that will likely convert, but it could just be an old version of the service that continues to run.

This may help explain why I was so confused by using semantic version for the apiVersion!

@andresmgot
Copy link
Contributor Author

andresmgot commented May 22, 2024

I think this comment points to the biggest confusion -- every apiVersion that we publish should keep working until we explicitly want to break or stop supporting only APIs. When you publish "v2", something must still be running at "v1" that continues to support the payloads that were valid at that time -- behind the scenes that will likely convert, but it could just be an old version of the service that continues to run.

Got it, I think the conversation diverted a bit from the original question.

Let's go back a bit. I want to save a query for an API server that is running v1 with a data source that was saved with a reference to v1. From your comment:

but lets not depend on settings.meta.apiVersion -- since that is just is a placeholder now and needs to be updated

Is your concern about the format (e.g. settings.meta.apiServer.preferredVersion would be fine?) or because the query should not automatically add a reference to the data source settings api version? If the second, then we would need to store the apiVersion in the query body (and read it from the plugin.json), not in its datasourceRef.

My point is that the client, in this case the Grafana UI, needs to set the apiVersion of the query somewhere and read it before saving from somewhere too. Either it gets it from the data source metadata (and save it in the data source ref) or from the plugin metadata (and stores it in the query model). Or maybe you are thinking on a third option I am not seeing.

@ryantxu
Copy link
Member

ryantxu commented May 22, 2024

Now I see the key difference:

My point is that the client, in this case the Grafana UI, needs to set the apiVersion of the query somewhere and read it before saving from somewhere too

The client should know what apiVersion it is targeting -- it should not ask something else what its version is and then blindly use it. This is why I am fine if we just remove apiVersion from the (legacy) /api endpoints

When introducing new apiVersions, the target will likely want/need to support multiple versions at the same time.

@ryantxu
Copy link
Member

ryantxu commented May 22, 2024

For queries specifically -- the apiserver will soon publish the query schemas and support validation. I'm happy if the client uses this to know the apiversion

@andresmgot
Copy link
Contributor Author

the apiserver will soon publish the query schemas and support validation. I'm happy if the client uses this to know the apiversion

It seems we are lacking the infrastructure for this yet. If the approach on this PR is not the way forward then I'll close. Please let us know when we can re-evaluate this.

@andresmgot andresmgot closed this May 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants