Experimental: Add query type definition and schemas#897
Conversation
There was a problem hiding this comment.
This file was generated by dropping the folder in grafana/grafana/pkg/apis and running codegen
Eventually this should not require any manual steps, but as an initial pass it seems OK
There was a problem hiding this comment.
Can you add instructions about how to update this in the README?
ryantxu
changed the title
ryantxu
requested review from
andresmgot,
oshirohugo and
wbrowne
and removed request for
a team
andresmgot
left a comment
andresmgot
left a comment
There was a problem hiding this comment.
I see no harm (increased size) for existing plugins so we can continue with this
There was a problem hiding this comment.
Can you add instructions about how to update this in the README?
| @@ -0,0 +1,19 @@ | |||
| package v0alpha1 | |||
There was a problem hiding this comment.
if this is going to hold only query schemas for the time being we can call the group query (apis/query/v0alpha1/). If the plan is to add more things here then data would also be fine.
There was a problem hiding this comment.
Also, why not adding this to experimental first?
There was a problem hiding this comment.
If the plan is to add more things here then data would also be fine.
The next thing is settings (app + datasource... maybe the same object?) so want to avoid "query" -- i'll use data; it is a bit awkward since that is also the package for data.Frame, but eventually Frame should be in something clients would want access to
why not adding this to experimental first?
I have bounced between them a few times 🤣 -- the question is really how compatible we want to be with k8s. The directory structure /apis/{group}/{version} is standard in k8s and deviating from it makes life hard. In that world "v0*" is experimental, and also has a clear pattern for version evolution.
Working from the assumption that we will eventually land in k8s style apis seems like the right path here (though I do not feel too strongly about it).
There was a problem hiding this comment.
For the purpose of not confusing/communicating to plugin authors whether this is experimental or not it would be good to put this in the experimental package I think for now until things has settled/this is officially supported.
| headers []string | ||
| } | ||
|
|
||
| func NewQueryDataClient(url string, client *http.Client, headers ...string) QueryDataClient { |
There was a problem hiding this comment.
I am not sure I understand the purpose of this new client? Is it an example?
There was a problem hiding this comment.
It is what alert ruler would call (currently we say "call /ds/query" but there is no typed client).
It is also the abstraction used inside the query service that will execute query requests. We have a few implementations:
- OSS/single binary: just delegate to the internal.PluginClient
- multi-tenant query service > single tenant ds query (pretty much use this client)
- multi-tenant query service > multi-tenant datasource, use this client with an http.Client provided by https://github.com/kubernetes/client-go/blob/e34b66436f2c4e898fd0199e30ac0a1bcc473d3b/rest/transport.go#L32
We don't necessarily need the implementation here (could be internal) but then not sure how to best have alert ruler use the client.
There was a problem hiding this comment.
it may make sense in its own package (and perhaps with a generic for the query type) but given this is all in experimental/v0alpha I don't think it matters too much yet
|
@marefr -- i move the package, can you take another look? |
|
|
||
| type QueryDataRequest struct { | ||
| // Time range applied to each query (when not included in the query body) | ||
| TimeRange `json:",inline"` |
There was a problem hiding this comment.
I think it would be good to remove this if we can since I don't think it adds any additional functionality - but does add complexity to the Request and handling of the Request.
If there is no additional functionality, and it is just for convenience, then I think it would be better to have Code (e.g. Methods) to set a single on all the queries, rather then have the Type that supports both.
| // TimeRange represents a time range for a query and is a property of DataQuery. | ||
| type TimeRange struct { | ||
| // From is the start time of the query. | ||
| From string `json:"from" jsonschema:"example=now-1h,default=now-6h"` |
There was a problem hiding this comment.
- From and Two should be typed beyond being a string so we can have valid and valid values for what they are.
- If relative times are supported, there should also be an (optional) Timestamp to indicate an absolute value for "now".
- We should have methods or function that return time.Time for these, so that does not vary between receivers.
There was a problem hiding this comment.
If not, then things like following are valid, and if they don't work it is a bug ;-)
4 participants
This PR has two key parts:
In an effort to discover if the shapes are useful and easy enough to integrate, these changes are exercised in a few projects: