Debug Flag for Cookie Sync #3107

AlexBVolcy · 2023-09-14T19:15:31Z

Adds debug flag for cookie sync endpoint as described in this issue #2173

endpoints/cookie_sync.go

endpoints/cookie_sync_test.go

usersync/chooser.go

endpoints/cookie_sync.go

VeronikaSolovei9 · 2023-09-27T23:22:38Z

endpoints/cookie_sync.go

+	if debug {
+		biddersSeen := make(map[string]struct{})
+		var debugMessages []string
+		for _, bidderEval := range biddersEvaluted {
+			if bidderEval.Status == usersync.StatusDuplicate && biddersSeen[bidderEval.Bidder] == struct{}{} {
+				debugMessages = append(debugMessages, getDebugMessage(bidderEval.Status)+" synced as "+bidderEval.SyncerKey)
+			} else if bidderEval.Status != usersync.StatusDuplicate {
+				debugMessages = append(debugMessages, getDebugMessage(bidderEval.Status))
+			}
+			biddersSeen[bidderEval.Bidder] = struct{}{}
+		}
+		response.Debug = debugMessages
+	}


I tested it with activities where only "pubmatic" and "appnexus" are allowed.
Request to cookie_sync:

{ "bidders": ["appnexus", "pubmatic", "adf", "synacormedia"], "debug":true }

Expected: 2 bidders ("adf", "synacormedia") are rejected by privacy and 2 bidders are allowed.
Here is debug part of response:

"debug": [ "", "Rejected by privacy", "Rejected by privacy", "" ]

Can you please add bidder name to message, f.i "Rejected by privacy: synacormedia" and remove empty strings from debug messages.
In activity requirements it states that message should be "Bidder sync blocked for privacy reasons". Not sure what our message should be, I feel like having bidder name in it is reasonable.

Also, about debug flag in request. Here it is a field in json body: "debug": true
Is it confirmed it should be like this?
I'm asking because we handle debug flag differently in different endpoints. F.i. in /action it is "test": 1

@AlexBVolcy Please match the response format established with PBS-Java.

I agree here. I also want to point out that it seems that some of our syncer error messages are a bit different from PBS-Java. I think that's ok. @SyntaxNode, @VeronikaSolovei9 thoughts? What's most important is that the request and response formats are the same.

We should match the debug messages too. Unless they reference the "cookie family", which is called the "syncer key" in Go.

@SyntaxNode @VeronikaSolovei9 @bsardo So here are the debug messages that PBS-Java uses

builder = switch (reason) { case INVALID_BIDDER -> builder.conditionalError(requested, "Unsupported bidder"); case DISABLED_BIDDER -> builder.conditionalError(requested, "Disabled bidder"); case REJECTED_BY_TCF -> builder.conditionalError(requested || coopSync, "Rejected by TCF"); case REJECTED_BY_CCPA -> builder.conditionalError(requested || coopSync, "Rejected by CCPA"); case DISALLOWED_ACTIVITY -> builder.conditionalError(requested || coopSync, "Disallowed activity"); case UNCONFIGURED_USERSYNC -> builder.conditionalError(requested, "No sync config"); case DISABLED_USERSYNC -> builder.conditionalError(requested || coopSync, "Sync disabled by config"); case REJECTED_BY_FILTER -> builder.conditionalError(requested || coopSync, "Rejected by request filter"); case ALREADY_IN_SYNC -> builder.conditionalError(requested, "Already in sync"); };

I tried to match debug messages the best I could, but there are some statues in my PR that don't overlap with the PBS-Java statuses (i.e. StatusTypeNotSupported), so in those cases I came up with my own debug message. Is this alright?

And yes @VeronikaSolovei9, the debug flag in the request should be like this debug: true

Thank you for addressing comments, just checked it locally and it works as expected.
Question: for bidders restricted by activity here is an expected message:
case DISALLOWED_ACTIVITY -> builder.conditionalError(requested || coopSync, "Disallowed activity");
In this PR we have "Rejected by privacy: synacormedia" (I know I asked about the bidder name).
Should it be? Disallowed activity?

usersync/chooser.go

bsardo · 2023-09-28T16:46:00Z

endpoints/cookie_sync.go

+	if debug {
+		biddersSeen := make(map[string]struct{})
+		var debugMessages []string
+		for _, bidderEval := range biddersEvaluted {
+			if bidderEval.Status == usersync.StatusDuplicate && biddersSeen[bidderEval.Bidder] == struct{}{} {
+				debugMessages = append(debugMessages, getDebugMessage(bidderEval.Status)+" synced as "+bidderEval.SyncerKey)
+			} else if bidderEval.Status != usersync.StatusDuplicate {
+				debugMessages = append(debugMessages, getDebugMessage(bidderEval.Status))
+			}
+			biddersSeen[bidderEval.Bidder] = struct{}{}
+		}
+		response.Debug = debugMessages
+	}


I agree here. I also want to point out that it seems that some of our syncer error messages are a bit different from PBS-Java. I think that's ok. @SyntaxNode, @VeronikaSolovei9 thoughts? What's most important is that the request and response formats are the same.

VeronikaSolovei9 · 2023-10-05T19:48:36Z

endpoints/cookie_sync.go

+	if debug {
+		biddersSeen := make(map[string]struct{})
+		var debugInfo []cookieSyncResponseDebug
+		for _, bidderEval := range biddersEvaluated {
+			var debug cookieSyncResponseDebug


Nitpick: if debug { and var debug cookieSyncResponseDebug create an ambiguity in variables names.
Can you rename var debug cookieSyncResponseDebug to var debugResponse cookieSyncResponseDebug, or something else that makes sense to you?

VeronikaSolovei9

Local testing looks good. Added minor comment about naming

VeronikaSolovei9

LGTM

bsardo · 2023-10-24T16:24:44Z

endpoints/cookie_sync.go

-		case usersync.StatusBlockedByGDPR:
-			c.metrics.RecordSyncerRequest(bidder.SyncerKey, metrics.SyncerCookieSyncPrivacyBlocked)
-		case usersync.StatusBlockedByCCPA:
+		case usersync.StatusBlockedByPrivacy:


I'm curious what's driving the change from regulation-specific statuses to a more generic privacy status. Was there a discussion about this?

Scott and I did have a conversation a while ago and this name change was brought up I think in conjunction with removing CCPA from the evaluate() function. @SyntaxNode I think could touch on this more.

bsardo · 2023-10-24T16:30:02Z

endpoints/cookie_sync.go

+	case usersync.StatusDuplicate:
+		return "Duplicate bidder"


Is this equivalent to PBS-Java's "synced as FAMILY" status or is there a difference here?

Yes this is the equivalent, and it'll have appended to it: synced as name_of_bidder_syncer

bsardo · 2023-10-24T16:44:13Z

usersync/chooser.go

 	}

 	syncersSeen := make(map[string]struct{})
+	biddersSeen := make(map[string]struct{})
 	limitDisabled := request.Limit <= 0

 	biddersEvaluated := make([]BidderEvaluation, 0)
 	syncersChosen := make([]SyncerChoice, 0)

 	bidders := c.bidderChooser.choose(request.Bidders, c.biddersAvailable, request.Cooperative)
 	for i := 0; i < len(bidders) && (limitDisabled || len(syncersChosen) < request.Limit); i++ {


I'm not sure if this is necessary, but I see that PBS-Java provides debug information indicating when a bidder is ignored because the limit has been reached. Similarly they are indicating if a bidder is known but disabled. We should discuss whether we want to add this logic, perhaps in a separate PR.

Oh interesting, yeah let's talk about this at the next team time.

Discussed offline. PBS-Go behavior is different from PBS-Java. We stop processing when we reach the limit so there's no need to report on this.

endpoints/cookie_sync.go

bsardo · 2023-10-24T17:02:35Z

usersync/chooser_test.go

 		},
 		{
-			description:                 "Case Insensitivity Check For Blocked By CCPA",


Maybe we should keep this case insensitivitiy test case?

I removed it primarily because we removed the CCPA check from the evaluate() logic, thus making this test case unnecessary.

Oops I didn't realize there was a GDPR case insensitivity test case above this one. It makes sense to delete this.

AlexBVolcy added 7 commits August 21, 2023 15:38

Debug flag framework

53dd683

Add non fatal errors handling

6cd2272

Bidder Evaluation reporting

ae6259c

Update how status is attached to response

2d5afdc

Add tests

0cf88cd

Clean up, add tests

2541956

Merge branch 'master' into debug_flag_cookie_sync

ff41f45

AlexBVolcy marked this pull request as draft September 14, 2023 21:40

SyntaxNode reviewed Sep 19, 2023

View reviewed changes

Minor improvements

cb33792

AlexBVolcy marked this pull request as ready for review September 20, 2023 16:25

bsardo assigned bsardo and guscarreon Sep 20, 2023

Update tests

96caaa9

SyntaxNode added the do not port label Sep 21, 2023

bsardo assigned VeronikaSolovei9 and unassigned guscarreon Sep 27, 2023

VeronikaSolovei9 reviewed Sep 27, 2023

View reviewed changes

usersync/chooser.go Show resolved Hide resolved

VeronikaSolovei9 reviewed Sep 27, 2023

View reviewed changes

endpoints/cookie_sync.go Outdated Show resolved Hide resolved

VeronikaSolovei9 reviewed Sep 27, 2023

View reviewed changes

bsardo reviewed Sep 28, 2023

View reviewed changes

Address comments

5ffbc7a

This was referenced Sep 28, 2023

Make user.ext.prebid.buyeruids bidder name case insensitive #3152

Merged

update cookie sync endpoint to be case insensitive #3103

Merged

Re-work debug structure in response

e725769

VeronikaSolovei9 reviewed Oct 5, 2023

View reviewed changes

AlexBVolcy added 3 commits October 5, 2023 16:48

Update name

22a7777

Merge branch 'master' into debug_flag_cookie_sync

2b837c6

Merge fix

fdf26b4

AlexBVolcy added 4 commits October 18, 2023 16:32

Merge branch 'master' into debug_flag_cookie_sync

008db67

Merge branch 'master' into debug_flag_cookie_sync

7c1759e

Remove print stmts

b000c9e

Remove print stmt

f4a38bf

VeronikaSolovei9 previously approved these changes Oct 23, 2023

View reviewed changes

bsardo reviewed Oct 24, 2023

View reviewed changes

Add omitempty

462295a

AlexBVolcy dismissed VeronikaSolovei9’s stale review via 462295a October 24, 2023 19:43

bsardo approved these changes Oct 26, 2023

View reviewed changes

VeronikaSolovei9 approved these changes Oct 26, 2023

View reviewed changes

bsardo merged commit 905436f into prebid:master Nov 16, 2023
3 checks passed

svamiftah pushed a commit to sovrn/prebid-server that referenced this pull request Nov 21, 2023

Add debug flag to cookie sync endpoint (prebid#3107)

436e3b3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Debug Flag for Cookie Sync #3107

Debug Flag for Cookie Sync #3107

AlexBVolcy commented Sep 14, 2023 •

edited

Loading

VeronikaSolovei9 Sep 27, 2023

SyntaxNode Sep 28, 2023

bsardo Sep 28, 2023

SyntaxNode Sep 28, 2023

AlexBVolcy Sep 28, 2023 •

edited

Loading

AlexBVolcy Sep 28, 2023

VeronikaSolovei9 Sep 28, 2023

bsardo Sep 28, 2023

VeronikaSolovei9 Oct 5, 2023

VeronikaSolovei9 left a comment

VeronikaSolovei9 left a comment

bsardo Oct 24, 2023

AlexBVolcy Oct 24, 2023

bsardo Oct 24, 2023

AlexBVolcy Oct 24, 2023

bsardo Oct 24, 2023

AlexBVolcy Oct 24, 2023

bsardo Oct 26, 2023

bsardo Oct 24, 2023

AlexBVolcy Oct 24, 2023

bsardo Oct 26, 2023

Debug Flag for Cookie Sync #3107

Debug Flag for Cookie Sync #3107

Conversation

AlexBVolcy commented Sep 14, 2023 • 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

AlexBVolcy Sep 28, 2023 • 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

VeronikaSolovei9 left a comment

Choose a reason for hiding this comment

VeronikaSolovei9 left a 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

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

AlexBVolcy commented Sep 14, 2023 •

edited

Loading

AlexBVolcy Sep 28, 2023 •

edited

Loading