/cookie_sync Endpoint Rewrite #1879

SyntaxNode · 2021-06-05T04:48:14Z

I've rewritten the majority of the /cookie_sync endpoint and user syncer architecture to implement #924, #1477, #1554, and to further progress #1429 as the config system now includes user syncing settings.

Todo

Merge from master
Add config tests
Standardize metrics and add unit tests.
Convert all existing user sync configs to the new format.

Configuration Changes

Technical details will be included in a separate PR to https://github.com/prebid/prebid.github.io before this is merged.

User sync configuration is relocated to the bidder yaml files in static/bidder-info from being hardcoded in the config.go and adapter usersync.go files. Hosts can now override all user syncing options via either pbs.yaml or environment variables.
User sync configuration now supports new macros, automatic url encoding, and a new configuration hierarchy. The external url may now be overridden at the host level, the bidder level, and the endpoint level. The default redirect url is now represented with macros such that no modification is needed for most cases. Look for more details in the docs PR.
Existing usersync_url overrides are temporarily supported for backwards compatibility, but please update your configs as soon as possible after release.
Bidders may share the same user sync key, this is ideal for hardcoded aliases or bidders with multiple adapter implementations. As a side effect, I'm delighted to announce this removes the special adnxs hardcoded handling for AppNexus.
Bidders may now specify both iframe and/or redirect endpoints.
Cooperative cookie syncing is implemented and disabled by default.

Example Current Config: Buried In config/config.go

setDefaultUsersync(cfg.Adapters, openrtb_ext.BidderAppnexus, "https://ib.adnxs.com/getuid?"+url.QueryEscape(externalURL)+"%2Fsetuid%3Fbidder%3Dadnxs%26gdpr%3D{{.GDPR}}%26gdpr_consent%3D{{.GDPRConsent}}%26uid%3D%24UID")

Example New Config: In static/bidder-info/appnexus.yaml

userSync:
  key: "adnxs"
  redirect:
    url: "https://ib.adnxs.com/getuid?{{.RedirectURL}}"
    userMacro: "$UID"

API Changes

The /cookie_sync endpoint now defaults to all bidders if the bidders field in the request is missing, null, or empty. This matches behavior in PBS-Java. We discussed and couldn't find a use case for handling each of these conditions separately. Currently, a missing bidders parameter results in an error response, a null parameter results in using all bidders, and an empty parameter results in no user syncing.
The /cookie_sync endpoint will now accept filters in the request defining which user sync types (iframe, redirect) are permitted. The format is fully compatible with Prebid.js and PBS-Java.
Cooperative cookie syncing is now supported. Requests may specify if they want this feature enabled or disabled.
The SSCookie sidecar cookie is no longer required and is removed with this PR.
PBS will no longer invalidate user syncs when it detects the browser has upgraded to a new version with support for same site cookies. This behavior isn't present in PBS-Java and our usage data shows the vast majority of end users are already using same site cookie compatible browsers.
The /setuid endpoint will now properly respond with an empty image for redirects and an empty html response for iframes.

Metric Changes

Influx

Metrics have be reorganized around endpoint names and syncer keys. cookie_sync_requests.* are directly related to the /cookie_sync endpoint, set_uid_requests.* are directly related to the /setuid endpoint, and syncer.* are related to each syncer. Specifically,

usersync.opt_outs and usersync.bad_requests are removed and replaced with cookie_sync_requests.[status] where status is ok, bad_request, opt_out, and gdpr_blocked_host_cookie.
usersync.unknown.sets and usersync.unknown.gdpr_prevent are removed and replaced with set_uid_requests.key_unknown.
cookie_syncs[bidder].gen and cookie_sync.[bidder].gdpr_prevent are removed and replaced with syncer.[key].request.[status] where the syncer key may be shared among multiple bidders and status is ok, privacy_blocked, already_synced, type_not_supported.
usersync.[bidder].sets and usersync.[bidder].gdpr_prevent are removed and replaced with syncer.[key].set.[status] where the syncer key may be shared among multiple bidders and status is ok and privacy_blocked.
setuid_requests.[status] is new where status is ok, opt_out, gdpr_blocked_host_cookie, key_unknown, and type_not_supported.

Prometheus

Metrics have be reorganized around endpoint names and syncer keys. cookie_sync_requests is directly related to the /cookie_sync endpoint, set_uid_requests is directly related to the /setuid endpoint, syncer_requests and syncer_sets are related to each syncer. Specifically,

cookie_sync_request now has a status label with values ok, bad_request, opt_out, and gdpr_blocked_host_cookie. Existing PromQL queries will still accurately report the total number of requests to the endpoint.
setuid_requests is new and has a status label with values ok, opt_out, gdpr_blocked_host_cookie, key_unknown, and type_not_supported.
adapter_cookie_sync has been removed and replaced with syncer_requests which has a syncer key label and status label with the values ok, privacy_blocked, already_synced, type_not_supported. These syncer metrics are only counted when the syncer has been selected as a user sync candidate.
adapter_user_sync has been removed and replaced with syncer_sets which has a syncer key label and status label with the values ok and privacy_blocked.

