HYPERFLEET-430 | fix: formalize the executor log to help on debugging and tracking #24
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughThe diff refactors executor logging into a phase-oriented model: phases (Parameter Extraction, Preconditions, Resources, Post Actions, Finalization) now emit phase-qualified RUNNING/FAILED/NOT_MET/SUCCESS messages and phase-level summaries. Per-item index-based logs were replaced by name-based logs or consolidated phase summaries. Resource execution adds generation-based decision logic (Create/Update/Recreate/Skip) and records an Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
Comment |
xueli181114
force-pushed
the
HYPERFLEET-430
branch
from
3b883ef to
8627b2a
Compare
xueli181114
force-pushed
the
HYPERFLEET-430
branch
from
8627b2a to
79bf801
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/executor/resource_executor.go (1)
153-159: Consider splitting long log lines for readability.The detailed logging is excellent for debugging, but the lines exceed typical length guidelines. Consider formatting the log arguments across multiple lines for better maintainability.
🔎 Optional formatting improvement
- re.log.Errorf(ctx, "Resource[%s] processed: FAILED - operation=%s reason=%s kind=%s name=%s error=%v", - resource.Name, result.Operation, result.OperationReason, gvk.Kind, manifest.GetName(), err) + re.log.Errorf(ctx, + "Resource[%s] processed: FAILED - operation=%s reason=%s kind=%s name=%s error=%v", + resource.Name, result.Operation, result.OperationReason, gvk.Kind, manifest.GetName(), err)- re.log.Infof(ctx, "Resource[%s] processed: SUCCESS - operation=%s reason=%s kind=%s name=%s", - resource.Name, result.Operation, result.OperationReason, gvk.Kind, manifest.GetName()) + re.log.Infof(ctx, + "Resource[%s] processed: SUCCESS - operation=%s reason=%s kind=%s name=%s", + resource.Name, result.Operation, result.OperationReason, gvk.Kind, manifest.GetName())
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
internal/executor/executor.gointernal/executor/post_action_executor.gointernal/executor/precondition_executor.gointernal/executor/resource_executor.gointernal/executor/types.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/executor/post_action_executor.go
- internal/executor/types.go
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: xueli181114
Repo: openshift-hyperfleet/hyperfleet-adapter PR: 13
File: internal/executor/post_action_executor.go:198-205
Timestamp: 2025-12-04T14:06:51.656Z
Learning: Logger refactor is tracked in HYPERFLEET-304 for the hyperfleet-adapter repository, which will address how CEL evaluation failures and similar errors are handled and logged.
🧬 Code graph analysis (1)
internal/executor/executor.go (1)
internal/executor/types.go (3)
PhaseResources(25-25)PhasePostActions(27-27)StatusFailed(37-37)
🔇 Additional comments (8)
internal/executor/executor.go (2)
91-127: LGTM! Phase-oriented logging enhances observability.The phase-qualified logs (RUNNING, SUCCESS, FAILED, NOT_MET) provide clear execution flow and debugging context. The distinction between phase execution success and business outcomes (MET vs. NOT_MET) is well-handled.
131-167: LGTM! Resource and post-action phase logging is consistent.The phase-driven logging pattern is uniformly applied to resource and post-action phases, including proper handling of skipped resources and comprehensive error context.
internal/executor/resource_executor.go (3)
6-6: LGTM! Name-based iteration and logging improve clarity.The shift from index-based to name-based iteration and logging makes debugging easier by using meaningful resource identifiers throughout the execution flow.
Also applies to: 42-42, 76-76
95-98: LGTM! Appropriate log level adjustment.Moving discovery logs to Debug level reduces noise while maintaining visibility when detailed troubleshooting is needed.
101-143: LGTM! Operation decision logic is well-structured.The refactored decision logic cleanly separates operation determination from execution. The generation-based change detection with
OperationReasonprovides clear audit trails for debugging. The switch statement improves maintainability and makes the execution flow explicit.internal/executor/precondition_executor.go (3)
35-61: LGTM! Name-based precondition logging improves traceability.The refactored iteration and logging using precondition names rather than indices makes debugging clearer. The
formatConditionDetailshelper provides structured failure reasons that enhance observability.
86-86: LGTM! Appropriate log level adjustments for API call details.Moving API call implementation details to Debug level reduces noise in standard operations while keeping detailed traces available for troubleshooting.
Also applies to: 126-126, 153-153
171-171: LGTM! Well-balanced log level adjustments for conditions.The log level changes appropriately distinguish between implementation details (Debug) and business outcomes (Info). Keeping NOT_MET conditions at Info level ensures visibility for operational issues while reducing noise from successful evaluations.
Also applies to: 187-187, 189-189, 201-201, 211-211, 217-217
| result.Errors[result.CurrentPhase] = precondErr | ||
| execCtx.SetError("PreconditionFailed", precondOutcome.Error.Error()) | ||
| e.log.Errorf(ctx, "Precondition execution failed: %v", precondOutcome.Error) | ||
| e.log.Errorf(ctx, "Phase %s: FAILED - %v", result.CurrentPhase, precondOutcome.Error) |
There was a problem hiding this comment.
Do you mean it should add error= here? Should we keep consistent in one PR?
| result.Errors[result.CurrentPhase] = resErr | ||
| execCtx.SetError("ResourceFailed", err.Error()) | ||
| e.log.Errorf(ctx, "Resource execution FAILED: %v", err) | ||
| e.log.Errorf(ctx, "Phase %s: FAILED - %v", result.CurrentPhase, err) |
| postErr := fmt.Errorf("post action execution failed: %w", err) | ||
| result.Errors[result.CurrentPhase] = postErr | ||
| e.log.Errorf(ctx, "Post action execution FAILED: %v", err) | ||
| e.log.Errorf(ctx, "Phase %s: FAILED - %v", result.CurrentPhase, err) |
2 participants
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.