From ce8f68fc09447bc17da7fac3cf9a44edbd01a347 Mon Sep 17 00:00:00 2001 From: AntonYPost Date: Mon, 29 Jul 2024 14:56:45 +0300 Subject: [PATCH 1/3] Skip hooks build for disabled modules --- hooks/plan.go | 35 ++++++++++----- hooks/plan_test.go | 4 +- modules/modules.go | 17 ++++---- modules/modules_test.go | 95 ++++++++++++++++++++++------------------- router/router.go | 4 +- 5 files changed, 89 insertions(+), 66 deletions(-) diff --git a/hooks/plan.go b/hooks/plan.go index a3a0e9af66..b2c974d2b3 100644 --- a/hooks/plan.go +++ b/hooks/plan.go @@ -1,6 +1,7 @@ package hooks import ( + "slices" "time" "github.com/golang/glog" @@ -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{} @@ -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] { @@ -93,6 +96,7 @@ func (p PlanBuilder) PlanForEntrypointStage(endpoint string) Plan[hookstage.Entr endpoint, StageEntrypoint, p.repo.GetEntrypointHook, + p.disabledModules, ) } @@ -103,6 +107,7 @@ func (p PlanBuilder) PlanForRawAuctionStage(endpoint string, account *config.Acc endpoint, StageRawAuctionRequest, p.repo.GetRawAuctionHook, + p.disabledModules, ) } @@ -113,6 +118,7 @@ func (p PlanBuilder) PlanForProcessedAuctionStage(endpoint string, account *conf endpoint, StageProcessedAuctionRequest, p.repo.GetProcessedAuctionHook, + p.disabledModules, ) } @@ -123,6 +129,7 @@ func (p PlanBuilder) PlanForBidderRequestStage(endpoint string, account *config. endpoint, StageBidderRequest, p.repo.GetBidderRequestHook, + p.disabledModules, ) } @@ -133,6 +140,7 @@ func (p PlanBuilder) PlanForRawBidderResponseStage(endpoint string, account *con endpoint, StageRawBidderResponse, p.repo.GetRawBidderResponseHook, + p.disabledModules, ) } @@ -143,6 +151,7 @@ func (p PlanBuilder) PlanForAllProcessedBidResponsesStage(endpoint string, accou endpoint, StageAllProcessedBidResponses, p.repo.GetAllProcessedBidResponsesHook, + p.disabledModules, ) } @@ -153,6 +162,7 @@ func (p PlanBuilder) PlanForAuctionResponseStage(endpoint string, account *confi endpoint, StageAuctionResponse, p.repo.GetAuctionResponseHook, + p.disabledModules, ) } @@ -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) } @@ -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 { diff --git a/hooks/plan_test.go b/hooks/plan_test.go index 064403cb8c..f36f394f1e 100644 --- a/hooks/plan_test.go +++ b/hooks/plan_test.go @@ -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) }) } @@ -798,7 +798,7 @@ func getPlanBuilder( return nil, err } - return NewExecutionPlanBuilder(hooks, repo), nil + return NewExecutionPlanBuilder(hooks, repo, []string{}), nil } type fakeEntrypointHook struct{} diff --git a/modules/modules.go b/modules/modules.go index f3ccd6b1ec..d8540e5639 100644 --- a/modules/modules.go +++ b/modules/modules.go @@ -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 ( @@ -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 @@ -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 { @@ -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 @@ -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 } diff --git a/modules/modules_test.go b/modules/modules_test.go index 008c1e75c5..a6bc6daced 100644 --- a/modules/modules_test.go +++ b/modules/modules_test.go @@ -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), }, } @@ -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) }) } diff --git a/router/router.go b/router/router.go index 0712f6723d..7d86a9e2c6 100644 --- a/router/router.go +++ b/router/router.go @@ -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) } @@ -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 From 0af99e590b54fac97ceeb65883fe032dfcb4c882 Mon Sep 17 00:00:00 2001 From: Eugene Dorfman Date: Tue, 26 Nov 2024 14:14:49 +0100 Subject: [PATCH 2/3] introduce DI for logging, test warnings produced by plan.go --- di/di.go | 18 +++++++++ hooks/plan.go | 4 +- modules/modules_test.go | 89 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 109 insertions(+), 2 deletions(-) create mode 100644 di/di.go diff --git a/di/di.go b/di/di.go new file mode 100644 index 0000000000..a12140f33a --- /dev/null +++ b/di/di.go @@ -0,0 +1,18 @@ +package di + +import ( + "github.com/golang/glog" +) + +type ILogger interface { + Warningf(format string, args ...interface{}) +} + +type DefaultLogger struct{} + +func (d *DefaultLogger) Warningf(format string, args ...interface{}) { + glog.Warningf(format, args...) +} + +var defaultLogger = DefaultLogger{} +var Logger ILogger = &defaultLogger diff --git a/hooks/plan.go b/hooks/plan.go index b0bedfb9f8..c5b4ac5c8c 100644 --- a/hooks/plan.go +++ b/hooks/plan.go @@ -4,8 +4,8 @@ import ( "slices" "time" - "github.com/golang/glog" "github.com/prebid/prebid-server/v3/config" + "github.com/prebid/prebid-server/v3/di" "github.com/prebid/prebid-server/v3/hooks/hookstage" ) @@ -211,7 +211,7 @@ func getGroup[T any](getHookFn hookFn[T], cfg config.HookExecutionGroup, disable if h, ok := getHookFn(hookCfg.ModuleCode); ok { group.Hooks = append(group.Hooks, HookWrapper[T]{Module: hookCfg.ModuleCode, Code: hookCfg.HookImplCode, Hook: h}) } else { - glog.Warningf("Not found hook while building hook execution plan: %s %s", hookCfg.ModuleCode, hookCfg.HookImplCode) + di.Logger.Warningf("Not found hook while building hook execution plan: %s %s", hookCfg.ModuleCode, hookCfg.HookImplCode) } } diff --git a/modules/modules_test.go b/modules/modules_test.go index e916efa688..2ed57b13b1 100644 --- a/modules/modules_test.go +++ b/modules/modules_test.go @@ -5,6 +5,8 @@ import ( "encoding/json" "errors" "fmt" + "github.com/prebid/prebid-server/v3/di" + "github.com/prebid/prebid-server/v3/util/jsonutil" "math" "net/http" "testing" @@ -118,6 +120,93 @@ func TestModuleBuilderBuild(t *testing.T) { } } +func TestPlanForDisabledModule(t *testing.T) { + testCases := map[string]struct { + moduleCode string + enabled bool + expectedPlanLen int + expectedLogOutput string + }{ + "Correct module_code and module enabled = plan contains one hook": { + moduleCode: "prebid.ortb2blocking", + enabled: true, + expectedPlanLen: 1, + expectedLogOutput: "", + }, + "Incorrect module_code but module enabled = plan contains no hooks": { + moduleCode: "prebid_ortb2blocking", + enabled: true, + expectedPlanLen: 0, + expectedLogOutput: "Not found hook while building hook execution plan: prebid_ortb2blocking foo", + }, + "Correct module_code but module disabled = plan contains no hooks": { + moduleCode: "prebid.ortb2blocking", + enabled: false, + expectedPlanLen: 0, + expectedLogOutput: "", + }, + } + + old_logger := di.Logger + testLogger := TestLogger{} + di.Logger = &testLogger + defer func() { di.Logger = old_logger }() + + for name, test := range testCases { + t.Run(name, func(t *testing.T) { + testLogger.log = "" + hooksCfgData, accountCfgData := constructCfg(test.moduleCode, test.enabled) + var hooksCfg config.Hooks + var accountCfg config.Account + var err = jsonutil.UnmarshalValid([]byte(hooksCfgData), &hooksCfg) + assert.Nil(t, err) + + err = jsonutil.UnmarshalValid([]byte(accountCfgData), &accountCfg) + assert.Nil(t, err) + + planBuilder, err := constructPlanBuilder(hooksCfg) + assert.Nil(t, err) + + plan := planBuilder.PlanForRawBidderResponseStage("/openrtb2/auction", &accountCfg) + assert.Equal(t, test.expectedPlanLen, len(plan)) + assert.Equal(t, test.expectedLogOutput, testLogger.log) + + }) + } +} + +type TestLogger struct { + log string +} + +func (logger *TestLogger) Warningf(format string, args ...interface{}) { + logger.log = logger.log + fmt.Sprintf(format, args...) +} + +func constructCfg(module_code string, enabled bool) (string, string) { + group := `{"timeout": 5, "hook_sequence": [{"module_code": "` + module_code + `", "hook_impl_code": "foo"}]}` + executionPlanData := `{"endpoints": {"/openrtb2/auction": {"stages": {"raw_bidder_response": {"groups": [` + group + `]}}}}}` + enabledS := `false` + if enabled { + enabledS = `true` + } + modules := `"modules": {"prebid": {"ortb2blocking": {"enabled": ` + enabledS + `}}}` + hooksCfgData := `{"enabled":true, ` + modules + `, "execution_plan": ` + executionPlanData + `}` + accountCfgData := `{"hooks":` + hooksCfgData + `}` + return hooksCfgData, accountCfgData +} + +func constructPlanBuilder(cfgHooks config.Hooks) (hooks.ExecutionPlanBuilder, error) { + moduleDeps := moduledeps.ModuleDeps{} + repo, _, disabledModuleCodes, err := NewBuilder().Build(cfgHooks.Modules, moduleDeps) + if err != nil { + return nil, err + } + + planBuilder := hooks.NewExecutionPlanBuilder(cfgHooks, repo, disabledModuleCodes) + return planBuilder, nil +} + type module struct{} func (h module) HandleEntrypointHook(_ context.Context, _ hookstage.ModuleInvocationContext, _ hookstage.EntrypointPayload) (hookstage.HookResult[hookstage.EntrypointPayload], error) { From c15063f4f1c62abf3425a6ea55c16636fd7b0e95 Mon Sep 17 00:00:00 2001 From: Eugene Dorfman Date: Tue, 26 Nov 2024 14:51:35 +0100 Subject: [PATCH 3/3] rename di/di.go -> di/logger.go --- di/{di.go => logger.go} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename di/{di.go => logger.go} (100%) diff --git a/di/di.go b/di/logger.go similarity index 100% rename from di/di.go rename to di/logger.go