Make UID of grafana Fluent Bit dashboard configurable in values

[eg-ayoub]

value still defaults to null

[stevehipwell]

@eg-ayoub what version of Grafana are you using?

[eg-ayoub]

@stevehipwell I have tested the dashboards from this on grafana 10.3.3
this seems to work properly with or without uid set.
this is the output with dashboards.uids.fluent_bit unset:

...
        ]
      },
      "timezone": "",
      "title": "fluent-fluent-bit",
      "uid": null,
      "version": 7,
      "weekStart": ""
    }

and this is with the value set to some arbitrary uid

...
        ]
      },
      "timezone": "",
      "title": "fluent-fluent-bit",
      "uid": "some_id",
      "version": 7,
      "weekStart": ""
    }

[stevehipwell]

this seems to work properly with or without uid set.

@eg-ayoub I'm not sure what the purpose of the PR is given that the current dashboards using null work correctly?

[eg-ayoub]

@stevehipwell thank you for your response.
While the dashboard itself works properly, I use a test case which would install the chart and then query this dashboard by UID to ensure a proper deployment of my monitoring stack. This test broke as a result of #509.
I have opened #530 to track the issue.
I should have been more specific, sorry!

[eg-ayoub]

@stevehipwell checking back about this PR. Is this an unwanted feature ?
I can rebase my branch if the change is OK.

[stevehipwell]

@eg-ayoub I think you might be better off looking outside this chart if you want to manage Grafana dashboards based on the UID; the chart implementation of a random UID looks to be the right solution on this context.

[TeddyAndrieux]

@stevehipwell sorry but I disagree, I don't see any good reason to not allow the user to use fixed UID for the dashboard, especially since the behavior for the users not specifying anything will be exactly the same as today.

[stevehipwell]

@TeddyAndrieux you're more than welcome to disagree, but you haven't provided a good reason to back your disagreement up with.

[TeddyAndrieux]

@stevehipwell the reason is "Why not ?" I mean there is no reason to not authorize it if it helps some users.

And I don't see why people that want to have fixed UID shouldn't use this chart ?

To me having a fixed UID is quite useful if you want to pre-define link to a dashboard (i.e.: <grafana-url>/d/<dashboard UID>)

[stevehipwell]

@TeddyAndrieux I'm happy to listen to a case for why the extra maintenance and cognitive overhead caused by additional chart complexity is worth accepting. My standing position is that bespoke behaviour on ancillary functionality which can easily be managed outside of the chart is something to avoid adding.

[TeddyAndrieux]

@stevehipwell "complexity" ?? It's a simple value

My standing position is that bespoke behaviour on ancillary functionality which can easily be managed outside of the chart is something to avoid adding.

Except managing the dashboard by hand I don't see how it can be done, to me it just means with your approach that if you want a fixed UID you cannot use the dashboard that comes with the helm chart, which is really sad, we have to manage our own dashboard make sure it gets updated properly ... where it could basically come with the helm chart

To me either it's a hardcoded value (like in most of the helm charts in the community) either it's configurable.
But having null is a bad move to me (especially since it was not the case before, it's a regression for me, before we were able to reference the dashboard easily because the UID was fixed )

---

By the way, the PR could be simplified a bit with just

  "uid": {{ with .Values.dashboards.uids.fluent_bit }}"{{ . }}"{{ else }}null{{ end }}

[stevehipwell]

@TeddyAndrieux this chart isn't single purpose and there can be multiple installations in a given cluster so the fixed uid was explicitly incorrect for the chart use case. Part of the overhead is understanding how Grafana dashboards work, so links to docs would help reduce this.

What would you be looking to set the uid to? A meaningful string, a GUID or something else?

[TeddyAndrieux]

@stevehipwell

What would you be looking to set the uid to? A meaningful string, a GUID or something else?

To be honest we don't really care we just want this to be pre-define (even if it changes on each version it's fine for us)
Basically when we deploy a specific version of the helm chart we know the dashboard will have "this" UID

[stevehipwell]

Basically when we deploy a specific version of the helm chart we know the dashboard will have "this" UID

