Per Impression TID Generation and Source TID Logic Updated #2412

ccorbo · 2022-10-07T17:25:49Z

This PR is to update the logic SourceTID logic when enabled to fill source tid. This is related to #2381.

Logic is outlined in this issue: #2381 (comment)

Changes in this PR:

Logic update in validateAndFillSourceTid to support the logic outlined above
Changes to ImpWrapper to support accessing and setting tid in imp.ext
Unit tests added to support the above changes

bsardo

I opened up a comment on the corresponding feature request issue to verify the requirements:
#2381 (comment)

endpoints/openrtb2/auction.go

bsardo · 2022-10-18T14:08:12Z

endpoints/openrtb2/auction.go

+ req.Source.TID = req.ID
+ if rawUUID, err := uuid.NewV4(); err == nil {
+ ie.SetTid(rawUUID.String())
+ impWrapper.RebuildImp()


I believe we should be rebuilding the imps here but I need to confirm. By doing so we would ensure that after the validation phase the imps on the request accurately reflect any changes made to exts on the wrapper.

endpoints/openrtb2/auction_test.go

openrtb_ext/request_wrapper.go

openrtb_ext/request_wrapper_test.go

bsardo

The updates look good. I'm waiting for confirmation on the requirements before approving and I'll look into my comment about RebuildImp.

bsardo · 2022-10-19T02:25:33Z

Also just for future reference, if you could avoid force pushing after the PR has been reviewed that would be ideal. We prefer you just push another commit with your changes as this helps the reviewers save a lot of time since they often only need to review the delta. And if conflicts arise they can be resolved by merging with master instead of rebasing with a force push 🙂. Thanks again for your contribution; I'll be on the lookout for confirmation on the requirements.

ccorbo · 2022-10-19T14:30:38Z

Thank you for the review and sounds good! I'll make sure to merge instead of rebasing next time when merging master.

bsardo · 2022-11-07T18:35:34Z

@ccorbo I noticed you pushed up a bunch of commits, which I assume are in accordance with the new requirements. Is this ready for review?

ccorbo · 2022-11-07T18:39:46Z

@ccorbo I noticed you pushed up a bunch of commits, which I assume are in accordance with the new requirements. Is this ready for review?

@bsardo Yes this is ready for review, thanks in advance!

hhhjort · 2022-11-22T20:42:16Z

endpoints/openrtb2/auction.go

+ req.Source.TID = rawUUID.String()
+ }
+
+ if generateRequestID && (isAmp || hasStoredBidRequest) {


We may want to put this if statement in an else from the if above, so that we don't call rawUUID.String() twice.

Actually we probably want to put it all into one, so we don't call uuid.NewV4() when we don't have to, and potentially error out when we don't even have to generate a random UUID for the request.

if req.Source.TID == "" || req.Source.TID == "{{UUID}}" || ( generateRequestID && (isAmp || hasStoredBidRequest) ) { rawUUID, err := uuid.NewV4() if err != nil { return errors.New("error creating a random UUID for source.tid") } req.Source.TID = rawUUID.String() }

thanks for the review, updated

endpoints/openrtb2/auction.go

hhhjort · 2022-11-22T20:50:28Z

endpoints/openrtb2/auction.go

+ if err != nil {
+ return errors.New("imp.ext.tid missing in the imp and error creating a random UID")
+ }
+ if ie.GetTid() == "" || ie.GetTid() == "{{UUID}}" {


Shouldn't we also check that req.Source.Tid was originally empty before setting imp.ext.tid as per the spec in this PR? Probably need to set a boolean to indicate if this was empty before the code above is executed, or move this above setting req.Source.Tid.

Sorry my PR summary was outdated, now updated. The logic comes from Bret's comment on this issue if you want to cross reference. We went away from tying source.tid to imp.ext.tid

#2381 (comment)

Okay, if we are adhering to what prebid.js is already doing, then I am okay. There is a bit of tension between allowing publishers to send in IDs while also leveraging stored requests for config management vs. dealing with IDs that were inadvertently put into a stored request rendering them static.

hhhjort · 2022-11-28T20:55:43Z

Seems like something odd happened with the merge from master. Github is showing all the new code from master as new code, not recognizing it as code that already exists in master.

ccorbo · 2022-11-28T21:17:14Z

Seems like something odd happened with the merge from master. Github is showing all the new code from master as new code, not recognizing it as code that already exists in master.

weird, might of been because I mixed rebasing and merging, anyway rebased and its showing the correct diff now

hhhjort

LGTM

bsardo · 2023-01-09T18:53:53Z

@ccorbo would you mind merging with master to resolve the conflicts when you get a moment? Thanks!

ccorbo · 2023-01-09T19:29:38Z

@ccorbo would you mind merging with master to resolve the conflicts when you get a moment? Thanks!

Thanks for the ping @bsardo, merged master and conflicts are resolved

AlexBVolcy

Looks good! Just one small test coverage comment

AlexBVolcy · 2023-01-10T20:44:33Z

openrtb_ext/request_wrapper.go

+ if hasTid {
+ if err := json.Unmarshal(tidJson, &e.tid); err != nil {


Could you add a test where if hasTid is true? I noticed that the code block where it is true isn't covered by tests. I'm not looking for the unmarshal return err line (line 1306) to be covered, but line 1305 should be covered, just to show that hasTid being true is being tested.

Yep, just added test coverage around this block

AlexBVolcy

LGTM

Co-authored-by: Chris Corbo <[email protected]>

bsardo self-requested a review October 10, 2022 17:34

bsardo self-assigned this Oct 10, 2022

SyntaxNode self-requested a review October 12, 2022 21:32

SyntaxNode self-assigned this Oct 12, 2022

bsardo reviewed Oct 18, 2022

View reviewed changes

ccorbo force-pushed the tid_PB-1265 branch from ac6e38f to 60307d5 Compare October 18, 2022 18:19

bsardo reviewed Oct 19, 2022

View reviewed changes

ccorbo mentioned this pull request Nov 3, 2022

Feature Request: Per-Impression Transaction IDs (BidRequest.imp[].ext.tid) #2381

Closed

ccorbo force-pushed the tid_PB-1265 branch 2 times, most recently from 3bae81c to dbffbea Compare November 7, 2022 15:35

SyntaxNode added the port to java label Nov 22, 2022

bsardo requested a review from hhhjort November 22, 2022 18:20

bsardo assigned hhhjort and unassigned SyntaxNode Nov 22, 2022

hhhjort reviewed Nov 22, 2022

View reviewed changes

endpoints/openrtb2/auction.go Show resolved Hide resolved

hhhjort reviewed Nov 22, 2022

View reviewed changes

ccorbo changed the base branch from master to 0.69.0 November 28, 2022 21:10

ccorbo changed the base branch from 0.69.0 to master November 28, 2022 21:10

ccorbo force-pushed the tid_PB-1265 branch from 3a69e48 to 3a6fb70 Compare November 28, 2022 21:16

hhhjort previously approved these changes Nov 29, 2022

View reviewed changes

bsardo requested a review from AlexBVolcy December 8, 2022 18:42

bsardo assigned AlexBVolcy and unassigned bsardo Dec 8, 2022

AlexBVolcy previously approved these changes Dec 9, 2022

View reviewed changes

Chris Corbo added 2 commits January 9, 2023 13:50

feat: new tid logic [PB-1265]

1de1d26

PR feedback

d3317ef

Chris Corbo added 3 commits January 9, 2023 14:12

wip

4ef0da3

New logic implemented

30f3a21

pr feedback cont

a9e746b

ccorbo dismissed stale reviews from AlexBVolcy and hhhjort via a9e746b January 9, 2023 19:23

ccorbo force-pushed the tid_PB-1265 branch from 3a6fb70 to a9e746b Compare January 9, 2023 19:23

hhhjort previously approved these changes Jan 10, 2023

View reviewed changes

AlexBVolcy reviewed Jan 10, 2023

View reviewed changes

add test coverage around hasTid unmarshal

c9ad99d

ccorbo dismissed hhhjort’s stale review via c9ad99d January 10, 2023 21:01

hhhjort approved these changes Jan 11, 2023

View reviewed changes

AlexBVolcy approved these changes Jan 11, 2023

View reviewed changes

bsardo merged commit 82c03b0 into prebid:master Jan 12, 2023

shunj-nb pushed a commit to ParticleMedia/prebid-server that referenced this pull request Jan 31, 2023

Per Impression TID Generation and Source TID Logic Updated (prebid#2412)

816e2e0

Co-authored-by: Chris Corbo <[email protected]>

AvinashKapre mentioned this pull request Mar 16, 2023

unmarshal of imp.ext.{bidder} fails in dealTier flow after addition of tid field in impression #2647

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Per Impression TID Generation and Source TID Logic Updated #2412

Per Impression TID Generation and Source TID Logic Updated #2412

ccorbo commented Oct 7, 2022 •

edited

Loading

bsardo left a comment

bsardo Oct 18, 2022

bsardo left a comment

bsardo commented Oct 19, 2022

ccorbo commented Oct 19, 2022

bsardo commented Nov 7, 2022

ccorbo commented Nov 7, 2022

hhhjort Nov 22, 2022

ccorbo Nov 23, 2022

hhhjort Nov 22, 2022

ccorbo Nov 23, 2022

hhhjort Nov 28, 2022

hhhjort commented Nov 28, 2022

ccorbo commented Nov 28, 2022

hhhjort left a comment

bsardo commented Jan 9, 2023

ccorbo commented Jan 9, 2023

AlexBVolcy left a comment

AlexBVolcy Jan 10, 2023

ccorbo Jan 10, 2023

AlexBVolcy left a comment

		if hasTid {
		if err := json.Unmarshal(tidJson, &e.tid); err != nil {

Per Impression TID Generation and Source TID Logic Updated #2412

Per Impression TID Generation and Source TID Logic Updated #2412

Conversation

ccorbo commented Oct 7, 2022 • edited Loading

bsardo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsardo left a comment

Choose a reason for hiding this comment

bsardo commented Oct 19, 2022

ccorbo commented Oct 19, 2022

bsardo commented Nov 7, 2022

ccorbo commented Nov 7, 2022

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 commented Nov 28, 2022

ccorbo commented Nov 28, 2022

hhhjort left a comment

Choose a reason for hiding this comment

bsardo commented Jan 9, 2023

ccorbo commented Jan 9, 2023

AlexBVolcy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexBVolcy left a comment

Choose a reason for hiding this comment

ccorbo commented Oct 7, 2022 •

edited

Loading