Skip to content

Comments

Modules: Validate execution plans at startup to remove per-request warnings#4662

Open
postindustria-code wants to merge 3 commits intoprebid:masterfrom
postindustria-tech:execution-plan-validation-for-disabled-modules
Open

Modules: Validate execution plans at startup to remove per-request warnings#4662
postindustria-code wants to merge 3 commits intoprebid:masterfrom
postindustria-tech:execution-plan-validation-for-disabled-modules

Conversation

@postindustria-code
Copy link
Contributor

Issue

Addresses Issue #3748

Behavior Changes

Before:

  • Warning logged on every request for disabled modules: "Not found hook while building hook execution plan: "
  • Created massive log noise in high-traffic environments

After:

  • Warning logged once at startup for disabled modules with detailed context
  • Zero warnings during request processing - clean logs
  • Operators can easily toggle modules via enabled flag without modifying execution plans

✅ All existing tests pass - No breaking changes
✅ All new tests pass - New functionality verified
✅ Code formatting correct - Passes gofmt -s
✅ No vet issues - Passes go vet
✅ Project builds successfully - Compiles without errors
✅ Handles edge cases - Empty plans, nil repos, missing hooks

@postindustria-code postindustria-code changed the title Validate execution plans at startup to remove per-request warnings Modules: Validate execution plans at startup to remove per-request warnings Jan 21, 2026
antoxas1986
antoxas1986 previously approved these changes Jan 28, 2026
Copy link

@antoxas1986 antoxas1986 left a comment

Choose a reason for hiding this comment

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

Code Review - Minor Suggestions

Great PR! The implementation successfully addresses the log noise issue. Here are some minor suggestions to improve code clarity:

1. Variable Shadowing in router/router.go

File: router/router.go (lines 218-221)

Current code:

if errs = hooks.ValidateExecutionPlan(cfg.Hooks, repo); len(errs) > 0 {
    for _, err = range errs {
        logger.Warnf("%s", err.Error())
    }
}

Suggested:

if validationErrs := hooks.ValidateExecutionPlan(cfg.Hooks, repo); len(validationErrs) > 0 {
    for _, validationErr := range validationErrs {
        logger.Warnf("%v", validationErr)
    }
}

Rationale:

  • Avoids shadowing the outer err variable
  • Simplifies string formatting (%v instead of %s with .Error())

2. Add Clarifying Comment in hooks/plan.go

File: hooks/plan.go (around line 209, before the loop)

Suggested addition:

// Hook lookup and assembly. Hooks not found in repository are silently skipped
// as they are reported at startup via ValidateExecutionPlan().
for _, hookCfg := range cfg.HookSequence {
    if h, ok := getHookFn(hookCfg.ModuleCode); ok {
        group.Hooks = append(group.Hooks, HookWrapper[T]{
            Module: hookCfg.ModuleCode,
            Hook:   h,
        })
    }
}

Rationale: Makes it clear why hooks are silently skipped (they're already validated at startup), which helps future maintainers understand the design decision.


Summary

These are non-blocking suggestions that improve code clarity. The PR is ready to merge as-is if you prefer to keep the current implementation.

Overall: LGTM

@postindustria-code
Copy link
Contributor Author

@antoxas1986 Thanks for the review! I’ve fixed the minor nits. Need a re-approve.

Copy link

@antoxas1986 antoxas1986 left a comment

Choose a reason for hiding this comment

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

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants