DOOH support #2758

minaguib · 2023-05-09T00:06:38Z

Implements #2532 with minor deviations from proposal:

No DOOH FirstPartyData implementation (will implement after it's refactored)
Metrics: Did not add requests_without_cookie label to existing requests metrics keys:
- Instead classified DOOH as a top-level request type (sibling to site and app) which is clear enough in output metrics

VeronikaSolovei9

Thank you for your contribution!
Added some comments.

Just want to reiterate requirements

Adjust validation logic to verify that only one of app, dooh, and site are provided. - Done

Allow bidders to specify in their configuration if they support media types for a new dooh capability. - Done

Filter unsupported dooh impressions before calling a bidder, extending existing opt-in logic for app and site. - Done, unit test is needed

Add a new dooh channel type and auto populate to dooh when the value is not provided by a publisher. - Not sure where is this, can you please point to the code? Is it a part of setFieldsImplicitly function changes?

When searching for the account id, check dooh.publisher in addition to app and site. - Done

Add support for dooh in the video endpoint data model. This endpoint is planned to be deprecated, but its strange IMHO to implement DOOH support everyone but here. - not done. This should be a simple change: just add DOOH object to BidRequestVideo (similar to Site and App). No changes in endpoint are needed, it should be handled automatically by HoldAuction.

endpoints/openrtb2/auction.go

adapters/infoawarebidder.go

adapters/infoawarebidder_test.go

config/bidderinfo_test.go

config/bidderinfo.go

endpoints/openrtb2/sample-requests/invalid-whole/site-dooh-both.json

VeronikaSolovei9 · 2023-05-16T00:44:13Z

endpoints/openrtb2/auction.go

+	} else if isDOOHReq {
+		labels.Source = metrics.DemandDOOH
+		labels.RType = metrics.ReqTypeORTB2DOOH
+		labels.PubID = accountId


Please add unit test for this case

This is not covered, please consider to add a unit test for these lines

@VeronikaSolovei9 I really struggled with adding test coverage here.

The reason is, there's no testing for all of the other branches in this if-else group - so I had no easy way to simply add tests for this new condition.

I couldn't find a portion of the test suite that explicitly covers parseRequest() - only tests for some of the other functions it calls.

I went down the rabbit hole of potentially adding metrics labeling tests by extending the json examples in endpoints/openrtb2/sample-requests (something like expectedMetricLabels to complement expectedReturnCode, but the driver of the tests actually lacked access to the metric labels to assert on expected values).

Happy to hear any recommendations to pull this off reasonably, or punt for not.

endpoints/openrtb2/auction.go

SyntaxNode

@VeronikaSolovei9 has a lot of good feedback. In addition to the few nitpick comments,

Please add a DOOH Channel in config/accounts.go + exchange/utils.go
Add support in the RequestWrapper for doohExt alongside appExt and siteExt.

adapters/infoawarebidder.go

adapters/infoawarebidder_test.go

config/bidderinfo.go

config/bidderinfo_test.go

endpoints/info/bidders_detail_test.go

endpoints/openrtb2/auction.go

endpoints/openrtb2/sample-requests/invalid-whole/site-dooh-both.json

From feedback from PR prebid#2758

VeronikaSolovei9 · 2023-06-14T20:23:18Z

Thank you for addressing comments!
Please review remaining comments, they are mostly minor and relate to unit tests.
Just want to reiterate:
-New DOOH object is not added to video endpoint data model BidRequestVideo
-FPD support for DOOH will be added in future PR

From my previous comment:

Add a new dooh channel type and auto populate to dooh when the value is not provided by a publisher. - Not sure where is this, can you please point to the code? Is it a part of setFieldsImplicitly function changes?

Can you please answer this?

From feedback from PR pull/2758

bsardo · 2023-06-27T17:12:06Z

Hi @minaguib, if you don't mind, in the future could you merge with master instead of force pushing? Force pushing makes it hard for us to review the delta. Thanks!

VeronikaSolovei9 · 2023-06-28T01:55:47Z

Scott commented earlier:

Please add a DOOH Channel in config/accounts.go + exchange/utils.go

Add support in the RequestWrapper for doohExt alongside appExt and siteExt.

@minaguib please address these too. I don't see it's added. Mentioned channelType in config/account.go will cover my question about DOOH channel type. Please include it in func GetByChannelType and AccountChannel struct.

Please confirm DOOH FPD support and BidRequestVideo will be updated in future PRs.

guscarreon · 2023-07-07T18:34:40Z

config/test/bidder-info-valid/stroeerCore.yaml

+  dooh:
+    mediaTypes:
+      - banner
+      - video


Not part of this PR but, is the maintainer email at the top of this file real?

1 maintainer: 2 email: "some-email@domain.com" 3 gvlVendorID: 42 ~/newgo/src/github.com/prebid/prebid-server/config/test/bidder-info-valid/stroeerCore.yaml

No. This is a test file.

guscarreon · 2023-07-07T22:33:31Z

endpoints/openrtb2/auction.go

@@ -1823,6 +1845,30 @@ func validateDevice(device *openrtb2.Device) error {
 	return nil
 }

+func validateExactlyOneInventoryType(reqWrapper *openrtb_ext.RequestWrapper) error {


This function is well covered by integration tests, but can we add a TestValidateExactlyOneInventoryType anyways?

@guscarreon Pushed requested test below

hhhjort · 2023-07-19T17:10:35Z

Just a friendly ping to make sure you are still working on this PR.

SyntaxNode · 2023-07-19T20:37:42Z

Just a friendly ping to make sure you are still working on this PR.

Confirmed with @minaguib recently on the approach and that it's on his radar to finish up the last few items.

guscarreon · 2023-08-22T17:13:51Z

Hi @minaguib. Friendly ping to make sure you are still working on this PR. On to of review comments, merge conflicts also need to be resolved.

minaguib · 2023-08-22T17:21:20Z

@guscarreon @hhhjort confirming I'm back on this and will be wrapping up outstanding items shortly.

This is currently a bit clinical as no fields are expected, however it now exists for consistency with the others (site and app) to accomodate any future dooh.ext attributes

VeronikaSolovei9 · 2023-08-30T23:08:37Z

Thank you for the updates!

Please include DOOH support in func GetByChannelType and AccountChannel struct in config/accounts.go and to channelTypeMap in exchange/utils.go
Please confirm DOOH FPD support and BidRequestVideo will be updated in future PRs.

minaguib · 2023-09-12T14:02:38Z

I believe all significant points raised in this PR are now addressed.

Re BidRequestVideo DOOH support - I'm not convinced this will ever be necessary. From early discussions with Scott, I feel we should bake and launch, and through adoption see if it makes sense to add DOOH support there or not at a later point

SyntaxNode · 2023-09-12T21:03:39Z

Thank you for updating to keep up with recent changes, and sorry for the moving target. That being said, there is a request from @bretg to update fillChannel to include dooh (per #2532 (comment)). I'm fine if you'd like to do this as a follow-up so we can get the bulk of this functionality merged in.

VeronikaSolovei9

Thank you for the changes, LGTM

SyntaxNode self-assigned this May 9, 2023

SyntaxNode self-requested a review May 9, 2023 17:05

bsardo assigned VeronikaSolovei9 May 10, 2023

VeronikaSolovei9 reviewed May 16, 2023

View reviewed changes

SyntaxNode reviewed May 19, 2023

View reviewed changes

minaguib added a commit to Hivestack/prebid-server that referenced this pull request Jun 10, 2023

DOOH: Testing improvements

8d3e8d0

From feedback from PR prebid#2758

minaguib added 3 commits June 9, 2023 21:44

DOOH: Adapter capability declaration and filtering

e82b9e9

DOOH: Basic validation and support

577b0bb

DOOH: Metrics

77cd57f

minaguib force-pushed the dooh-support branch from 8d3e8d0 to 93eb938 Compare June 10, 2023 01:44

minaguib added a commit to Hivestack/prebid-server that referenced this pull request Jun 10, 2023

DOOH: Testing improvements

93eb938

From feedback from PR prebid#2758

minaguib force-pushed the dooh-support branch from 93eb938 to 8a38f73 Compare June 10, 2023 01:46

bsardo assigned guscarreon and unassigned SyntaxNode Jun 14, 2023

DOOH: Testing improvements

8bcae1d

From feedback from PR pull/2758

minaguib force-pushed the dooh-support branch from 8a38f73 to 8bcae1d Compare June 27, 2023 13:03

guscarreon reviewed Jul 7, 2023

View reviewed changes

SyntaxNode assigned SyntaxNode and unassigned guscarreon Jul 25, 2023

minaguib added 4 commits August 22, 2023 14:46

Merge remote-tracking branch 'prebid/master' into dooh-support

4c3da8c

Merge remote-tracking branch 'prebid/master' into dooh-support

081f1cf

DOOH: Adapter aliases support

eaed654

DOOH: Add RequestWrapper doohExt

a46952b

This is currently a bit clinical as no fields are expected, however it now exists for consistency with the others (site and app) to accomodate any future dooh.ext attributes

DOOH: Auction: Add TestValidateExactlyOneInventoryType

46b17e6

minaguib added 2 commits September 7, 2023 14:43

Merge remote-tracking branch 'prebid/master' into dooh-support

0e6da54

DOOH: Add ChannelDOOH support

0749eb9

VeronikaSolovei9 approved these changes Sep 13, 2023

View reviewed changes

SyntaxNode approved these changes Sep 19, 2023

View reviewed changes

bsardo changed the title ~~Dooh support~~ DOOH support Sep 20, 2023

bsardo merged commit 48e6275 into prebid:master Sep 20, 2023

cross-repo-issue-creation bot mentioned this pull request Sep 20, 2023

Port PR from PBS-Go: DOOH support prebid/prebid-server-java#2637

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DOOH support #2758

DOOH support #2758

minaguib commented May 9, 2023

VeronikaSolovei9 left a comment

VeronikaSolovei9 May 16, 2023

VeronikaSolovei9 Jun 14, 2023

minaguib Sep 12, 2023

SyntaxNode left a comment

VeronikaSolovei9 commented Jun 14, 2023

bsardo commented Jun 27, 2023

VeronikaSolovei9 commented Jun 28, 2023

guscarreon Jul 7, 2023

SyntaxNode Sep 12, 2023

guscarreon Jul 7, 2023

minaguib Aug 30, 2023

hhhjort commented Jul 19, 2023

SyntaxNode commented Jul 19, 2023

guscarreon commented Aug 22, 2023

minaguib commented Aug 22, 2023

VeronikaSolovei9 commented Aug 30, 2023

minaguib commented Sep 12, 2023

SyntaxNode commented Sep 12, 2023

VeronikaSolovei9 left a comment

DOOH support #2758

DOOH support #2758

Conversation

minaguib commented May 9, 2023

VeronikaSolovei9 left a comment

Choose a reason for hiding this comment

VeronikaSolovei9 May 16, 2023

Choose a reason for hiding this comment

VeronikaSolovei9 Jun 14, 2023

Choose a reason for hiding this comment

minaguib Sep 12, 2023

Choose a reason for hiding this comment

SyntaxNode left a comment

Choose a reason for hiding this comment

VeronikaSolovei9 commented Jun 14, 2023

bsardo commented Jun 27, 2023

VeronikaSolovei9 commented Jun 28, 2023

guscarreon Jul 7, 2023

Choose a reason for hiding this comment

SyntaxNode Sep 12, 2023

Choose a reason for hiding this comment

guscarreon Jul 7, 2023

Choose a reason for hiding this comment

minaguib Aug 30, 2023

Choose a reason for hiding this comment

hhhjort commented Jul 19, 2023

SyntaxNode commented Jul 19, 2023

guscarreon commented Aug 22, 2023

minaguib commented Aug 22, 2023

VeronikaSolovei9 commented Aug 30, 2023

minaguib commented Sep 12, 2023

SyntaxNode commented Sep 12, 2023

VeronikaSolovei9 left a comment

Choose a reason for hiding this comment