-
Notifications
You must be signed in to change notification settings - Fork 24
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
Datasource selection priority #14
Comments
Yeah this is a weakness, do you think we can remove this?
Is it still set to a variable? Yeah I think that should be valid. But yeah, they should all be the same datasource IMO. |
Yep, will do. I don't see any reason to filter this based on panel type.
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. |
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 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. |
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.
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. |
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"? |
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. |
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. |
In reviewing korfuri/django-prometheus#318 I noticed a couple of things.
panel-datasource-rule
is very specific about the panel types it checks (graph
,singlestat
, andtable
) and therefore excludedtimeseries
. I've created Add timeseries to list of panel types when checking datasource #13 for this.Thoughts @tomwilkie ?
The text was updated successfully, but these errors were encountered: