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.

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

Do not include hooks from disabled modules into execution plan #3829

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
35 changes: 24 additions & 11 deletions hooks/plan.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package hooks

import (
"slices"
"time"

"github.com/golang/glog"
Expand Down Expand Up @@ -69,11 +70,12 @@ type HookWrapper[T any] struct {

// NewExecutionPlanBuilder returns a new instance of the ExecutionPlanBuilder interface.
// Depending on the hooks' status, method returns a real PlanBuilder or the EmptyPlanBuilder.
func NewExecutionPlanBuilder(hooks config.Hooks, repo HookRepository) ExecutionPlanBuilder {
func NewExecutionPlanBuilder(hooks config.Hooks, repo HookRepository, disabledModules []string) ExecutionPlanBuilder {
if hooks.Enabled {
return PlanBuilder{
hooks: hooks,
repo: repo,
hooks: hooks,
repo: repo,
disabledModules: disabledModules,
}
}
return EmptyPlanBuilder{}
Expand All @@ -82,8 +84,9 @@ func NewExecutionPlanBuilder(hooks config.Hooks, repo HookRepository) ExecutionP
// PlanBuilder is a concrete implementation of the ExecutionPlanBuilder interface.
// Which returns hook execution plans for specific stage defined by the hook config.
type PlanBuilder struct {
hooks config.Hooks
repo HookRepository
hooks config.Hooks
repo HookRepository
disabledModules []string
}

func (p PlanBuilder) PlanForEntrypointStage(endpoint string) Plan[hookstage.Entrypoint] {
Expand All @@ -93,6 +96,7 @@ func (p PlanBuilder) PlanForEntrypointStage(endpoint string) Plan[hookstage.Entr
endpoint,
StageEntrypoint,
p.repo.GetEntrypointHook,
p.disabledModules,
)
}

Expand All @@ -103,6 +107,7 @@ func (p PlanBuilder) PlanForRawAuctionStage(endpoint string, account *config.Acc
endpoint,
StageRawAuctionRequest,
p.repo.GetRawAuctionHook,
p.disabledModules,
)
}

Expand All @@ -113,6 +118,7 @@ func (p PlanBuilder) PlanForProcessedAuctionStage(endpoint string, account *conf
endpoint,
StageProcessedAuctionRequest,
p.repo.GetProcessedAuctionHook,
p.disabledModules,
)
}

Expand All @@ -123,6 +129,7 @@ func (p PlanBuilder) PlanForBidderRequestStage(endpoint string, account *config.
endpoint,
StageBidderRequest,
p.repo.GetBidderRequestHook,
p.disabledModules,
)
}

Expand All @@ -133,6 +140,7 @@ func (p PlanBuilder) PlanForRawBidderResponseStage(endpoint string, account *con
endpoint,
StageRawBidderResponse,
p.repo.GetRawBidderResponseHook,
p.disabledModules,
)
}

Expand All @@ -143,6 +151,7 @@ func (p PlanBuilder) PlanForAllProcessedBidResponsesStage(endpoint string, accou
endpoint,
StageAllProcessedBidResponses,
p.repo.GetAllProcessedBidResponsesHook,
p.disabledModules,
)
}

Expand All @@ -153,6 +162,7 @@ func (p PlanBuilder) PlanForAuctionResponseStage(endpoint string, account *confi
endpoint,
StageAuctionResponse,
p.repo.GetAuctionResponseHook,
p.disabledModules,
)
}

Expand All @@ -164,22 +174,23 @@ func getMergedPlan[T any](
endpoint string,
stage Stage,
getHookFn hookFn[T],
disabledModules []string,
) Plan[T] {
accountPlan := cfg.DefaultAccountExecutionPlan
if account != nil && account.Hooks.ExecutionPlan.Endpoints != nil {
accountPlan = account.Hooks.ExecutionPlan
}

plan := getPlan(getHookFn, cfg.HostExecutionPlan, endpoint, stage)
plan = append(plan, getPlan(getHookFn, accountPlan, endpoint, stage)...)
plan := getPlan(getHookFn, cfg.HostExecutionPlan, disabledModules, endpoint, stage)
plan = append(plan, getPlan(getHookFn, accountPlan, disabledModules, endpoint, stage)...)

return plan
}

func getPlan[T any](getHookFn hookFn[T], cfg config.HookExecutionPlan, endpoint string, stage Stage) Plan[T] {
func getPlan[T any](getHookFn hookFn[T], cfg config.HookExecutionPlan, disabledModules []string, endpoint string, stage Stage) Plan[T] {
plan := make(Plan[T], 0, len(cfg.Endpoints[endpoint].Stages[stage.String()].Groups))
for _, groupCfg := range cfg.Endpoints[endpoint].Stages[stage.String()].Groups {
group := getGroup(getHookFn, groupCfg)
group := getGroup(getHookFn, groupCfg, disabledModules)
if len(group.Hooks) > 0 {
plan = append(plan, group)
}
Expand All @@ -188,13 +199,15 @@ func getPlan[T any](getHookFn hookFn[T], cfg config.HookExecutionPlan, endpoint
return plan
}

func getGroup[T any](getHookFn hookFn[T], cfg config.HookExecutionGroup) Group[T] {
func getGroup[T any](getHookFn hookFn[T], cfg config.HookExecutionGroup, disabledModules []string) Group[T] {
group := Group[T]{
Timeout: time.Duration(cfg.Timeout) * time.Millisecond,
Hooks: make([]HookWrapper[T], 0, len(cfg.HookSequence)),
}

for _, hookCfg := range cfg.HookSequence {
if slices.Contains(disabledModules, hookCfg.ModuleCode) {
continue
}
if h, ok := getHookFn(hookCfg.ModuleCode); ok {
group.Hooks = append(group.Hooks, HookWrapper[T]{Module: hookCfg.ModuleCode, Code: hookCfg.HookImplCode, Hook: h})
} else {
Expand Down
4 changes: 2 additions & 2 deletions hooks/plan_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ func TestNewExecutionPlanBuilder(t *testing.T) {

for name, test := range testCases {
t.Run(name, func(t *testing.T) {
gotPlanBuilder := NewExecutionPlanBuilder(test.givenConfig, nil)
gotPlanBuilder := NewExecutionPlanBuilder(test.givenConfig, nil, nil)
assert.Equal(t, test.expectedPlanBuilder, gotPlanBuilder)
})
}
Expand Down Expand Up @@ -798,7 +798,7 @@ func getPlanBuilder(
return nil, err
}

return NewExecutionPlanBuilder(hooks, repo), nil
return NewExecutionPlanBuilder(hooks, repo, []string{}), nil
}

type fakeEntrypointHook struct{}
Expand Down
17 changes: 9 additions & 8 deletions modules/modules.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ type Builder interface {
// It returns hook repository created based on the implemented hook interfaces by modules
// and a map of modules to a list of stage names for which module provides hooks
// or an error encountered during module initialization.
Build(cfg config.Modules, client moduledeps.ModuleDeps) (hooks.HookRepository, map[string][]string, error)
Build(cfg config.Modules, client moduledeps.ModuleDeps) (hooks.HookRepository, map[string][]string, []string, error)
}

type (
Expand All @@ -49,8 +49,10 @@ type builder struct {
func (m *builder) Build(
cfg config.Modules,
deps moduledeps.ModuleDeps,
) (hooks.HookRepository, map[string][]string, error) {
) (hooks.HookRepository, map[string][]string, []string, error) {
modules := make(map[string]interface{})
modulesDisabledId := []string{}

for vendor, moduleBuilders := range m.builders {
for moduleName, builder := range moduleBuilders {
var err error
Expand All @@ -60,7 +62,7 @@ func (m *builder) Build(
id := fmt.Sprintf("%s.%s", vendor, moduleName)
if data, ok := cfg[vendor][moduleName]; ok {
if conf, err = jsonutil.Marshal(data); err != nil {
return nil, nil, fmt.Errorf(`failed to marshal "%s" module config: %s`, id, err)
return nil, nil, nil, fmt.Errorf(`failed to marshal "%s" module config: %s`, id, err)
}

if values, ok := data.(map[string]interface{}); ok {
Expand All @@ -69,15 +71,15 @@ func (m *builder) Build(
}
}
}

if !isEnabled {
modulesDisabledId = append(modulesDisabledId, id)
glog.Infof("Skip %s module, disabled.", id)
continue
}

module, err := builder(conf, deps)
if err != nil {
return nil, nil, fmt.Errorf(`failed to init "%s" module: %s`, id, err)
return nil, nil, nil, fmt.Errorf(`failed to init "%s" module: %s`, id, err)
}

modules[id] = module
Expand All @@ -86,10 +88,9 @@ func (m *builder) Build(

collection, err := createModuleStageNamesCollection(modules)
if err != nil {
return nil, nil, err
return nil, nil, nil, err
}

repo, err := hooks.NewHookRepository(modules)

return repo, collection, err
return repo, collection, modulesDisabledId, err
}
95 changes: 52 additions & 43 deletions modules/modules_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,62 +30,70 @@ func TestModuleBuilderBuild(t *testing.T) {
}

testCases := map[string]struct {
givenModule interface{}
givenConfig config.Modules
givenHookBuilderErr error
expectedHookRepo hooks.HookRepository
expectedModulesStages map[string][]string
expectedErr error
givenModule interface{}
givenConfig config.Modules
givenHookBuilderErr error
expectedHookRepo hooks.HookRepository
expectedModulesStages map[string][]string
expectedModulesDisabled []string
expectedErr error
}{
"Can build module with config": {
givenModule: module{},
givenConfig: defaultModulesConfig,
expectedModulesStages: map[string][]string{vendor + "_" + moduleName: {hooks.StageEntrypoint.String(), hooks.StageAuctionResponse.String()}},
expectedHookRepo: defaultHookRepository,
expectedErr: nil,
givenModule: module{},
givenConfig: defaultModulesConfig,
expectedModulesStages: map[string][]string{vendor + "_" + moduleName: {hooks.StageEntrypoint.String(), hooks.StageAuctionResponse.String()}},
expectedModulesDisabled: []string{},
expectedHookRepo: defaultHookRepository,
expectedErr: nil,
},
"Module is not added to hook repository if it's disabled": {
givenModule: module{},
givenConfig: map[string]map[string]interface{}{vendor: {moduleName: map[string]interface{}{"enabled": false, "attr": "val"}}},
expectedModulesStages: map[string][]string{},
expectedHookRepo: emptyHookRepository,
expectedErr: nil,
givenModule: module{},
givenConfig: map[string]map[string]interface{}{vendor: {moduleName: map[string]interface{}{"enabled": false, "attr": "val"}}},
expectedModulesStages: map[string][]string{},
expectedModulesDisabled: []string{fmt.Sprintf("%s.%s", vendor, moduleName)},
expectedHookRepo: emptyHookRepository,
expectedErr: nil,
},
"Module considered disabled if status property not defined in module config": {
givenModule: module{},
givenConfig: map[string]map[string]interface{}{vendor: {moduleName: map[string]interface{}{"foo": "bar"}}},
expectedHookRepo: emptyHookRepository,
expectedModulesStages: map[string][]string{},
expectedErr: nil,
givenModule: module{},
givenConfig: map[string]map[string]interface{}{vendor: {moduleName: map[string]interface{}{"foo": "bar"}}},
expectedHookRepo: emptyHookRepository,
expectedModulesStages: map[string][]string{},
expectedModulesDisabled: []string{fmt.Sprintf("%s.%s", vendor, moduleName)},
expectedErr: nil,
},
"Module considered disabled if its config not provided and as a result skipped from execution": {
givenModule: module{},
givenConfig: nil,
expectedHookRepo: emptyHookRepository,
expectedModulesStages: map[string][]string{},
expectedErr: nil,
givenModule: module{},
givenConfig: nil,
expectedHookRepo: emptyHookRepository,
expectedModulesStages: map[string][]string{},
expectedModulesDisabled: []string{fmt.Sprintf("%s.%s", vendor, moduleName)},
expectedErr: nil,
},
"Fails if module does not implement any hook interface": {
givenModule: struct{}{},
givenConfig: defaultModulesConfig,
expectedHookRepo: nil,
expectedModulesStages: nil,
expectedErr: fmt.Errorf(`hook "%s.%s" does not implement any supported hook interface`, vendor, moduleName),
givenModule: struct{}{},
givenConfig: defaultModulesConfig,
expectedHookRepo: nil,
expectedModulesStages: nil,
expectedModulesDisabled: nil,
expectedErr: fmt.Errorf(`hook "%s.%s" does not implement any supported hook interface`, vendor, moduleName),
},
"Fails if module builder function returns error": {
givenModule: module{},
givenConfig: defaultModulesConfig,
givenHookBuilderErr: errors.New("failed to build module"),
expectedHookRepo: nil,
expectedModulesStages: nil,
expectedErr: fmt.Errorf(`failed to init "%s.%s" module: %s`, vendor, moduleName, "failed to build module"),
givenModule: module{},
givenConfig: defaultModulesConfig,
givenHookBuilderErr: errors.New("failed to build module"),
expectedHookRepo: nil,
expectedModulesStages: nil,
expectedModulesDisabled: nil,
expectedErr: fmt.Errorf(`failed to init "%s.%s" module: %s`, vendor, moduleName, "failed to build module"),
},
"Fails if config marshaling returns error": {
givenModule: module{},
givenConfig: map[string]map[string]interface{}{vendor: {moduleName: math.Inf(1)}},
expectedHookRepo: nil,
expectedModulesStages: nil,
expectedErr: fmt.Errorf(`failed to marshal "%s.%s" module config: unsupported value: +Inf`, vendor, moduleName),
givenModule: module{},
givenConfig: map[string]map[string]interface{}{vendor: {moduleName: math.Inf(1)}},
expectedHookRepo: nil,
expectedModulesStages: nil,
expectedModulesDisabled: nil,
expectedErr: fmt.Errorf(`failed to marshal "%s.%s" module config: unsupported value: +Inf`, vendor, moduleName),
},
}

Expand All @@ -101,9 +109,10 @@ func TestModuleBuilderBuild(t *testing.T) {
},
}

repo, modulesStages, err := builder.Build(test.givenConfig, moduledeps.ModuleDeps{HTTPClient: http.DefaultClient})
repo, modulesStages, modulesDisabled, err := builder.Build(test.givenConfig, moduledeps.ModuleDeps{HTTPClient: http.DefaultClient})
assert.Equal(t, test.expectedErr, err)
assert.Equal(t, test.expectedModulesStages, modulesStages)
assert.Equal(t, test.expectedModulesDisabled, modulesDisabled)
assert.Equal(t, test.expectedHookRepo, repo)
})
}
Expand Down
4 changes: 2 additions & 2 deletions router/router.go
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ func New(cfg *config.Configuration, rateConvertor *currency.RateConverter) (r *R
}

moduleDeps := moduledeps.ModuleDeps{HTTPClient: generalHttpClient, RateConvertor: rateConvertor}
repo, moduleStageNames, err := modules.NewBuilder().Build(cfg.Hooks.Modules, moduleDeps)
repo, moduleStageNames, moduleDisabledCodes, err := modules.NewBuilder().Build(cfg.Hooks.Modules, moduleDeps)
if err != nil {
glog.Fatalf("Failed to init hook modules: %v", err)
}
Expand Down Expand Up @@ -240,7 +240,7 @@ func New(cfg *config.Configuration, rateConvertor *currency.RateConverter) (r *R
priceFloorFetcher := floors.NewPriceFloorFetcher(cfg.PriceFloors, floorFechterHttpClient, r.MetricsEngine)

tmaxAdjustments := exchange.ProcessTMaxAdjustments(cfg.TmaxAdjustments)
planBuilder := hooks.NewExecutionPlanBuilder(cfg.Hooks, repo)
planBuilder := hooks.NewExecutionPlanBuilder(cfg.Hooks, repo, moduleDisabledCodes)
macroReplacer := macros.NewStringIndexBasedReplacer()
theExchange := exchange.NewExchange(adapters, cacheClient, cfg, requestValidator, syncersByBidder, r.MetricsEngine, cfg.BidderInfos, gdprPermsBuilder, rateConvertor, categoriesFetcher, adsCertSigner, macroReplacer, priceFloorFetcher)
var uuidGenerator uuidutil.UUIDRandomGenerator
Expand Down
Loading