Add QueryConversionHandler #1071
Conversation
andresmgot
requested review from
wbrowne,
marefr and
oshirohugo
and removed request for
a team
There was a problem hiding this comment.
Left some comments. Still a bit hard to grasp all of this :)
Query migration is definitely a tricky topic. Sorry if my comments are hard to understand, but let me know and we talk about it instead.
Thinking about config migration it feels a bit more straight forward.
But code wise, say you have multiple API versions supported then to me feels like it would make sense to have one datasource/set of config/types/go package per each API version rather than having one of these and trying to mix logic to support multiple API versions. In that case would make sense to have some abstraction/api above the instance management. Again, maybe I've misunderstood and we're not on such path where supporting multiple api versions in one datasource is desired.
If I've misunderstood then the routing to API version/datasource plugin version will happen on a higher level (api aggregation? 🤷). Then the question is who would be responsible deciding whether a query should be converted/migrated or not. And will there still be needs for converting a query (like this proposal) even though the API version is correct?
| "context" | ||
| ) | ||
|
|
||
| // QueryConversionHandler is an EXPERIMENTAL service that allows converting queries between versions |
There was a problem hiding this comment.
Based on this should it live in an experimental package?
There was a problem hiding this comment.
could be but I am just following the same approach than the other new handlers.
| @@ -27,6 +45,19 @@ func (a *dataSDKAdapter) QueryData(ctx context.Context, req *pluginv2.QueryDataR | |||
| err := wrapHandler(ctx, parsedReq.PluginContext, func(ctx context.Context) (RequestStatus, error) { | |||
| ctx = withHeaderMiddleware(ctx, parsedReq.GetHTTPHeaders()) | |||
| var innerErr error | |||
| if a.queryConversionHandler != nil && GrafanaConfigFromContext(ctx).FeatureToggles().IsEnabled("dsQueryConvert") { | |||
| convertedQuery, innerErr := a.ConvertQueryData(ctx, parsedReq) | |||
There was a problem hiding this comment.
Based on the example, https://github.com/grafana/grafana-plugin-examples/pull/340/files#diff-e608edd147405086920bcb097aae449ffb1ca927fea2ab583e93e9bde6b6ee93, this looks a bit costly resource wise to always call it if implemented. You would have to parse the JSON to figure out if it should be converted/migrated. Then after handling the query data in plugin you would have to parse the JSON again. Not optimal, but maybe not a big deal?
But from plugin development perspective I'm not sure how this helps me compared to having some conversion/migration happening within QueryData that you could do already today without these changes.
It feels like this could be useful if the SDK would handle everything related to figure out if conversion is needed using API version, plugin version or some version/incremented value. Maybe I've been out of the loop lately and missed where we're headed with this?
There was a problem hiding this comment.
yeah, the goal here was to avoid people having to manually call the migrate function but that comes with the cost of the extra marshalling-unmarshalling. I am going to refactor it so people will need to call it manually.
|
Again, making this a draft until we discuss this more. |
So far, my goal is to run this for the same API version, so developers don't need to modify a major version in case they want to modify the query model. |
What this PR does / why we need it:
Automatically executes a function
ConvertQueryif a plugin implements it. This is executed before callingQueryDataand is not yet exposed as a gRPC call (even though the goal is usingConvertObjectsto call it).See an example of how this may look like for a plugin here
Which issue(s) this PR fixes:
First part of grafana/grafana#92749
Special notes for your reviewer: