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

Greenbids: Add account level config for modules RTD, analytics #3596

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

EvgeniiMunin
Copy link
Contributor

@EvgeniiMunin EvgeniiMunin commented Dec 5, 2024

🔧 Type of changes

  • new bid adapter
  • bid adapter update
  • new feature
  • new analytics adapter
  • new module
  • module update
  • bugfix
  • documentation
  • configuration
  • dependency update
  • tech debt (test coverage, refactorings, etc.)

✨ What's the context?

As discussed with @bretg and @jbogp we need to add extraction of modules params from account level config for both RTD module and analytics reporter. At the same time if the params are defined in BidRequestExt it should take precedent before account level config. This makes modules integration more publisher friendly

🧠 Rationale behind the change

account/id identifies account for specific publisher -> so if we define a config for a specific account it means it should correspond to the unique publisher id = pbuid.

Setup of default-account-config for Greenbids example in prebid-config-with-module.yaml should look like this

settings:
  enforce-valid-account: false
  generate-storedrequest-bidrequest-id: true
  filesystem:
    settings-filename: sample/configs/sample-app-settings.yaml
    stored-requests-dir: sample
    stored-imps-dir: sample
    stored-responses-dir: sample
    categories-dir:
  default-account-config: >
    {
      "auction": {
        "price-floors": {
          "enabled": true,
          "fetch": {
            "enabled": false,
            "timeout-ms": 5000,
            "max-rules": 0,
            "max-file-size-kb": 200,
            "max-age-sec": 86400,
            "period-sec": 3600
          },
          "enforce-floors-rate": 100,
          "adjust-for-bid-adjustment": true,
          "enforce-deal-floors": true,
          "use-dynamic-data": true
        }
      },
      "hooks": {
          "modules": {
              "greenbids": {
                  "pbuid": "PBUID_FROM_GREENBIDS",
                  "targetTpr": 0.95,
                  "explorationRate": 0.001
              }
          }
      },
      "analytics": {
          "modules": {
              "greenbids": {
                  "pbuid": "PBUID_FROM_GREENBIDS",
                  "greenbidsSampling": 0.001
                }
            }
        }
    }

The modules can extract fields of Account class from AuctionContext at hook call (not on spring boot)

Optional.ofNullable(invocationContext)
	.map(AuctionInvocationContext::auctionContext)
	.map(AuctionContext::getAccount)

Then depending on the calling module we get analytics or RTD Map[String, ObjectNode] modules and parse it from json.

🔎 New Bid Adapter Checklist

  • verify email contact works
  • NO fully dynamic hostnames
  • geographic host parameters are NOT required
  • direct use of HTTP is prohibited - implement an existing Bidder interface that will do all the job
  • if the ORTB is just forwarded to the endpoint, use the generic adapter - define the new adapter as the alias of the generic adapter
  • cover an adapter configuration with an integration test

🧪 Test plan

I have introduced a specific test case in Greenbids analytics reporter test to fallback to account level config if bid request ext config is not defined: shouldReceiveValidResponseAndFallbackToAccountLevelConfigWhenPartnerNotActivatedInBidRequest

For RTD module have modified the existing test case when partner not activated on bid request ext -> then fallback to account level config and proceed with the filtering callShouldFilterBiddersAndFallbackToAccountLevelConfigWhenPartnerNotActivatedInBidRequest

🏎 Quality check

  • Are your changes following our code style guidelines?
  • Are there any breaking changes in your code?
  • Does your test coverage exceed 90%?
  • Are there any erroneous console logs, debuggers or leftover code in your changes?

@EvgeniiMunin EvgeniiMunin marked this pull request as ready for review December 10, 2024 11:00
@EvgeniiMunin EvgeniiMunin changed the title Greenbids RTD Accoung level config Greenbids: Add account level config for modules RTD, analytics Dec 10, 2024
Comment on lines 95 to 108
private Partner parseAccountConfig(AuctionContext auctionContext) {
final Map<String, ObjectNode> modules = Optional.ofNullable(auctionContext)
.map(AuctionContext::getAccount)
.map(Account::getHooks)
.map(AccountHooksConfiguration::getModules)
.orElse(null);
}

private boolean isNotEmptyObjectNode(JsonNode analytics) {
return analytics != null && analytics.isObject() && !analytics.isEmpty();
Partner partner = null;
if (modules != null && modules.containsKey("greenbids")) {
final ObjectNode moduleConfig = modules.get("greenbids");
partner = toPartner(moduleConfig);
}

return partner;
Copy link
Collaborator

Choose a reason for hiding this comment

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

you can skip this parsing, and just call toPartner(invocationContext.accountConfig()) and you'll get what you need (of course some null checks and json parsing exception should be handled)

also if the Partner is considered to be a representation of the account config, I suggest rename it to the Config or GreenbidsConfig (something like that)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modified to

private GreenbidsConfig parseAccountConfig(Account account) {
    return Optional.ofNullable(account)
            .map(Account::getHooks)
            .map(AccountHooksConfiguration::getModules)
            .map(modules -> modules.get(name()))
            .map(this::toGreenbidsConfig)
            .orElse(null);
}

private GreenbidsConfig toGreenbidsConfig(ObjectNode adapterNode) {
    try {
        return mapper.treeToValue(adapterNode, GreenbidsConfig.class);
    } catch (JsonProcessingException e) {
        return null;
    }
}

The NPE is handled by the optional chain and JsonProcessingException is handled in toGreenbidsConfig

Copy link
Collaborator

Choose a reason for hiding this comment

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

You haven't followed my suggestion. The idea of the account config is that the module has it already as the invocationContext.accountConfig()

Copy link
Collaborator

Choose a reason for hiding this comment

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

@osulzhenko
Copy link
Collaborator

Depends on PR: #3596

@osulzhenko
Copy link
Collaborator

@EvgeniiMunin fix was merged, this PR should be unlocked

@EvgeniiMunin EvgeniiMunin force-pushed the greenbids-rtd-account-level-config branch from 9216dfd to f155ee3 Compare January 22, 2025 12:14
final int hashInt = Integer.parseInt(
greenbidsId.substring(greenbidsId.length() - 4), 16);
return hashInt < partner.getExplorationRate() * RANGE_16_BIT_INTEGER_DIVISION_BASIS;
return hashInt < greenbidsConfig.getExplorationRate() * RANGE_16_BIT_INTEGER_DIVISION_BASIS;
Copy link
Collaborator

Choose a reason for hiding this comment

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

as I can see it's never checked that properties of configs are required or can't be null, so this is a NPE potentially

check other properties as well and handle them if it's necessary

Comment on lines +80 to +81
final GreenbidsConfig greenbidsConfig = Optional.ofNullable(parseBidRequestExt(auctionContext))
.orElse(parseAccountConfig(auctionContext.getAccount()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the execution shouldn't proceed of the config is null

yes, it shouldn't be the problem if the account config is required to invoke the module, but I guess it's still better to return the failed future in the null case

@@ -131,7 +134,7 @@ public <T> Future<Void> processEvent(T event) {

final String greenbidsId = greenbidsId(analyticsResultFromAnalyticsTag);

if (!isSampled(greenbidsBidRequestExt.getGreenbidsSampling(), greenbidsId)) {
if (!isSampled(greenbidsConfig.getGreenbidsSampling(), greenbidsId)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

make sure the nullable properties won't cause NPE if they're underconfigured

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants