update to telem work #588
update to telem work #588
Conversation
WilliamMortlMicrosoft
added
enhancement
high-priority
controllers/async_controller.go
Outdated
| @@ -35,36 +36,30 @@ type AsyncReconciler struct { | |||
| Telemetry telemetry.PrometheusTelemetry | |||
| Recorder record.EventRecorder | |||
| Scheme *runtime.Scheme | |||
| Log logr.Logger | |||
There was a problem hiding this comment.
logr.Logger is already contained within the Telemetry library in PrometheusClient::Logger... all events that are logged at any level are sent to logr as well as to Prometheus. This isn't needed. If you want to just log to logr, just use r.Telemetry.LogTrace() (that doesn't log to Prometheus, just to logr)
Or we can expose the Logger as part of the Telemetry
controllers/async_controller.go
Outdated
| // r.Telemetry.LogSuccess() | ||
| // } | ||
| // }() | ||
| log := r.Log.WithValues("instance", req.String()) |
There was a problem hiding this comment.
This isn't needed. If you want to just log to logr, just use r.Telemetry.LogTrace()
There was a problem hiding this comment.
I think I was struggling to find a way to get the instance name into the logger because it was being instantiated in main.go, maybe we can fork that client with the new value here
controllers/async_controller.go
Outdated
|
|
||
| if err := r.Get(ctx, req.NamespacedName, local); err != nil { | ||
| r.Telemetry.LogInfo("ignorable error", "error during fetch from api server") | ||
| log.Info("error during fetch from api server") |
There was a problem hiding this comment.
r.Telemetry.LogInfo
controllers/async_controller.go
Outdated
| res, err := meta.Accessor(local) | ||
| if err != nil { | ||
| r.Telemetry.LogError("accessor fail", err) | ||
| log.Info("failed getting meta accessor", "err", err) |
There was a problem hiding this comment.
r.Telemetry.LogInfo
controllers/async_controller.go
Outdated
| @@ -79,7 +74,8 @@ func (r *AsyncReconciler) Reconcile(req ctrl.Request, local runtime.Object) (res | |||
| found, deleteErr := r.AzureClient.Delete(ctx, local) | |||
| final := multierror.Append(deleteErr) | |||
| if err := final.ErrorOrNil(); err != nil { | |||
| r.Telemetry.LogError("error deleting object", err) | |||
| log.Info("error deleting object", "err", err) | |||
There was a problem hiding this comment.
r.Telemetry.LogInfo or LogWarning
controllers/async_controller.go
Outdated
|
|
||
| if pAccessor, err := meta.Accessor(p.Target); err == nil { | ||
| if err := controllerutil.SetControllerReference(pAccessor, res, r.Scheme); err == nil { | ||
| r.Telemetry.LogInfo("status", "setting parent reference to object: "+pAccessor.GetName()) | ||
| log.Info("setting parent reference", "target", pAccessor.GetName()) |
There was a problem hiding this comment.
r.Telemetry.LogInfo
controllers/async_controller.go
Outdated
| } | ||
| break | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| r.Telemetry.LogInfo("status", "reconciling object") | ||
| log.Info("reconciling object") |
There was a problem hiding this comment.
I'm not going to add any more comments regarding this, but all log.* should use the Telemetry LogTrace instead
| RequestedAt *metav1.Time `json:"requested,omitempty"` | ||
| CompletedAt *metav1.Time `json:"completed,omitempty"` |
|
|
||
| // log that operator is running | ||
| client.Logger.Info("Component has started", "Component", client.Component) | ||
| activeGuage.WithLabelValues(client.Component).Inc() | ||
| //client.Logger.Info("Component has started", "Component", client.Component) |
There was a problem hiding this comment.
Why are these commented out? Are these lines needed?
closes #489
closes #432