Open tomwilkie opened 5 years ago
Yep, I'm all for folders and wasn't sure about how to structure the dashboards. See my comment here https://github.com/kubernetes-monitoring/kubernetes-mixin/pull/143#pullrequestreview-196505395
The structure looks good to me.
Are you suggesting to add this to the mixin proposal? If so I have somewhat mixed view on this. I definitely see the use of folders and want them, but it seems to me, that this is very specific to the setup, so I think each individual setup should have its own configuration that decides into what folder a dashboard goes. I think we're putting too much opinion on peoples setups to generalize this, I don't think there will ever be a good default for folder structure as everyone seems to organize them differently.
I had a play and came up with:
Basically, instead of putting dashboards in $.dashboards
you put them in $.dashboardsByFolder
.
WDYT?
I've so far treated it as largely a consumers responsibility: https://github.com/brancz/kubernetes-grafana/blob/master/grafana/grafana.libsonnet#L100
How can users use that to put mixins into specific folders? Do you have an example?
Alternatively, we could add an (optional) grafanaDashboardFolder
field to each mixin, and the system used to install the mixin could choose to honour that or not - how does that sound?
I'd rather stay away from adding more global state to mixins. A better link that I should have immediately shared is: https://github.com/brancz/kubernetes-grafana#organizing-dashboards.
Meaning if people want to organize dashboards, then that's a very opinionated thing, and it should be left entirely up to the consumer in my opinion.
I've put together a working PoC for that here: https://github.com/grafana/jsonnet-libs/pull/234
I'd rather stay away from adding more global state to mixins.
IIUC there is nothing about the mixins that require them to be merged into the "global" namespace; thats was a side effect of the early implementations that used them. The change I'm making to our mixin "user" moves them out of the global namespace (for obvious reasons).
I agree polluting the global namespace is bad; if we update the mixin spec to make it clear these shouldn't be merged into the global namespace, would that alleviate your concerns?
if we update the mixin spec to make it clear these shouldn't be merged into the global namespace, would that alleviate your concerns?
Yes, I think we should do this regardless.
I still think that choosing the dashboard folder name is something that should be left to the consumer, it's such an opinionated space that I doubt a maintainer's choice of a folder name is universal to users.
I still think that choosing the dashboard folder name is something that should be left to the consumer, it's such an opinionated space that I doubt a maintainer's choice of a folder name is universal to users.
Isn't the whole point of mixins that users can override maintainers choices? I'm more concerned about sensible defaults. We have Grafana's with ~100 dashboards all from mixins, and more mixins are being written everyday.
Sorry for the late reply. I had to think about this a bit more, but I think you're right, it should be encouraged to make the folder easily configurable I think. I expect users that don't use jsonnet for the entire setup to suffer from this potentially as it's essentially an abstraction and roughly translates into specifying datasources as part of a mixin, which I'm still not entirely comfortable with, but since people aren't forced to use it, I think I could live with it.
@sh0rez @metalmatze @csmarchbanks @povilasv thoughts?
The current spec for mixins doesn't support dashboards folders, which need to be specified in the dashboard provisioning YAML: http://docs.grafana.org/administration/provisioning/#dashboards
I suggest we add a new top-level field,
dashboardFolders
, which is of typemap[folder name]map[dashboard filename]dashboard
if you get what I mean.Integrations can then take care of provisioning the yamls etc to put these into folders. Existing integrations can make this backwards compatible by lifting this all back into the
dashboards
key.WDYT @metalmatze @brancz