Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Criteo real time user sync #3930

Merged
merged 5 commits into from
Jul 9, 2019
Merged

Criteo real time user sync #3930

merged 5 commits into from
Jul 9, 2019

Conversation

jaiminpanchal27
Copy link
Collaborator

Type of change

  • Feature

Description of change

Bidders who work with Criteo can increase their demand by sending user id to Criteo. This PR adds criteo rtus id system. Bidders need to configure their own client identifier in order to get the user id. Use following snippet on page.

pbjs.setConfig({
    usersync: {
        userIds: [{
            name: "criteortus",
            params: {
              clientIdentifier: {
                "appnexus": 30
              }
            }
        }]
    }
});

This PR also removes storage restrictions in user id module. #3883
User id returned by criteo has max age of 1 hour and user id module stores the id in cookie or local storage for days. Workaround to do this was module can handle its own storage without hurting the auction timing.

TODO: Docs PR coming soon

mkendall07
mkendall07 reviewed Jun 19, 2019
View reviewed changes
modules/criteortusIdSystem.js
const onResponse = utils.delayExecution(afterAllResponses, bidders.length);

bidders.forEach((bidder) => {
let url = `https://gum.criteo.com/sync?c=${configParams.clientIdentifier[bidder]}&r=3`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this isn't going to scale out so well. Is that the only way Criteo supports (1 ID at a time?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, each bidder will have it's own client identifier. As per information we have now, this seems only option for now. There will be changes once we know more about this.

mkendall07
mkendall07 approved these changes Jun 19, 2019
View reviewed changes
Copy link
Member

@mkendall07 mkendall07 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM. Let's get @idettman to review as well

idettman
idettman reviewed Jun 20, 2019
View reviewed changes
Copy link
Contributor

@idettman idettman left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note, awaiting update to the Prebid.js submodule bundling system. A small change to this PR, specifically to the id system will be required once bundling change has been merged.

PR#3924

idettman
idettman reviewed Jun 24, 2019
View reviewed changes
modules/userId.js Outdated
@@ -298,6 +300,13 @@ function initSubmodules(submodules, consentData) {
} else if (submodule.config.value) {
// cache decoded value (this is copied to every adUnit bid)
submodule.idObj = submodule.config.value;
} else {
let result = submodule.submodule.getId(submodule.config.params, consentData);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If result is not reassigned, should be a const

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

idettman
idettman reviewed Jun 24, 2019
View reviewed changes
Copy link
Contributor

@idettman idettman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small change, the rest looks good

@idettman idettman self-requested a review June 24, 2019 17:15
idettman
idettman suggested changes Jun 24, 2019
View reviewed changes
Copy link
Contributor

@idettman idettman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small change for changing result from let to const

@jaiminpanchal27
Copy link
Collaborator Author

@idettman Updated to use submodules pattern and updated let to const

@jaiminpanchal27 jaiminpanchal27 merged commit 748f651 into master Jul 9, 2019
@jaiminpanchal27 jaiminpanchal27 mentioned this pull request Jul 9, 2019
Merged
@bretg
Copy link
Collaborator

bretg commented Jul 10, 2019

Docs PR prebid/prebid.github.io#1381

This was referenced Jul 11, 2019
Closed
Closed
leonardlabat pushed a commit to criteo-forks/Prebid.js that referenced this pull request Jul 30, 2019
@jaiminpanchal27 @leonardlabat
Criteo real time user sync (prebid#3930)
1124078 
* add criteo rtus submodule and user id changes

* update appnexus adapter to include criteo user id

* updated to submodules pattern
VideoReach pushed a commit to VideoReach/Prebid.js that referenced this pull request Aug 1, 2019
@jaiminpanchal27
Criteo real time user sync (prebid#3930)
927319a 
* add criteo rtus submodule and user id changes

* update appnexus adapter to include criteo user id

* updated to submodules pattern
@bretg bretg removed the needs docs label Sep 20, 2019
@bretg bretg removed the on hold label Sep 20, 2019
@bretg
Copy link
Collaborator

bretg commented Sep 20, 2019

@jaiminpanchal27 - noticed that several user ID modules aren't being supported by prebid server bid adapter, with criteortus being one of them.

Who were you working with at criteo on this module? We need to confirm with them what source value they'd like for user.ext.eids.source. e.g.

          {
                "source": "criteortus",    // or would they prefer criteo.com?
                "uids": [{
                    "id": "11111111"
                }]
            }

sa1omon pushed a commit to gamoshi/Prebid.js that referenced this pull request Nov 28, 2019
@jaiminpanchal27
Criteo real time user sync (prebid#3930)
66bb9fc 
* add criteo rtus submodule and user id changes

* update appnexus adapter to include criteo user id

* updated to submodules pattern
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@idettman idettman idettman requested changes

@mkendall07 mkendall07 mkendall07 approved these changes

@jsnellbaker jsnellbaker jsnellbaker approved these changes

Assignees

@jsnellbaker jsnellbaker

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jaiminpanchal27 @bretg @idettman @mkendall07 @jsnellbaker