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.

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

updated logic for userSync - new field filterSettings #2499

Merged
merged 5 commits into from
Jun 18, 2018

Conversation

jsnellbaker
Copy link
Collaborator

@jsnellbaker jsnellbaker commented May 4, 2018

Type of change

  • Feature
  • Does this change affect user-facing APIs or examples documented on http://prebid.org?

Description of change

This set of changes introduces a new field for the userSync API, called filterSettings. This field allows the publisher to configure independent filtering rules against a defined list of bidders (or all bidders) with a specified white-list/black-list filter option. These rules are configurable for both iframe and image pixels, so that you can have a white-list for bidders A, B, C for iframe pixels and a black list for bidder A for image pixels.

These changes are part of an intermediate step to deprecate several userSync API fields; specifically iframeEnabled, pixelEnabled and enabledBidders. While these fields are not fully deprecated and can still be used after this PR, the new filterSettings field will trump those settings if the new field exists (and is properly configured). As a contrived example, if pixelEnabled was set to false, but there was a filterSettings.image config object - the image pixels would be dropped as per the latter logic.

So long as the filterSettings iframe or image object config is defined and valid, it effectively the same value as pixelEnabled or iframeEnabled set to true. If the config options are invalid, then we fallback to the default config logic where all bidders can drop image pixels and no bidders can drop iframe pixels.

Below is a copy of the config object with all its possible fields/options:

userSync: {
  filterSettings: {
    iframe: {
      bidders: ['*'],      // '*' represents all bidders
      filter: 'include'
    },
    image: {
      bidders: ['appnexus', 'testBidder'],
      filter: 'exclude'
    }
  }
}

Other information

PR for #2386

@stale
Copy link

stale bot commented May 21, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label May 21, 2018
Copy link
Collaborator

@snapwich snapwich left a comment

Choose a reason for hiding this comment

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

I think we should have a default for filterType. probably 'include'. Also rather than doing ['*'] we should just do '*', I think that's a little less confusing.

I'm guessing more often than not people are just going to be enabling or disabling all pixels for a bidder, so it might be worth adding a 'all' or(*, but I kind of like 'all' better) option for the filterSettings keys, along with iframe and image

@stale stale bot removed the stale label May 22, 2018
@jsnellbaker
Copy link
Collaborator Author

@snapwich To clarify on your feedback, the new config could look like the following:

userSync: {
  filterSettings: {
    iframe: {
      bidders: '*',      // '*' represents all bidders
      filter: 'include'
    },
    image: {
      bidders: ['appnexus', 'testBidder'],
      filter: 'exclude'
    },
    all: {
      bidders: ['newBidder']
      // undefined filter = 'include'
    }
  }
}

If so - in the case someone sets up a config with the same bidder in both all and either of the other types, should we consider that an invalid config and fallback to the default logic?

@mkendall07
Copy link
Member

I don't like the all option because you can set ambiguous things then, such as this:

filterSettings : {
  all : { 
    {
      bidders: '*',
       filter: 'include'
    }
  },
iframe : { 
    {
      bidders: '*',
       filter: 'exclude'
    }
  }
}

Unless we disallow all with explicit settings...

@snapwich
Copy link
Collaborator

snapwich commented May 31, 2018

@mkendall07 I agree there can be some ambiguous cases. Perhaps they should throw errors? I'm just thinking from the perspective of this being used, It's most likely to just enable/disable pixels for bidders as opposed to types of pixels (although it's nice to have that fine control if needed).

If someone wants to enable/disable all pixels for rubicon (I'm guessing will be a common use case), w/o an all they need to know all the pixels that rubicon uses and/or list all the pixels types (which could possibly grow over time) and include rubicon in each entry.

I don't think it needs to be a blocker for me, but it makes sense I think. It's also something that could be added at a later point.

@jsnellbaker
Copy link
Collaborator Author

@snapwich As you may have seen, I've updated the code in this PR with an overall refactor as well as some of your points (the filter default of include and changing the bidder wildcard to just the string '*' instead of placing it in an array).

I've talked further with @mkendall07 on the all config field, and I'm going to try to implement it as well.

As I noted earlier, if the logic of the config is invalid - I am planning to throw a warning and revert to the old userSync logic (allow image pixels, block iframe). This is the current behavior if other parts of the filterSettings config is invalid, so we'll be consistent here too. If you have other thoughts on this part of the functionality, please let me know.

@jsnellbaker
Copy link
Collaborator Author

I have just pushed in some changes to support the all config option. As a note - it was very difficult to try to resolve the intended logic when the all config was mixed with the other two configs without adding more sets of rules for the end-user to follow.

In lieu of adding complexity like that, I talked with @jaiminpanchal27 and we thought to implement the all config as being mutually exclusive with the image/iframe configs. So now, a user can either use filterSettings.all or use filterSettings.iframe and/or filterSettings.image. If they try to combine them, we'll return an error indicating an invalid config and will default to old logic.

Please let me know of any thoughts, comments, feedback on this new addition. Thanks.

@snapwich
Copy link
Collaborator

snapwich commented Jun 7, 2018

Does this need to be updated as part of this pull-request?

config.setDefaults({
'userSync': {
syncEnabled: true,
pixelEnabled: true,
syncsPerBidder: 5,
syncDelay: 3000
}
});

@jsnellbaker
Copy link
Collaborator Author

@snapwich No - that config will still preserve the native functionality of the userSync feature if you don't declare the filterSettings object. Eventually when we rework the feature and make breaking changes (ie removing some fields), then that would likely change.

Copy link
Collaborator

@snapwich snapwich left a comment

Choose a reason for hiding this comment

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

LGTM. Is this a breaking change? Does it change default behavior for pixels?

@jsnellbaker
Copy link
Collaborator Author

There are no breaking changes here as per @bretg's request on #2386. The default behavior (where all bidders can drop image pixels and no bidders can drop iframe pixels) is still in place.

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.

Looks great! Nice code.

@mkendall07 mkendall07 merged commit 6e946d8 into master Jun 18, 2018
@mkendall07 mkendall07 deleted the userSync_changes branch June 18, 2018 19:21
@EskelCz
Copy link
Contributor

EskelCz commented Jan 18, 2023

@jsnellbaker Sorry to reopen this topic but I can't find the answer anywhere. Why is iframe sync disabled by default? It seems that most bidders are encouraging publishers to enable them, so why not enable them by default? Thanks.

@jsnellbaker
Copy link
Collaborator Author

Hi @EskelCz

I believe the historical reason is that iframe usersyncs offer a lot more complexity and chance to run more sophisticated code that the publisher may not always be aware of up-front. Publishers should understand what a particular bidder's iframe usersync should be doing (in a high level at least) before giving them permission to run that code; which is why the default for iframe syncs was keep them disabled.

Adding @bretg @patmmccann @mkendall07 for any other reasons that may be relevant here.

@bretg
Copy link
Collaborator

bretg commented Jan 19, 2023

Of course bidders encourage publishers to enable -- it's a super-hypercharged open code block on the page.

Prebid is doing the right thing by making publishers think about who they trust with this privilege.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants