Datasources: Propagate TestDataDB default selection to PanelModel #57703
Conversation
mmandrus
added
no-backport
😩 |
|
Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/39439 |
|
Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/39444 |
mmandrus
requested review from
joshhunt and
JoaoSilvaGrafana
and removed request for
a team
|
Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/39477 |
|
Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/39483 |
There was a problem hiding this comment.
Calling onChange onMount is not great (it has caused various issues in the past) so are strongly discouraged, ideally, we should have a better way to set query defaults. But this ability has been lacking for a long time :(
@sunker did you explore ways to do this before? I know you have hit this problem many times
|
@mmandrus you can specify a default query using the getDefaultQuery method in the datasource api. That way you don't have to call |
| if (!query.datasource && this.datasource) { | ||
| query.datasource = this.datasource; | ||
| } | ||
| if (Object.keys(query).length === 2) { |
There was a problem hiding this comment.
This is based on the combination of the above if statement, plus the ensureQueryIds() function. If the only two properties on the query are the ones we are hard-coding in, the query is not well-formed. If there is a better way of inferring this, let me know.
|
Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/39616 |
|
@torkelo one more try... now it takes care of all the default query handling at query creation time. Thanks for your patience as I learn my way around the front-end codebase! |
|
Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/39629 |
|
Awaiting approval before I address build issues. |
| getDefaultQuery(app: CoreApp): TestDataQuery { | ||
| switch (app) { | ||
| // Ignore the app input for now, always use the same default query | ||
| case CoreApp.CloudAlerting: | ||
| case CoreApp.UnifiedAlerting: | ||
| case CoreApp.Dashboard: | ||
| case CoreApp.Explore: | ||
| case CoreApp.PanelEditor: | ||
| case CoreApp.PanelViewer: | ||
| case CoreApp.Unknown: | ||
| default: | ||
| return defaultQuery; | ||
| } | ||
| } |
There was a problem hiding this comment.
| getDefaultQuery(app: CoreApp): TestDataQuery { | |
| switch (app) { | |
| // Ignore the app input for now, always use the same default query | |
| case CoreApp.CloudAlerting: | |
| case CoreApp.UnifiedAlerting: | |
| case CoreApp.Dashboard: | |
| case CoreApp.Explore: | |
| case CoreApp.PanelEditor: | |
| case CoreApp.PanelViewer: | |
| case CoreApp.Unknown: | |
| default: | |
| return defaultQuery; | |
| } | |
| } | |
| getDefaultQuery(app: CoreApp): TestDataQuery { | |
| return defaultQuery; | |
| } |
| if (changedQueries) { | ||
| this.onChange({ | ||
| queries | ||
| }); | ||
| } |
There was a problem hiding this comment.
This is basically the same as calling onChange onMount in the query editor, you could maybe refactor out to a function that can also be used from runRequest? Then we can detect and perform this logic before issuing requests as well. And then we don't need to call this.onChange (which should be preserved for use-initiated change)
There was a problem hiding this comment.
Do we really need to call onChange? If not I think this can be simplified a bit
const queries = options.queries.map((q) => {
if (!q.datasource) {
q = { ...q, datasource };
}
if (Object.keys(q).length === 2) {
q = { ...this.newQuery(ds, dsSettings), ...q };
}
return q;
});
this.setState({ queries, dataSource: ds, dsSettings, defaultDataSource });Think we may also need to set default query in [updateQueries](https://github.com/grafana/grafana/blob/mmandrus/fix-testdatadb-default-selection/public/app/features/query/state/updateQueries.ts#L30-L36) too. Otherwise, this won't work when changing the data source.
|
This pull request was removed from the 9.3.0-beta1 milestone because 9.3.0-beta1 is currently being released. |
|
Closing, fixed by #59625 |
What is this feature?
Currently, when a TestDataDB datasource is selected, it will automatically create a Random Walk scenario based on a frontend. However, this doesn't get set in the dashboard JSON itself, and actually relies on the backend to have the same default. As a result, in public dashboard mode, there is no query json and the panel can't render anything.
Why do we need this feature?
Not only does this fix a bug in public dashboards, it ensures that we don't have to have matching default behaviors on the front and back ends of the app.
Which issue(s) does this PR fix?:
Fixes #57702