Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Emiting reconcile_elapsed_ms metrics #6534

Merged
merged 4 commits into from
Feb 4, 2025
Merged

Emiting reconcile_elapsed_ms metrics #6534

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6575b4d
Emiting reconcile_time_ms metrics
NamanMahor Jan 30, 2025
9c220ae
fix linting error
NamanMahor Jan 30, 2025
a5218d3
removing reconcile log event as metrics were added
NamanMahor Jan 31, 2025
1c64d3d
review comments
NamanMahor Feb 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 28 additions & 19 deletions runtime/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,6 @@ func (c *Controller) Run(ctx context.Context) error {
if len(hanging) != 0 {
loopErr = fmt.Errorf("reconciles for resources %v have hung for more than %s after cancelation", hanging, reconcileCancelationTimeout.String())
stop = true
break
}
case <-c.catalog.hasEventsCh: // The catalog has events to process
// Need a write lock to call resetEvents.
Expand All @@ -257,7 +256,6 @@ func (c *Controller) Run(ctx context.Context) error {
c.mu.RUnlock()
case <-ctx.Done(): // We've been asked to stop
stop = true
break
}
}

Expand All @@ -275,7 +273,7 @@ func (c *Controller) Run(ctx context.Context) error {
}
c.mu.RUnlock()

// Allow 10 seconds for closing invocations and reconcilers
// Allow 30 seconds for closing invocations and reconcilers
ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
defer cancel()

Expand Down Expand Up @@ -1360,24 +1358,35 @@ func (c *Controller) processCompletedInvocation(inv *invocation) error {
c.Logger.Info("Reconciled resource", logArgs...)
}

// Emit event unless it was a cancellation.
if inv.cancelledOn.IsZero() {
eventArgs := []attribute.KeyValue{
attribute.String("name", inv.name.Name),
attribute.String("type", PrettifyResourceKind(inv.name.Kind)),
attribute.Int64("elapsed_ms", elapsed.Milliseconds()),
}
if inv.isDelete {
eventArgs = append(eventArgs, attribute.Bool("deleted", true))
}
if inv.isRename {
eventArgs = append(eventArgs, attribute.Bool("renamed", true))
}
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),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
attribute.String("resource_id", inv.name.Name),
attribute.String("resource_name", inv.name.Name),

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

attribute.String("resource_type", PrettifyResourceKind(inv.name.Kind)),
}
if inv.isDelete {
commonDims = append(commonDims, attribute.Bool("is_deleted", true))
}
if inv.isRename {
commonDims = append(commonDims, attribute.Bool("is_renamed", true))
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Grammar:

Suggested change
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))
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added reconcile_operation instead of two different dimensions


if !inv.cancelledOn.IsZero() {
commonDims = append(commonDims, attribute.String("reconcile_status", "Cancelled"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

enum ReconcileStatus {
RECONCILE_STATUS_UNSPECIFIED = 0;
RECONCILE_STATUS_IDLE = 1;
RECONCILE_STATUS_PENDING = 2;
RECONCILE_STATUS_RUNNING = 3;
}

Maybe consider reconcile_result instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

} else if inv.result.Err != nil {
if errors.Is(inv.result.Err, context.Canceled) {
commonDims = append(commonDims, attribute.String("reconcile_status", "Cancelled"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit : Canceled and not Cancelled

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

} else if errors.Is(inv.result.Err, context.DeadlineExceeded) {
commonDims = append(commonDims, attribute.String("reconcile_status", "Timeout"))
} else {
commonDims = append(commonDims, attribute.String("reconcile_status", "Error"), attribute.String("reconcile_error", inv.result.Err.Error()))
}
c.Activity.Record(context.Background(), activity.EventTypeLog, "reconciled_resource", eventArgs...)
} else {
commonDims = append(commonDims, attribute.String("reconcile_status", "Succeeded"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: call it "success" instead of "Succeeded"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

}

if !inv.result.Retrigger.IsZero() {
commonDims = append(commonDims, attribute.String("retrigger_time", inv.result.Retrigger.Format(time.RFC3339)))
}
c.Activity.RecordMetric(context.Background(), "reconcile_elapsed_ms", float64(elapsed.Milliseconds()), commonDims...)

r, err := c.catalog.get(inv.name, true, false)
if err != nil {
Expand Down
Loading