[pkowalski-id5]

🔧 Type of changes

• new module

✨ What's the context?

ID5 wants to have its own module that can enrich bid requests with ID5 universal identifiers. This module fetches identity signals from ID5's API and automatically injects them into OpenRTB bid requests as Extended
Identifiers (EIDs).

🧠 Rationale behind the change

Why a dedicated module?

• ID5 identifiers enhance user matching capabilities across the programmatic ecosystem
• Having a dedicated module allows PBS operators to enable ID5 enrichment without requiring publisher-side changes
• The module respects all major privacy signals (GDPR, CCPA, COPPA, GPP)

Key design decisions:

• Two-hook architecture: Separate fetch and inject hooks to optimize performance
  • Fetch hook (ProcessedAuctionRequestHook): Initiates async ID5 API call early in the request lifecycle
  • Inject hook (BidderRequestHook): Injects EIDs into each bidder request only when not already present
• Non-blocking async implementation: ID5 fetch doesn't block the auction processing
• Preservation of existing IDs: Module checks if ID5 EID already exists before fetching/injecting
• Flexible filtering: Account, country, bidder filters and sampling for gradual rollout
• Configurable partner ID provider: Interface allows custom logic for determining partner ID per request

Trade-offs:

• Requires both hooks to be configured in execution plan (fetch + inject) for the module to function
• Additional HTTP call to ID5 API adds latency (mitigated by async execution and timeout awareness)

🔎 New Bid Adapter Checklist

Not applicable - this is a module, not a bid adapter

🧪 Test plan

Unit Tests:

• Comprehensive unit test coverage for both hooks
• Filter logic tested (account, country, bidder, sampling)
• Privacy signal handling tested (GDPR, CCPA, COPPA, GPP)
• Edge cases covered (timeouts, existing IDs, empty responses)

Integration Tests:

• Full PBS instance integration tests with the module enabled
• Tests verify complete fetch + inject flow
• WireMock-based local testing setup included in sample/ directory

Manual Testing:

• Local end-to-end testing with WireMock mocks
• Sample configurations provided for testing different scenarios
• Debug logging available for troubleshooting

The module has been tested with various configurations and filter combinations to ensure safe production deployment.

🏎 Quality check

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

🤔 Questions for reviewers

1. Hook invocation status/action handling:
We would particularly appreciate review of our hook invocation status and action returns. Please verify that we're correctly returning:

• Appropriate InvocationStatus values in different scenarios (success, failure, skipped)
• Correct InvocationAction to control execution flow
• Proper handling of edge cases (timeouts, errors, existing IDs)

2. Execution plan configuration simplification:
Both hooks (fetch + inject) are required for the module to function - the module is non-functional if either hook is missing from the execution plan. Currently, operators must configure both hooks separately in the
execution plan:

stages:
  processed-auction-request:
    groups:
      - hook-sequence:
          - module-code: "id5-user-id"
            hook-impl-code: "id5-user-id-fetch-hook"
  bidder-request:
    groups:
      - hook-sequence:
          - module-code: "id5-user-id"
            hook-impl-code: "id5-user-id-inject-hook"

Question: Is there a way to simplify this configuration? For example:

• A single module registration that automatically registers both required hooks?
• A validation mechanism that warns when one hook is configured without the other?
• Any other pattern for "atomic" multi-hook modules?

[pkowalski-id5]

@CTMBNara Thank you for the review. I've updated the code. Could you please take another look?

[CTMBNara]

No need for @JsonProperty here. snake_case is a standard for our object mapper

[pkowalski-id5]

Thank you for raising this. It made me realise how tightly coupled and risky it was to rely on the shared mapper.

The global ObjectMapper uses SNAKE_CASE today, but that is an implementation detail of the host application, not a guarantee this module can rely on. What's more, several fields use names that don't follow SNAKE_CASE at all like providerMetadata or _trace. They are defined by the ID5 API schema, not by any naming convention. Removing @JsonProperty and trusting the naming strategy would silently couple the wire format of ID5 requests to the
host configuration. If that ever changes, the breakage would be invisible at compile time and potentially very hard to trace in production.

HttpFetchClient will own a private ObjectMapper configured independently of the application-wide one. Together with explicit @JsonProperty annotations, this guarantees the request is serialized according to the ID5 API schema regardless of any changes to the host configuration.

[pkowalski-id5]

I’ve updated the module based on your suggestions. Whenever you have a chance to re-review this, I’d love to hear your thoughts @Net-burst @CTMBNara

[And1sS]

I’ve updated the module based on your suggestions. Whenever you have a chance to re-review this, I’d love to hear your thoughts @Net-burst @CTMBNara

Hello @pkowalski-id5! Thank you for your contribution. There are still a few comments that need to be addressed before we can proceed with merging your PR.

[pkowalski-id5]

@And1sS thanks for looking at this!

There are still a few comments that need to be addressed before we can proceed with merging your PR.

Can you help me better understand what is missing? Are there any issues not yet submitted, or are some of the submitted not fully resolved and cannot be approved?

[And1sS]

@And1sS thanks for looking at this!

There are still a few comments that need to be addressed before we can proceed with merging your PR.

Can you help me better understand what is missing? Are there any issues not yet submitted, or are some of the submitted not fully resolved and cannot be approved?

It looks like a few remarks from @CTMBNara are still pending. When you have a moment, could you please address them so we can continue with the review process?

[pkowalski-id5]

It looks like a few remarks from @CTMBNara are still pending. When you have a moment, could you please address them so we can continue with the review process?

@And1sS @CTMBNara
I believe I've addressed all the comments, though GitHub might not be highlighting the changes clearly in the main view.

You can find the relevant updates in these commits:

• ef40eab
• fba0705

If I’ve missed anything specific, could you please point out which remarks are still outstanding? I’m happy to take another look

[CTMBNara]

@pkowalski-id5 Please, update your branch with latest master.

[pkowalski-id5]

@CTMBNara, thanks for the review. I've updated the code as suggested. Please verify.
I hope I haven't missed anything

[CTMBNara]

Please use pb-rule-engine as a reference when writing unit tests (style, mocks, assertions, etc.).

[CTMBNara]

Create new Future instance for each invocation. (remove EMPTY constant)

[CTMBNara]

List.of -> Collections.emptyList

[CTMBNara]

replace with @Mock annotation. Same for other occurrences

[CTMBNara]

extend VertxTest

[CTMBNara]

Move )); to previous line. Same for other occurrences

[CTMBNara]

No need for 2nd // then block. Just leave an empty line between headers and body assertions

[CTMBNara]

why do u still need a config for wiremock?

[CTMBNara]

Mock only necessary things. Use mock instead of actual implementations for AuctionInvocationContext

[CTMBNara]

Add some ids for this test

[CTMBNara]

Use assertThat(...).extracting(...).extracting(...).containsExactly(...) api (each part of chain on new line)