Do not include hooks from disabled modules into execution plan

[justadreamer]

Issue

Addresses #3748

How to reproduce

Here is a pbs.yaml config file to reproduce the problem described in the issue on the main branch:

gdpr:
  default_value: "0"
hooks:
  enabled: true
  modules:
    prebid:
      ortb2blocking:
        enabled: false

  host_execution_plan:
    endpoints:
      "/openrtb2/auction":
        stages:
          entrypoint:
            groups:
              timeout: 100 #ms
              hook_sequence:
          bidder_request:
            groups:
              timeout: 100 #ms
              hook_sequence:
                - module_code: "prebid.ortb2blocking"
                  hook_impl_code: "prebid.ortb2blocking"
          raw_bidder_response:
            groups:
              timeout: 50
              hook_sequence:
                - module_code: "prebid.ortb2blocking"
                  hook_impl_code: "prebid.ortb2blocking"

The server should be run with

go run main.go -alsologtostderr 

Then on every request like

curl --header "Content-Type: application/json" http://localhost:8000/openrtb2/auction --data '{"imp": [{"ext": {"prebid": {"bidder": {"appnexus": {"placement_id": 1,"use_pmt_rule": false}}}},"id": "2529eeea-813e-4da6-838f-f91c28d64867","banner": {"topframe": 1,"format": [{"w": 728,"h": 90}]}}],"site": {"id":"1"},"tmax": 1700,"id":"1"}'

the server it should spit out

Not found hook while building hook execution plan: ...

On the PR branch this does not happen.

Implementation Details

Disabled modules IDs are collected into the array disabledModules and passed down to the plan builder to be consulted when including hooks into the execution plan.

[SyntaxNode]

Closed / Reopened To Re-Trigger Build Actions

[hhhjort]

Am I missing something, or is there no test that a disabled module is excluded from the built plans? I see tests to ensure disabled modules make it on the disabled module list, but no test was added or modified that would fail prior to this change, but pass now that the plans exclude disabled modules.

[SyntaxNode]

Am I missing something, or is there no test that a disabled module is excluded from the built plans? I see tests to ensure disabled modules make it on the disabled module list, but no test was added or modified that would fail prior to this change, but pass now that the plans exclude disabled modules.

Good point. I missed that. There is good test coverage on the generation of the disabled modules slice.

[justadreamer]

Good catch, thanks, I'll look into adding one.

[justadreamer]

To produce the test mentioned above by @hhhjort - we need a Dependency Injection mechanism to inject a test logger.
The main intention of the issue this PR addresses is the absence of logged warnings. I've added a demo of a lightweight DI, but I think it would be beneficial to introduce this for logger project-wise - that way we will make the logs testable, and glog dependency encapsulated and easily replaceable by slog or whatever else if we need to in the future. I think di/di.go should be renamed into di/logger.go and other dependencies can follow suite if needed. Let me know if you'd like to discuss over slack or otherwise I can introduce a separate PR that'd encapsulate glog and route all calls to it through di.Logger

[hhhjort]

Modifying a package global value is always a scary thing to see. We will want a more formal dependency injection mechanism, on the other hand that would greatly expand the scope of the PR. At least the global is only modified in the test. On the down side, go can run multiple tests in parallel, which could lead to the test failing when some other test logs a warning. I would assume the other PR you are thinking of would resolve this. @SyntaxNode , what do you think of this?

[justadreamer]

Hi @hhhjort thanks for the comment. I've submitted a proposal: #4084 and a PoC PR: #4085 to illustrate the point. I think the lightweight DI mechanism would do for logger even though it is a global state. The alternative would be to use some DI framework of which Wire might be the most preferable as it is compile-time and uses code generation, while others use reflection. However I think the approach with di/providers that can be conditionally compiled using build tags can suffice for now.