@TeddyAndrieux I assume you mean the same version of the chart across multiple clusters? As once the chart is installed the uid shouldn't change on that cluster

[stevehipwell]

I think the following (untested) change would make the most sense to provide a deterministic 40 character uid that would be safe for multiple installations in a single cluster.

"uid": "{{ sha1sum (printf "%s-%s" .Release.Namespace .Release.Name) }}",

[TeddyAndrieux]

I assume you mean the same version of the chart across multiple clusters? As once the chart is installed the uid shouldn't change on that cluster

It's not the case today if we come from a previous version and we upgrade to the latest one it will change from d557c8f6-cac1-445f-8ade-4c351a9076b1 to null (and so generate a new one)

But on my side I was really talking about this case I'm fine if it changes from one version to another I just need to be able to know it before applying the install/upgrade

---

Yes to me it's fine also a hash based on Namespace + Name

[eg-ayoub]

@stevehipwell @TeddyAndrieux ok in that case I can go ahead and rework this PR to apply that specific change

[stevehipwell]

It's not the case today if we come from a previous version and we upgrade to the latest one it will change from d557c8f6-cac1-445f-8ade-4c351a9076b1 to null (and so generate a new one)

@TeddyAndrieux that was the one off change when it was switched to null and any future change will do the same thing again. The current system will keep the uid consistent unless we change the logic again.

Yes to me it's fine also a hash based on Namespace + Name

You're right it should be.

"uid": "{{ sha1sum (printf "%s-%s" .Release.Namespace (include "fluent-bit.fullname" .)) }}",

@eg-ayoub you could modify the logic slightly to support a dashboards.deterministicUid input.

"uid": {{ ternary (sha1sum (printf "\"%s-%s\"" .Release.Namespace (include "fluent-bit.fullname" .))) "null" .Values.dashboards.deterministicUid }},

[stevehipwell]

Thanks @eg-ayoub. You'll need to add the dashboards.deterministicUid value to values.yaml and it should default to false so it doesn't churn for anyone who doesn't opt in. Could you also updated ci/ci-values.yaml to set dashboards.deterministicUid to true.

To make this mergeable you'll also need to bump the version and update the release notes annotations.

[TeddyAndrieux]

Just tested this

"uid": {{ ternary (sha1sum (printf "\"%s-%s\"" .Release.Namespace (include "fluent-bit.fullname" .))) "null" .Values.dashboards.deterministicUid }},

with value set to True and it does not work on my side I think it's because it's the "namespace-name" that is quoted and not the sha1sum, I would do something like that instead

"uid": {{ if .Values.dashboards.deterministicUid }}"{{ sha1sum (printf "%s-%s" .Release.Namespace (include "fluent-bit.fullname" .)) }}"{{ else }}null{{ end }},

[stevehipwell]

@TeddyAndrieux I've tested it locally and it works correctly as pasted here if the default value is correctly configured. You could modify the clause to (not (not .Values.dashboards.deterministicUid)) to handle this without an error.

[TeddyAndrieux]

@stevehipwell I may have miss explained it, I mean the rendering of the helm template works well but the resulting json is wrong since there is no "quote" on the UID

E.g.: With namespace default and name "test" we get the following

      "title": "test-fluent-bit",
      "uid": e51705004faf6bbbf2d1a4d779250ef34f008917,
      "version": 7,

Because it's quoted before the sha1sum

$ echo -n '"default-test-fluent-bit"' | sha1sum
e51705004faf6bbbf2d1a4d779250ef34f008917  -

[stevehipwell]

Sorry I missed which bit wasn't working, yes the quotes were in the wrong place.

"uid": {{ ternary (printf "\"%s\"" (sha1sum (printf "%s-%s" .Release.Namespace (include "fluent-bit.fullname" .)))) "null" .Values.dashboards.deterministicUid }},

[eg-ayoub]

@stevehipwell I've applied all the changes, I think this is ready to merge 😄

[stevehipwell]

Thanks @eg-ayoub. Could you rebase your branch and bump the chart version so I can run the tests?

[eg-ayoub]

@stevehipwell rebased.

[stevehipwell]

LGTM