Future Plans

I'm working on account settings for the /cookie_sync endpoint, but this PR is already huge so that will be separate later on.

Bidder user sync configs

hhhjort

LGTM

bsardo

LGTM

This reverts commit 7d3bbb5.

SyntaxNode · 2021-08-17T21:10:25Z

config/bidderinfo_validate_test.go

 	}
+
+	errs := validateSyncers(t, bidderInfos)
+	assert.Empty(t, errs, "syncer errors")
 }


I improved the unit tests to use the same exact loading mechanism the app uses at startup to ensure no issues / the same validation is performed.

SyntaxNode · 2021-08-17T21:11:36Z

usersync/syncersbuilder.go

+
+	// a syncer may provide just a Supports field to provide hints to the host. we should only try to create a syncer
+	// if there is at least one non-Supports value populated.
+	return cfg.Syncer.Key != "" || cfg.Syncer.Default != "" || cfg.Syncer.IFrame != nil || cfg.Syncer.Redirect != nil || cfg.Syncer.SupportCORS != nil


This is a bit hacky. Perhaps it would be better to use different data structures for yaml vs config overrides... but I think this works.

bsardo · 2021-08-18T14:59:22Z

usersync/syncersbuilder_test.go

+			givenConfig:      hostConfig,
+			givenBidderInfos: map[string]config.BidderInfo{"bidder1": infoKeyAEmpty, "bidder2": infoKeyAError},
+			expectedErrors: []string{
+				"cannot create syncer for bidder bidder2 with key a: default is set to redirect but no redirect endpoint is configured",


Just to confirm, bidder1 does not result in an error here even though it is missing endpoints and a default because it shares the same key as bidder2 and bidder2 is supposed to define the endpoints for that key since it specifies a default right?

Correct. Exactly. I wrote it to cover the case where two bidders share the same key but the primary configuration (bidder2 here) is invalid.

bsardo · 2021-08-18T15:43:53Z

config/bidderinfo_validate_test.go

-	_, err = syncer.GetSync([]usersync.SyncType{usersync.SyncTypeIFrame, usersync.SyncTypeRedirect}, privacyPolicies)
-	if err != nil {
-		return fmt.Errorf("syncer has invalid macro: %s", err)
+	for _, v := range bidderInfo.Syncer.Supports {


You're leaning on the app validation performed in BuildSyncers in your tests but then we have this additional test only check here that makes sure we don't have an invalid supports type in the bidder info file. Shouldn't we be checking for invalid support types within our app validation too?

Good point. It was my intention to emit a warning for supported user syncs which aren't configured. I've added that feature with the supports validation.

SyntaxNode · 2021-08-18T18:44:37Z

router/router_test.go

+			assert.Equal(t, test.expectedBidderInfos, test.givenBidderInfos, test.description+":result")
+		} else {
+			assert.EqualError(t, resultErr, test.expectedError, test.description+":err")
+		}


I realize that I'm not asserting the glog warnings. That's because we don't have a good way to intercept the log writes for tests. I'd like to see that fixed, but that would be too much work for this PR. I manually verified for now.

hhhjort · 2021-08-18T19:08:08Z

router/router.go

+			switch endpointLower {
+			case "iframe":
+				if info.Syncer.IFrame == nil {
+					glog.Warningf("bidder %s supports iframe user sync, but doesn't have a default and must be configured by the host", name)


Just wanted to confirm that you don't feel a need to test for the warning. You do test for the error below.

I added a comment about that lack of coverage a little while ago.

I'd really like to test the warning, but we don't have an interface for logging and intercepting glog through output redirection isn't a good option. I think adding that logging interface is too much to do here, so I leave it for a future project. There are many other glog warnings we equally have no automated test coverage on. I ran through the test cases manually to ensure the proper glog output.

I also thought of using warnings and errors as returns from the method instead using our existing errortypes functionality, but then I need to filter them out in the router and it seemed to be getting messy IMHO.

Happy to add the assertions if you have an idea for how to reasonable approach it.

bsardo

LGTM

hhhjort

LGTM

SyntaxNode · 2021-08-19T17:21:16Z

Docs PR: prebid/prebid.github.io#3220

… prebid-master * 'master' of https://github.com/prebid/prebid-server: Update Viper (prebid#1969) /cookie_sync Endpoint Rewrite (prebid#1879) New Adapter: IQZone (prebid#1964) Remove old race conidtion tests (prebid#1958) Remove time.Now() as the first parameter of NewRates() (prebid#1953) # Conflicts: # config/config.go # usersync/usersyncers/syncer_test.go

SyntaxNode and others added 30 commits April 20, 2021 17:04

Implement New Bidder Chooser

72be0a4

Added User Sync Chooser + Test Cleanup

8dffdd1

Remoe Unused customBidderTTLs

9da6cd3

Cookie Refactor

5e3160b

Large Development Checkpoint

0a0de85

Fixed Syncer Template Composition

c454f5a

Finished TestComposeTemplate

a934a7f

Finish NewSyncer Tests + Refactor

fe14358

Finished Syncers Builder + First Pass SetUID

86a8635

Add SetUID Tests

258d1ca

Add Proper Syncer Key Lookup In Exchange Logic

f71c5d5

Initial Support For Config Overrides

684d997

Stuff I Missed From The Last Commit

c3217a7

Fixed Metric Tests + Added Config Bindings

021d57d

Biddeer Info Tweaks

d2339cf

Self Code Review Pass

fbc0709

Merge branch 'master' into usersync_v2

1b6ba8a

Remove Go 1.16 Specific Type

5d6d6bf

Don't Create Syncer If Disabled

23052a3

Finish Config Integration

70fb2f8

Merge branch 'master' into usersync_v2

556fcf0

Fix Merge Conflicts

bc31e6c

Implement Legacy UserSyncURL Overrides

68a2710

Set UserSync Endpoints

02cc6a7

Bidder user sync configs

6f4950b

User Sync Metrics

24d04f7

Merge branch 'master' into usersync_v2

4cbde48

Code review fixes

973cfb2

Merge pull request #2 from SyntaxNode/bidder_configs

5f74c4c

Bidder user sync configs

Redirect URL Comment / Warning

a031639

Update InMobi Syncer

5649c77

hhhjort previously approved these changes Aug 11, 2021

View reviewed changes

bsardo previously approved these changes Aug 12, 2021

View reviewed changes

Allow Bidders To Represent Support Without Default

7d3bbb5

SyntaxNode dismissed stale reviews from bsardo and hhhjort via 7d3bbb5 August 12, 2021 18:12

SyntaxNode added 3 commits August 16, 2021 15:21

Revert "Allow Bidders To Represent Support Without Default"

d7389d3

This reverts commit 7d3bbb5.

Improved Errors

745eb3a

User Sync Supports Field

c12d936

SyntaxNode commented Aug 17, 2021

View reviewed changes

bsardo reviewed Aug 18, 2021

View reviewed changes

Emit Warning For Unconfigured Supported User Sync

c5fb9b5

SyntaxNode commented Aug 18, 2021

View reviewed changes

Fixed Test Typo

2cc6ec3

hhhjort reviewed Aug 18, 2021

View reviewed changes

bsardo previously approved these changes Aug 18, 2021

View reviewed changes

hhhjort previously approved these changes Aug 18, 2021

View reviewed changes

Merge branch 'master' into usersync_v2

18ea3ca

SyntaxNode dismissed stale reviews from hhhjort and bsardo via 18ea3ca August 19, 2021 17:35

hhhjort approved these changes Aug 19, 2021

View reviewed changes

bsardo approved these changes Aug 19, 2021

View reviewed changes

SyntaxNode merged commit 6e0b3de into prebid:master Aug 19, 2021

SyntaxNode mentioned this pull request Aug 20, 2021

Update Viper #1969

Merged

SyntaxNode deleted the usersync_v2 branch September 2, 2021 16:37

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

/cookie_sync Endpoint Rewrite (prebid#1879)

36d68a8

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

/cookie_sync Endpoint Rewrite (prebid#1879)

4911aa9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

/cookie_sync Endpoint Rewrite #1879

/cookie_sync Endpoint Rewrite #1879

SyntaxNode commented Jun 5, 2021 •

edited

Loading

hhhjort left a comment

bsardo left a comment

SyntaxNode Aug 17, 2021

SyntaxNode Aug 17, 2021

bsardo Aug 18, 2021

SyntaxNode Aug 18, 2021

bsardo Aug 18, 2021

SyntaxNode Aug 18, 2021

SyntaxNode Aug 18, 2021

hhhjort Aug 18, 2021

SyntaxNode Aug 18, 2021 •

edited

Loading

bsardo left a comment

hhhjort left a comment

SyntaxNode commented Aug 19, 2021

/cookie_sync Endpoint Rewrite #1879

/cookie_sync Endpoint Rewrite #1879

Conversation

SyntaxNode commented Jun 5, 2021 • edited Loading

Todo

Configuration Changes

API Changes

Metric Changes

Influx

Prometheus

Future Plans

hhhjort left a comment

Choose a reason for hiding this comment

bsardo 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

SyntaxNode Aug 18, 2021 • edited Loading

Choose a reason for hiding this comment

bsardo left a comment

Choose a reason for hiding this comment

hhhjort left a comment

Choose a reason for hiding this comment

SyntaxNode commented Aug 19, 2021

SyntaxNode commented Jun 5, 2021 •

edited

Loading

SyntaxNode Aug 18, 2021 •

edited

Loading