Add Uid to the error log when reconciling dashboards #2143 #2173
Conversation
…ffective debugging when the reconcillation fails
RaviTejaNalluri
requested review from
nissessenap,
weisdd,
ishanjainn,
theSuess,
hubeadmin,
pb82 and
Baarsgaard
as code owners
|
Ravi Teja Nalluri seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
There was a problem hiding this comment.
I looked into logging when using kubebuilder, and I noticed we do not follow their documentation.
If we take a look at kubebuilder logging we see that they separate error logging into a separate log.Error(...) and then return the raw err struct.
Would you be open to update the dashboard and datasource controllers to follow the examples in the kubebuilder docs?
That would allows us to use automatically propegating values via:
log = log.WithValues("uid", uid)
ctx = logf.LogIntoContext(ctx, log)This would be a big benefit in that we don't have to repeat values and we are more compatible with solutions like Loki and ElasticSearch/OpenSearch.
| } | ||
| } | ||
|
|
||
| // then import the dashboard into the matching grafana instances | ||
| err = r.onDashboardCreated(ctx, &grafana, cr, dashboardModel, hash, folderUID) | ||
| if err != nil { | ||
| applyErrors[fmt.Sprintf("%s/%s", grafana.Namespace, grafana.Name)] = err.Error() | ||
| applyErrors[fmt.Sprintf("%s/%s", grafana.Namespace, grafana.Name)] = fmt.Sprintf("uid=%s: %s", uid, err.Error()) |
There was a problem hiding this comment.
I would revert this addition as we print all the errors at the end of the reconciliation.
| } | ||
|
|
||
| if grafana.Spec.Preferences != nil && uid == grafana.Spec.Preferences.HomeDashboardUID { | ||
| err = r.UpdateHomeDashboard(ctx, grafana, uid, cr) | ||
| if err != nil { | ||
| applyHomeErrors[fmt.Sprintf("%s/%s", grafana.Namespace, grafana.Name)] = err.Error() | ||
| applyHomeErrors[fmt.Sprintf("%s/%s", grafana.Namespace, grafana.Name)] = fmt.Sprintf("uid=%s: %s", uid, err.Error()) |
| @@ -174,20 +174,20 @@ func (r *GrafanaDashboardReconciler) Reconcile(ctx context.Context, req ctrl.Req | |||
| err = ReconcilePlugins(ctx, r.Client, r.Scheme, &grafana, cr.Spec.Plugins, fmt.Sprintf("%v-dashboard", cr.Name)) | |||
| if err != nil { | |||
| pluginErrors[fmt.Sprintf("%s/%s", grafana.Namespace, grafana.Name)] = err.Error() | |||
| log.Error(err, "error reconciling plugins", "dashboard", cr.Name, "grafana", grafana.Name) | |||
| log.Error(err, "error reconciling plugins", "dashboard", cr.Name, "grafana", grafana.Name, "uid", uid) | |||
There was a problem hiding this comment.
It is a mistake that we log here as the error is added to the pluginErrors map and printed later, we should instead remove this to not log the same error twice 😅
This is the issue that I worked for reviewers reference:#2143
This PR adds the Grafana resource UID to reconciliation error logs to improve troubleshooting in environments with multiple dashboards/datasources.
Changes Made:
Path: controllers/dashboard_controller.go:
Path: controllers/datasource_controller.go:
Testing: make all (lint/build/tests) passed locally.
Thank you. Do let me know if I need to change else where or if I need to make any other improvements.