feat: add GrafanaLibraryPanel CRD and reconciler #1858
Conversation
github-actions
bot
added
the
documentation
diurnalist
force-pushed
the
f/library-panels-2
branch
3 times, most recently
from
4e48b30 to
7488d2e
Compare
diurnalist
requested review from
NissesSenap,
weisdd,
ishanjainn,
theSuess,
hubeadmin and
pb82
as code owners
|
i did not add unit tests for the reconciler, because it was not clear to me how to effectively implement them, and there were no other examples in the repo of tests for reconcilers. i am continuing to do some testing in my own cluster and will think about unit tests we could implement. |
|
howdy @weisdd @NissesSenap - just wanted to ping on this, i know it is a larger PR. please let me know if you would prefer breaking it into smaller PRs rather than reviewing commit-by-commit. i'm highly motivated to finish the work so that others in the community can take advantage of this feature and i can remove the need for our custom fork of the operator! |
|
Hey @diurnalist - I'll take a look today. We haven't had a chance to look at this yet due to us working on other projects as well 😅 |
this adds a new GrafanaLibraryPanel CRD, updates all the auto-generated assets derived from the CRD type, and updates existing RBAC and Helm definitions to reference the new type. Co-authored-by: Matt Delaney <matt.delaney@getcruise.com>
this registers the new CRD with the Grafana CRD so that we can track which library panels have been provisioned with the operator, similar to how other resources are tracked. Co-authored-by: Matt Delaney <matt.delaney@getcruise.com>
Co-authored-by: Matt Delaney <matt.delaney@getcruise.com>
much of the content resolving/fetching logic is shared b/w dashboards and library panels, but there is one awkward thing where library panels are not currently (to my knowledge) available via grafana.com. rather than extract just that fetch logic out and only apply it to dashboards, i've opted to simply disable them as a source. an error will be issued if a user tries to configure this field. it is not the most graceful way to handle this, as it's not communicated via the CR validation, but given the effort required to bend the code to do this "right", and given the likelihood of somebody trying to configure a library panel via a grafana.com url (which does not exist right now), we are choosing the easy button.
to enable dashboards and library panels to track their fetch operations separately, split up the metrics by resource. this is accomplished by adding a new interface method to the GrafanaContentResource, and the returned struct holds references to the relevant metrics, if any. this also adds an additional guard to the http round tripper so that if no metric is defined, we don't panic.
diurnalist
force-pushed
the
f/library-panels-2
branch
from
6ccda3b to
31f70ee
Compare
since grafana#1856, the api module is separated. this poses problems with the approach to have the CRs expose which metrics correspond to their types via the GrafanaContentMetrics() function, because we create an import loop. recognizing that we're measuring essentially the same thing b/w library panels and dashboards, this updates the metrics to indicate this more explicitly. the following changes are made to metrics: `grafana_operator_dashboard_requests`: * renamed to `grafana_operator_content_requests`, but will always have a label `kind="GrafanaDashboard"`. * renamed `dashboard` label to `resource` `grafana_operator_dashboard_revision_requests`: * renamed to `grafana_operator_revision_requests`, but will always have a label `kind="GrafanaDashboard"`. * renamed `dashboard` label to `resource`
diurnalist
force-pushed
the
f/library-panels-2
branch
from
78e9adc to
8d0c441
Compare
|
@theSuess i think i've addressed the requested changes! unfortunately i had to add a new commit that changes a couple of metrics due to the |
to make the dashboard url request metric backwards compatible, this updates the signature of the roundtripper to be a bit more general, so we can pass a function that is called at the end of the response rather than explicitly increment a metric. this allows us to increment multiple metrics in the case of the dashboard url request, one for the old and one for the new.
diurnalist
force-pushed
the
f/library-panels-2
branch
from
be8f437 to
ffb275b
Compare
|
I tried my hand at simplifying the metrics logic and reconciler a bit more. Since this now includes code of mine as well, I'd appreciate a review from another maintainer as well |
|
@Baarsgaard thank you! |
|
@Baarsgaard thanks for the tips about the error handling. i have moved the validation checks earlier and we are now registering the InvalidSpec condition if those checks fail. also applied cleanup to the other points you mentioned :) |
Co-authored-by: Steffen Baarsgaard <steff.bpoulsen@gmail.com>
|
thanks everybody for the detailed reviews and timely feedback ❤️ . i learned many new things about good operator coding practices and patterns. as a next step i will write up some docs and add more examples to illustrate how this capability can work. |
this partially implements the feature proposed in #1611, which allows the operator to manage library panels just as it manages dashboards.
this is a large PR but i have made an attempt at breaking it into smaller pieces, first adding the CRD types, then the reconciler implementation, then a few refactor commits to make things cleaner and re-use or update existing abstractions.
my first approach was to copy the Dashboard reconciler, and I actually had that finished 😅 before realizing that the newer controllers were probably better to take inspiration from, as they seem to use more shared and modern patterns. so i rewrote it to share more with the alert* reconcilers.
library panels are a bit weird in that they have the concept of update vs. create, which we have to understand in the reconciler. otherwise they are very similar to dashboards and i worked to port most of the logic that dashboards have around setting custom UIDs and otherwise falling back to what is defined on the content model JSON.
this does not implement anything fancy around linking library panels to dashboards via references. i think that's an area of future improvement for this, so you can loosely couple dashboard and library panels when both are configured as code.