Datasource selection priority

Question

Datasource selection priority

rgeyer opened this issue 3 years ago · 7 comments

In reviewing korfuri/django-prometheus#318 I noticed a couple of things.

The panel-datasource-rule is very specific about the panel types it checks (graph, singlestat, and table) and therefore excluded timeseries. I've created #13 for this.
The django dashboard actually sets the datasource on each target for the panel, rather than once on the panel itself. Both are "technically" valid. Should the panel-datasource rule accommodate this case, or should we be opinionated that different datasources on targets for the same panel is an anti-pattern? I believe that we should consider it an anti-pattern, and that there should be a target specific rule to check for it.

Answer 1 · 2022-01-21T19:38:22.000Z

The panel-datasource-rule is very specific about the panel types it checks (graph, singlestat, and table) and therefore excluded timeseries. I've created Add timeseries to list of panel types when checking datasource #13 for this.

Yeah this is a weakness, do you think we can remove this?

The django dashboard actually sets the datasource on each target for the panel, rather than once on the panel itself. Both are "technically" valid. Should the panel-datasource rule accommodate this case, or should we be opinionated that different datasources on targets for the same panel is an anti-pattern? I believe that we should consider it an anti-pattern, and that there should be a target specific rule to check for it.

Is it still set to a variable? Yeah I think that should be valid. But yeah, they should all be the same datasource IMO.

Answer 2 · 2022-02-07T18:59:15.000Z

The panel-datasource-rule is very specific about the panel types it checks (graph, singlestat, and table) and therefore excluded timeseries. I've created Add timeseries to list of panel types when checking datasource #13 for this.

Yeah this is a weakness, do you think we can remove this?

Yep, will do. I don't see any reason to filter this based on panel type.

The django dashboard actually sets the datasource on each target for the panel, rather than once on the panel itself. Both are "technically" valid. Should the panel-datasource rule accommodate this case, or should we be opinionated that different datasources on targets for the same panel is an anti-pattern? I believe that we should consider it an anti-pattern, and that there should be a target specific rule to check for it.

Is it still set to a variable? Yeah I think that should be valid. But yeah, they should all be the same datasource IMO.

In this particular case, the datasource was hardcoded to one used by the author, accidentally. But yes, the targets defining their own datasource can (and should) use the variable.

Will create a rule to enforce all datasources to be the same for a panel, if defined on targets. I'll also create another warning/guidance rule that the datasource should preferably be set only on the panel, and not on individual targets.

Answer 3 · 2022-02-08T09:40:57.000Z

Yep, will do. I don't see any reason to filter this based on panel type.

I don't recall why I put that in there TBH, but I think there was a reason. Worth experimenting with removing it and seeing what breaks for sure.

Will create a rule to enforce all datasources to be the same for a panel, if defined on targets. I'll also create another warning/guidance rule that the datasource should preferably be set only on the panel, and not on individual targets.

I think this might be a recent change in Grafana behaviour TBH - it used to be you picked the datasource at the panel level, now it seems to be at the individual "target" (expression).

I think one "principle" for the linter it you should be able to construct a dashboard that passes the linter cleanly using the Grafana UI. WDYT?

These two things combined means yes, we have to support per-target datasources. I think we should still be super opinionated and not allow mixing of different datasources - this linter is about consistency and opinionation. Perhaps we go further and say only one datasource per dashboard? And that it must be a variable? I know there as valid use cases for multiple datasources but this linter is optional and people can opt out of individual rules.

Answer 4 · 2022-02-18T17:28:20.000Z

Yep, will do. I don't see any reason to filter this based on panel type.

I don't recall why I put that in there TBH, but I think there was a reason. Worth experimenting with removing it and seeing what breaks for sure.

In thinking about this, it is almost certainly the "row" panel, which wouldn't have any target/query/datasource values, by design. I'll experiment with a few existing dashboards. Will probably make this opt out for the "weird" panels which are okay not to have a DS.

Will create a rule to enforce all datasources to be the same for a panel, if defined on targets. I'll also create another warning/guidance rule that the datasource should preferably be set only on the panel, and not on individual targets.

I think this might be a recent change in Grafana behaviour TBH - it used to be you picked the datasource at the panel level, now it seems to be at the individual "target" (expression).

I think one "principle" for the linter it you should be able to construct a dashboard that passes the linter cleanly using the Grafana UI. WDYT?

These two things combined means yes, we have to support per-target datasources. I think we should still be super opinionated and not allow mixing of different datasources - this linter is about consistency and opinionation. Perhaps we go further and say only one datasource per dashboard? And that it must be a variable? I know there as valid use cases for multiple datasources but this linter is optional and people can opt out of individual rules.

Totally agree that the linter should pass on dashboards exported directly from Grafana. This does pose some challenges for backward compatibility, and we may have to start inspecting the schema version 🤔

I do not think we should be opinionated that there should be only one datasource. Perhaps one datasource-per-type. We are starting to build dashboards which have a configurable prometheus and loki datasource to show metrics and logs.

I have time set aside to work on this, so I should be able to make some considerable improvements.

Answer 5 · 2022-02-18T18:19:55.000Z

I do not think we should be opinionated that there should be only one datasource. Perhaps one datasource-per-type. We are starting to build dashboards which have a configurable prometheus and loki datasource to show metrics and logs.

Yes good point. One per type seems more reasonable.

Can we say "if there is one it should be called ${datasource}", and "if there is one per type then they should be calles ${prometheus_datasource} etc"?

Answer 6 · 2022-02-18T18:51:26.000Z

Can we say "if there is one it should be called ${datasource}", and "if there is one per type then they should be calles ${prometheus_datasource} etc"?

Yep, I like this. That way existing dashboards won't need to be updated to be specific, but new ones with datasources of different types need to be specific. Will implement.

Answer 7 · 2022-04-05T17:04:21.000Z

Yep, will do. I don't see any reason to filter this based on panel type.

I don't recall why I put that in there TBH, but I think there was a reason. Worth experimenting with removing it and seeing what breaks for sure.

I think I figured it out, after actually giving it some thought. Some panel types will be ones without a query/target, such as row, or text.

I believe this can safely be tweaked to only check that a panel has a datasource (and that the datasource is correct) when the panel has one or more targets.