Emiting reconcile_elapsed_ms metrics #6534
Conversation
There was a problem hiding this comment.
Hey
So we already have the metric being logged. But there are some changes that needs to be done.
- It should be a
EventTypeMetricand not log. - Needs to log for cancellations as well.
- Need to separate out normal errors from timeouts.
- Need to include
retrigger_time. - Need to include rill_version. It is not present in runtime as of now but you can populate the same by making changes similar to admin service here :
Lines 292 to 293 in baa6274
@k-anshul rill_version should already be there because it got added in activety |
NamanMahor
changed the title
runtime/controller.go
Outdated
| commonDims = append(commonDims, attribute.String("reconcile_status", "Cancelled")) | ||
| } else if inv.result.Err != nil { | ||
| if errors.Is(inv.result.Err, context.Canceled) { | ||
| commonDims = append(commonDims, attribute.String("reconcile_status", "Cancelled")) |
There was a problem hiding this comment.
Nit : Canceled and not Cancelled
runtime/controller.go
Outdated
| if inv.result.Err != nil { | ||
| eventArgs = append(eventArgs, attribute.String("error", inv.result.Err.Error())) | ||
| commonDims := []attribute.KeyValue{ | ||
| attribute.String("resource_id", inv.name.Name), |
There was a problem hiding this comment.
| attribute.String("resource_id", inv.name.Name), | |
| attribute.String("resource_name", inv.name.Name), |
runtime/controller.go
Outdated
| if inv.isDelete { | ||
| commonDims = append(commonDims, attribute.Bool("is_deleted", true)) | ||
| } | ||
| if inv.isRename { | ||
| commonDims = append(commonDims, attribute.Bool("is_renamed", true)) | ||
| } |
There was a problem hiding this comment.
Grammar:
| if inv.isDelete { | |
| commonDims = append(commonDims, attribute.Bool("is_deleted", true)) | |
| } | |
| if inv.isRename { | |
| commonDims = append(commonDims, attribute.Bool("is_renamed", true)) | |
| } | |
| if inv.isDelete { | |
| commonDims = append(commonDims, attribute.Bool("is_delete", true)) | |
| } | |
| if inv.isRename { | |
| commonDims = append(commonDims, attribute.Bool("is_rename", true)) | |
| } |
There was a problem hiding this comment.
added reconcile_operation instead of two different dimensions
runtime/controller.go
Outdated
| } | ||
|
|
||
| if !inv.cancelledOn.IsZero() { | ||
| commonDims = append(commonDims, attribute.String("reconcile_status", "Cancelled")) |
There was a problem hiding this comment.
reconcile_status is not a good key here – we already have something called ReconcileStatus and it takes different values:
rill/proto/rill/runtime/v1/resources.proto
Lines 12 to 17 in d4fa2a1
Maybe consider reconcile_result instead?
runtime/controller.go
Outdated
| } | ||
| c.Activity.Record(context.Background(), activity.EventTypeLog, "reconciled_resource", eventArgs...) | ||
| } else { | ||
| commonDims = append(commonDims, attribute.String("reconcile_status", "Succeeded")) |
There was a problem hiding this comment.
nit: call it "success" instead of "Succeeded"
runtime/controller.go
Outdated
| commonDims = append(commonDims, attribute.String("reconcile_status", "Cancelled")) | ||
| } else if inv.result.Err != nil { | ||
| if errors.Is(inv.result.Err, context.Canceled) { | ||
| commonDims = append(commonDims, attribute.String("reconcile_status", "Cancelled")) |
There was a problem hiding this comment.
Given the key is snake case, consider also using snake case for the values (this seems like pascal case or title case). E.g. canceled instead of Cancelled.
Emiting reconcile_elapsed_ms metrics