Generate Unique Bidrequest IDs for Amp Auctions #1938

AlexBVolcy · 2021-07-26T19:16:52Z

Addressing this issue here #1507

I've added a configuration boolean flag, GenerateRequestId, which when set to true, will generate a uuid for auction bid requests, which can be used to solve the amp auction issue found here #1507

This generation can occur in two different functions:

In amp_auction.go/loadRequestJSONForAmp( ) -- where we load the stored openRTB request for an incoming AMP request. Here we can check our boolean flag, and generate an ID if necessary.
In auction.go/processStoredRequests() -- we check here if GenerateRequestId is true, and if so, we generate a uuid and patch it into the request

hhhjort · 2021-08-02T19:24:47Z

endpoints/openrtb2/amp_auction_test.go

+ givenInTagId string
+ givenInStoredRequest json.RawMessage
+ givenGenerateStoredRequestId bool
+ expected int


Might want to call this expectedIDLength as at first I was wondering how you arranged it to have generated ID always be "36".

hhhjort · 2021-08-02T19:28:43Z

endpoints/openrtb2/auction.go

@@ -1448,7 +1449,13 @@ func getStoredRequestId(data []byte) (string, bool, error) {
 return "", true, errors.New("ext.prebid.storedrequest.id must be a string")
 }

- return string(value), true, nil
+ // Generates Request ID if flag is set to true -- used to solve issue with amp auctions


This is supposed to return the ID of the stored request, not the openrtb request's ID. Also by generating a random stored request ID you are virtually guaranteeing that the stored request lookup will fail, and potentially hammering the database with failing lookups.

To avoid having to unmarshal/remarshal the stored request json, you probably want to patch the retrieved stored request with { "id": "<generated uuid>" } before applying the patch to the incoming request. Unless we want the spec that the flag should always overwrite any incoming request IDs, in which case we will have to do something else. But I don't see a use case for overwriting the request ID in the incoming request other than the case where a PBS instance is only supporting a client that always sends a non-random ID, and no other clients. But in this case it really should be fixed on the client side rather than hacking PBS to enforce unique IDs in all cases.

Just to keep this PR updated, Hans and I talked about these comments and what I need to do to properly address them! A commit will be pushed today or tomorrow with the proper fixes / tests.

hhhjort · 2021-08-13T13:52:39Z

endpoints/openrtb2/auction.go

+ requestErrorJson, errL := storedRequestErrorChecker(err, requestJson, storedRequests, storedBidRequestId)
+ return requestErrorJson, nil, errL
+ }
+ } else if !deps.cfg.GenerateRequestId && hasStoredBidRequest {


If you reach this line, and hasStoredBidRequest is true, then you know !deps.cfg.GenerateRequestId also has to be true, you don't need to test both.

Another potential logic flow is:

Suggested change

} else if !deps.cfg.GenerateRequestId && hasStoredBidRequest {

storedReqForPatch = ''

if hasStoredBidRequest {

storedReqForPatch = storedRequests[storedBidRequestId]

}

if deps.cfg.GenerateRequestId {

uuidPatch, err := generateUuidForBidRequest()

if hasStoredBidRequest {

storedReqForPatch, err := jsonpatch.MergePatch(storedRequests[storedBidRequestId], uuidPatch)

} else {

storedReqForPatch := uuidPatch

hasStoredBidRequest = true

}

}

if hasStoredBidRequest {

resolvedRequest, err = jsonpatch.MergePatch(requestJson, storedReqForPatch)

}

with the error handling and other supporting code included.

hhhjort · 2021-08-13T14:09:39Z

endpoints/openrtb2/auction_test.go

@@ -1089,7 +1161,7 @@ func TestOversizedRequest(t *testing.T) {
 &mockStoredReqFetcher{},
 empty_fetcher.EmptyFetcher{},
 empty_fetcher.EmptyFetcher{},
- &config.Configuration{MaxRequestSize: int64(len(reqBody) - 1)},
+ &config.Configuration{MaxRequestSize: int64(len(reqBody) - 1), GenerateRequestId: false},


The nil value of a boolean is false, so you shouldn't have to explicitly set these values.

endpoints/openrtb2/amp_auction_test.go

config/config.go

endpoints/openrtb2/amp_auction.go

endpoints/openrtb2/amp_auction_test.go

endpoints/openrtb2/auction.go

hhhjort

LGTM

SyntaxNode

This is missing the {{UUID}} macro, but otherwise looks solid. I'm good if you'd like to add the macro in a follow-up PR. The Java version only performs this behavior for app requests, is that a necessary restriction?

config/config.go

endpoints/openrtb2/amp_auction_test.go

endpoints/openrtb2/auction.go

endpoints/openrtb2/auction_test.go

endpoints/openrtb2/auction.go

SyntaxNode · 2021-09-02T15:14:58Z

endpoints/openrtb2/auction.go

@@ -1567,3 +1585,35 @@ func getAccountID(pub *openrtb2.Publisher) string {
 }
 return metrics.PublisherUnknown
 }
+
+func storedRequestErrorChecker(err error, requestJson []byte, storedRequests map[string]json.RawMessage, storedBidRequestId string) []error {


The err argument is not used. Please consider removing it.

SyntaxNode · 2021-09-02T15:16:44Z

endpoints/openrtb2/auction_test.go

+ newRequest, _, errList := deps.processStoredRequests(context.Background(), json.RawMessage(test.givenRawData))
+ for _, err := range errList {
+ assert.Empty(t, err, test.description)
+ }


You don't need to use a loop here. You can simply write:

newRequest, _, errList := deps.processStoredRequests(context.Background(), json.RawMessage(test.givenRawData)) assert.Empty(t, errList, test.description)

SyntaxNode · 2021-09-02T15:17:26Z

endpoints/openrtb2/amp_auction.go

@@ -344,6 +347,15 @@ func (deps *endpointDeps) loadRequestJSONForAmp(httpRequest *http.Request) (req
 errs = []error{err}
 return
 }
+ // Generates Request ID if flag is set to true


Nitpick: Comment does not seem valuable.

SyntaxNode · 2021-09-02T15:19:42Z

endpoints/openrtb2/auction.go

+ isAppRequest, err := checkIfAppRequest(requestJson)
+ if err != nil {
+ return nil, nil, []error{err}
+ }


Looks good. I like that you've extracted the check to a method. To increase performance, I recommend moving this check within the deps.cfg.GenerateRequestID check, such that we only bother to check for an app request if the feature is turned on. I usually don't recommend nesting, but in this case it's a performance improvement in certain cases.

hhhjort

LGTM

SyntaxNode

The AMP flow checks out with local end to end tests. Please see me comment for a bug in the APP flow.

SyntaxNode · 2021-09-09T18:28:23Z

endpoints/openrtb2/auction.go

+ return nil, nil, []error{err}
+ }
+ if isAppRequest {
+ uuidPatch, err := generateUuidForBidRequest(deps.uuidGenerator)


The uuidPatch is being generated for all app requests, but it should only be generated when also hasStoredBidRequest is true.

Why do we want to disable generating request IDs when there is no stored request for an App request? Is that a performance tweak that we expect we should only need a request ID if there was a stored request involved?

Talked to SyntaxNode and this is desired behavior. Should be documented somewhere, in the end (not in the code, but the docs elsewhere) since it is not intuitively obvious when this feature should or should not kick in.

hhhjort

LGTM

SyntaxNode requested review from SyntaxNode and hhhjort July 28, 2021 17:44

SyntaxNode assigned SyntaxNode and hhhjort Jul 28, 2021

hhhjort reviewed Aug 2, 2021

View reviewed changes

VeronikaSolovei9 force-pushed the Generate_BidRequest_Id_Amp branch from 791257e to 895764a Compare August 12, 2021 18:23

hhhjort reviewed Aug 13, 2021

View reviewed changes

SyntaxNode reviewed Aug 16, 2021

View reviewed changes

VeronikaSolovei9 force-pushed the Generate_BidRequest_Id_Amp branch 2 times, most recently from 2747163 to a37cbec Compare August 24, 2021 19:10

hhhjort previously approved these changes Aug 26, 2021

View reviewed changes

SyntaxNode reviewed Aug 30, 2021

View reviewed changes

AlexBVolcy dismissed hhhjort’s stale review via 98e5351 August 31, 2021 15:12

SyntaxNode reviewed Sep 2, 2021

View reviewed changes

VeronikaSolovei9 force-pushed the Generate_BidRequest_Id_Amp branch from d860aa1 to c6d38aa Compare September 7, 2021 18:25

VeronikaSolovei9 added 2 commits September 7, 2021 11:27

Implementation+Tests for generating unique bid request id for amp

eeac82e

Rebase and fixes

86204e0

VeronikaSolovei9 force-pushed the Generate_BidRequest_Id_Amp branch from c6d38aa to 86204e0 Compare September 7, 2021 18:33

hhhjort previously approved these changes Sep 7, 2021

View reviewed changes

SyntaxNode requested changes Sep 9, 2021

View reviewed changes

Updated flow of uuid generation for app requests

90c65b1

AlexBVolcy dismissed hhhjort’s stale review via 90c65b1 September 13, 2021 02:06

hhhjort approved these changes Sep 13, 2021

View reviewed changes

SyntaxNode approved these changes Sep 14, 2021

View reviewed changes

SyntaxNode merged commit 24abd9c into prebid:master Sep 14, 2021

AlexBVolcy mentioned this pull request Sep 15, 2021

Adding Support for Macro {{UUID}} to Support Feature: Generate Unique Bidrequest ID #2000

Merged

jizeyopera pushed a commit to operaads/prebid-server that referenced this pull request Oct 13, 2021

Generate Unique Bidrequest IDs for Amp Auctions (prebid#1938)

554cbfd

AlexBVolcy mentioned this pull request Dec 20, 2021

Address stored request not merging with UUID bug #2059

Merged

AlexBVolcy mentioned this pull request Dec 23, 2021

bidrequest.id is not always unique in PBS requests #1507

Closed

AlexBVolcy mentioned this pull request Jan 13, 2022

Stored request not merged when using {{UUID}} #2045

Closed

AlexBVolcy mentioned this pull request Mar 25, 2022

Remove Restrictions for UUID Generation For Stored Bid Requests #2203

Merged

shunj-nb pushed a commit to ParticleMedia/prebid-server that referenced this pull request Nov 8, 2022

Generate Unique Bidrequest IDs for Amp Auctions (prebid#1938)

112f476

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate Unique Bidrequest IDs for Amp Auctions #1938

Generate Unique Bidrequest IDs for Amp Auctions #1938

AlexBVolcy commented Jul 26, 2021 •

edited

Loading

hhhjort Aug 2, 2021

hhhjort Aug 2, 2021

hhhjort Aug 2, 2021

AlexBVolcy Aug 4, 2021

hhhjort Aug 13, 2021

hhhjort Aug 13, 2021

hhhjort left a comment

SyntaxNode left a comment •

edited

Loading

SyntaxNode Sep 2, 2021

SyntaxNode Sep 2, 2021

SyntaxNode Sep 2, 2021

SyntaxNode Sep 2, 2021

hhhjort left a comment

SyntaxNode left a comment

SyntaxNode Sep 9, 2021 •

edited

Loading

hhhjort Sep 13, 2021 •

edited

Loading

hhhjort Sep 13, 2021

hhhjort left a comment

- } else if !deps.cfg.GenerateRequestId && hasStoredBidRequest {
+ storedReqForPatch = ''
+ if hasStoredBidRequest {
+ storedReqForPatch = storedRequests[storedBidRequestId]
+ }
+ if deps.cfg.GenerateRequestId {
+ uuidPatch, err := generateUuidForBidRequest()
+ if hasStoredBidRequest {
+ storedReqForPatch, err := jsonpatch.MergePatch(storedRequests[storedBidRequestId], uuidPatch)
+ } else {
+ storedReqForPatch := uuidPatch
+ hasStoredBidRequest = true
+ }
+ }
+ if hasStoredBidRequest {
+ resolvedRequest, err = jsonpatch.MergePatch(requestJson, storedReqForPatch)
+ }

Generate Unique Bidrequest IDs for Amp Auctions #1938

Generate Unique Bidrequest IDs for Amp Auctions #1938

Conversation

AlexBVolcy commented Jul 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hhhjort left a comment

Choose a reason for hiding this comment

SyntaxNode left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hhhjort left a comment

Choose a reason for hiding this comment

SyntaxNode left a comment

Choose a reason for hiding this comment

SyntaxNode Sep 9, 2021 • edited Loading

Choose a reason for hiding this comment

hhhjort Sep 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hhhjort left a comment

Choose a reason for hiding this comment

AlexBVolcy commented Jul 26, 2021 •

edited

Loading

SyntaxNode left a comment •

edited

Loading

SyntaxNode Sep 9, 2021 •

edited

Loading

hhhjort Sep 13, 2021 •

edited

Loading