Liveintent liveconnect module #4803

melgenek · 2020-01-31T09:44:18Z

Type of change

Feature

Description of change

This PR introduces the following changes:

a dependency is added to use live-connect - an npm module for interaction with LiveIntent's systems
id resolution is rewritten to use this npm module
a pixel call per id resolution is added to the LiveIntent Id module. This is needed to build models on server-side for id resolution. Currently, LiveIntent's id resolution only works on the pages that contain LiveConnect's js tag on these pages as well as Prebid. The change in this pr allows using Prebid only
the decode method receives submodule-config in all cases except resolution callback

The live-connect npm module is open source and uses the same license as Prebid: Apache License 2.0.
The usage of external npm dependency has some benefits:

for LiveIntent it is easier to maintain the compatibility among js and server-side code bases. The code is reusable among Prebid, LiveIntent's js tag, other proprietary header bidding solutions
for Prebid it brings the benefit of having a code that is tested not only by unit/BrowserStack tests but also runs in real-world websites via LiveIntent's js tag

Other information

docs pr Add a new param for LiveIntent Id module. Describe the LiveIntent identity resolution. prebid.github.io#1783

Fire an event each time id is resolved.

bretg · 2020-02-10T17:06:29Z

@melgenek - note that live-connect is not allowed to add any code to the runtime package unless the optional user id module is brought in. Asking the reviewers @mkendall07, @jlukas79, @jsnellbaker to confirm this.

jlukas79

Validated only loaded into runtime package with userId + LiveIntent modules are brought in

jsnellbaker · 2020-02-13T19:39:39Z

@melgenek I discussed the approach of using the Liveconnet module here with other Prebid members, and unfortunately, this isn't something we want to permit in this case. There are a few reasons that came up to help provide some context:

There are mechanisms we have implemented to control how external requests from Prebid.js are handled through the ajax file. Using the module here to manage the calls would bypass those controls and make it tricky to handle (as it would be a one-off setup-wise and we'd have to work-around that if we needed to make a core change around that logic).

Additionally, while the imported package is open-source, it would be a bit difficult for our users and other maintainers to keep track of how updates to logic there would impact the logic here. Keeping the code together makes it easier to interpret and understand what exactly is happening.

Further, the included package would add some extra size to the build files (roughly 24.5KB between this PR and master) with code that may not be directly related to your logic.

I understand a big portion of this PR was to refactor the code to utilize this package. If you still want to implement the other changes that were noted here, I would ask to please remove the external package and revert the related package aspects of the code.

CC: @mkendall07 to help comment on this topic if needed.

jsnellbaker

See comment above on the requested changes.

jankoulaga · 2020-02-13T20:47:31Z

Hi @jsnellbaker. The concerns you raised are valid. However, in order to achieve what we wanted to do, we created an additional PR we asked review for here. At the time, the additional module wasn't acceptable, as it doesn't fit the current module structure. And in that lies the problem...

Pushing the code we write into Prebid.js, duplicating the code we have to write to maintain an open source project, re-writing tests is(we thought) and overhead. If we wanted to support the feature set we wanted to contribute, we'd have to re-produce the entire code-base we have developed, tested, and still, the increase in size of the bundle would be the same(not to mention the additional libs we'd have to use, would add more modules that are allowed).

The big portion of this PR is not to refactor the existing code, but rather to provide a mechanism and an identifier which can be leveraged using LiveIntent technology. The optimisation of the ajax call is IMHO a minor portion of it.

We considered live-connect-js as a module would actually bring value, as changes to it would require less review time considering the module being fairly simple, well tested, and it already encapsulates LiveIntent's API, so without a lot of manipulation in the prebid code we could adapt more easily for our current product and seamlessly apply those changes here.

If this PR is a hard no, we'd have to revert to the old idea of maintaining two separate code bases, which is exactly he reason why we went with this approach. Would it be reasonable to expect that a 1k+ line PR, which anyway increases the size of the bundle, would be approved?

bretg · 2020-02-14T01:18:30Z

Let me take a stab at summarizing this thorny issue:

LiveIntent wants to import a rather large foreign file into the prebid package so pubs that don't want to manually include their library can get it conveniently as part of the prebid package.
Prebid engineers are concerned this external code is doing things that unconstrained by Prebid core, like making AJAX calls and setting cookies.

Is that it?

My questions/observations:

Does this model support publishers that already load liveConnect.js on their pages? Seems like they would double-load that code.
Please be aware that we're soon going to clamp down on the setting of cookies -- you will need to follow GDPR rules about verifying consent for Purpose 1. The core will do that for you if you use core util functions. When we adopt that rule, we will reject external code that doesn't Do The Right Thing.
As long as publishers are clear that when they load liveIntent they're going to: (a) significantly increase the size of code on their page, (b) incur extra fetches on each page load, and (c) are using a piece of code not constantly reviewed by the Prebid engineering team, I would be inclined to let that informed publisher make the choice. To start with, you could update the sub-module documentation with these disclosures. I suspect that other sub-modules are also incurring additional fetches... happy to make them add disclosures too if someone does the research.
There is a Code of Conduct committee forming that aims to make edge cases like these easier to navigate. Standard methods of disclosure will probably be a part of the solution, and we will of course need to review all modules to make sure they're on equal footing.

jankoulaga · 2020-02-14T14:41:55Z

Thanks for your input @bretg. I think your questions are valuable, and will result in better docs we have to write in order to explain the purpose of the module. That being said, let me just clarify what the goal is...
Publishers who actually want to use the LiveIntent Id module for providing a stable id in the bid stream, currently require our own LiveConnect JS on their page. Now, consider a new publisher wanting to use that stable id. The publisher first needs to add our JS on the page, which is sometimes a really long process. Using our module inside of Prebid.js, we will get better resolution because we're able to collect the signal and place the first party identifier on a publisher's domain, and re-use it.

Let me address the issues you stated:

LiveIntent wants to import a rather large foreign file into the prebid package so pubs that don't want to manually include their library can get it conveniently as part of the prebid package.

This is correct, and what we actually want to achieve is that our module is not considered a foreign file. That's why it's open sourced, it should be clear what it does, and we will take additional steps to make sure that it's behaviour is not an enigma.

Prebid engineers are concerned this external code is doing things that unconstrained by Prebid core, like making AJAX calls and setting cookies.

I think this is also tied to 1), and we want to make sure we're clear on what calls are being made. There is only 1 ajax call, similar to the one which was implemented before this PR. When it comes to cookies, there's an option in LiveConnect to not set cookies at all(we will update the docs accordingly), but its main advantage is that this feature can be set by each publisher individually.

We want to play nice, and we want to be transparent.

To comment on your questions;

Does this model support publishers that already load liveConnect.js on their pages? Seems like they would double-load that code.

Yes, but building a true "identifier to bid stream environment" should mean that the means of building a stable identifier need to coexist in the header bidding solution. We think that this would actually help the publishers to remove the existing LiveIntent JS, while covering both header bidding, and the usual business we have with the publishers. It should bring the publishers a lot of value. Let's not forget, one cannot "accidentally" include the user module. Publishers that are interested in it, should enable it.
In short, adding this into Prebid.js should actually help, as one less resource to load(by removing our existing tag) should make thing easier, as there's one less connection, ssl handshake etc.

Please be aware that we're soon going to clamp down on the setting of cookies -- you will need to follow GDPR rules about verifying consent for Purpose 1. The core will do that for you if you use core util functions. When we adopt that rule, we will reject external code that doesn't Do The Right Thing.

Definitely. We will make sure that we are doing the right thing. Again, adoption by the community should actually force us to always keep things clean.

As long as publishers are clear that when they load liveIntent they're going to: (a) significantly increase the size of code on their page, (b) incur extra fetches on each page load, and (c) are using a piece of code not constantly reviewed by the Prebid engineering team, I would be inclined to let that informed publisher make the choice. To start with, you could update the sub-module documentation with these disclosures. I suspect that other sub-modules are also incurring additional fetches... happy to make them add disclosures too if someone does the research.

That has to be very clear and we will add that information. And again, a publisher must build their Prebid.js with clear understanding what it does, and building a version that includes this is something a publisher must do and agree on. Re (c), our opinion on the matter is that each PR which is a mere version bump should contain a link to our release notes and PRs that were included in that release, so that Prebid.js contributors and maintainers have a clear understanding of those changes.

To summarize, if you haven't checked out what live-connect does, it:

depending on the configuration, does or does not set a 1st party identifier(you can choose between LS, CookieJar, or no cookie setting at all)
also depending on the configuration, lets you synchronize first party identifiers
issues a pixel call towards LiveIntent collector by default. One can event implement its own data collection and use it for that. The data that's being sent as a plain image pixel can be found here
additionally, it provides a simple resolve function that will use the data already being used for collection to resolve the defined identifiers to a stable one, and that is the only AJAX call. Of course, the timeout can be specified via config.

Hope this information and exchange comes to fruition. IMHO the module shouldn't be something we're afraid of, and we'll gladly work more on improving it for greater adoption.

We'll be updating the PR with the docs, and this PR to make it explicit how to set the crucial params(storage strategy, ajax timeout, etc) soon, so hopefully we can resolve the state of this PR in a way that it fits the community and our ideas.

jankoulaga · 2020-02-19T18:10:12Z

Hi @bretg @jsnellbaker, we've updated our docs(linked in the description) hoping it would provide more transparency around the code submitted.

I hope those would be helpful, and combined with the comments i've added above, we can find a way to get this into Prebid.js with a full understanding that this module is added into the distro with strict understanding/opt-in by the publishers.

We're eagerly awaiting your comments on how we can improve this process, or how we can do better to solve future needs where identity & header bidding need to work together.

…ect-module

- storage manipulation provided by PBJS - ajax calls directly from PBJS

jankoulaga · 2020-03-03T12:29:15Z

Hi @bretg , @jsnellbaker ... We've updated our PR not only to contain more docs, but also to address some of your concerns regarding the mechanisms used to do ajax calls and how storage us used.
To facilitate that, we've updated our code to support a third party storage handler.

In this specific case, we're passing your utils(https://github.com/prebid/Prebid.js/pull/4803/files#diff-2ad40e308bea17ca3442a7e90c8da938R86) which will be used for storage getter/setters, and those are also now visible in the specs.

For the ajax call, we've modified our code to return the fully constructed url which should be used for id resolution, which is then performed by Prebid.js ajax module.

The specs are actually updated to display the behaviour of the live-connect-js module(ajax, cookie setting and pixel firing) to increase transparency.

Hopefully that brings us closer to approval. Let us know if there's more we can do.

jsnellbaker · 2020-03-03T16:31:33Z

src/utils.js

+export function findSimilarCookies(keyLike) {
+  const all = [];
+  const cookies = document.cookie.split(';');
+  while (cookies.length) {
+    const cookie = cookies.pop();
+    let separatorIndex = cookie.indexOf('=');
+    separatorIndex = separatorIndex < 0 ? cookie.length : separatorIndex;
+    const cookieName = decodeURIComponent(cookie.slice(0, separatorIndex).replace(/^\s+/, ''));
+    if (cookieName.indexOf(keyLike) >= 0) {
+      all.push(decodeURIComponent(cookie.slice(separatorIndex + 1)));
+    }
+  }
+  return all;


Hi @jankoulaga

I noticed with the recent set of commits, this new function was added to the utils file. I saw there was a unit test that used this function, but it seems that the test was later removed.

Should this function be removed as well? Or will there be some future update were it will be used?

Hey @jsnellbaker true... I added this spec in order to provide coverage, however, the spec did not pass in browserstack for a reason unknown to me.

When i execute this exact snippet in IE 11 && Win 10, it passes without a glitch, so it was hard for me to understand what the actual problem was.

As the utils object is now used in live-connect-js, this function is necessary.

Let's make a deal :) If you're in general ok with the approach we're taking here, i'll gladly work on re-adding a spec for this function ASAP. How does that sound?

I think the approach used for this function is fine.

If you can add the test back in, we can try to help with the debugging if there's something strange going with the BrowserStack tests.

I might also suggest adding a comment before the function to note that it is used by the live-connect package, so it's not altered/removed accidentally later (ie if someone tried to clean-up the utils functions by removing ones not used in any of the project src or modules files).

Cool, @jsnellbaker ... I will re-add the specs and figure out what's wrong, add all the right comments and i'll update the PR soon. In the meantime, we'd appreciate more input on what can/has to be fixed so we can speed things up and make the review process faster, so we don't have to address individual comments in multiple commits, if that's ok with you.

I looked at the rest of the changes and added notes, and I think those are fine.

The only other part I want to confirm is related to the consent information. I saw you're reading the USP values to pass along the consent string. Would you want to do this for the GDPR/TCF info as well? Or is that definitely not in-scope for your use cases?

I don't think it applies for now, as in general, we're fine with the approach of the module not being initialized if there's no GDPR consent given. It's food for thought, but as this was not implemented initially we didn't touch it. The scope of this PR was to introduce a more valuable feature, and i think we can re-iterate on the GDPR topic in the future.

Ok, thanks for the confirmation.

As a follow-up to the IE11 test issue, I did a little digging and think I found the issue. IE11 doesn't like cookies that have a blank (or undefined) value for the expires field. In that scenario, it was advised to not have the expires field-name at all when writing the cookie.

I did a local test where I updated the test's setCookie functions to pass '' in the expires argument and the tests passed in BrowserStack.

Can you give this a try?

@jsnellbaker i was just about to push that :) I guess the cookie part of the utils module needs a bit more tests ;). Let's see how it goes.

@jsnellbaker i hope you can trigger cci re-builds, as the current one seems to be stale on BS

I have restarted the job and the IE11 failure did not occur. The only failure that did happen was unreleated to the changes made by this PR, so that's fine.

…ain name.

ingmarzach · 2020-03-05T21:59:57Z

@mkendall07 Let us know if you have any questions. We're looking forward to your review.

mkendall07

Overall I'll put my approval on this PR, with the caveat that we need to handle these cases better as Bret pointed out. For now, assuming there is documentation on prebid.org corresponding to this change I'm good with it .

Just make sure to remove your integration example.

mkendall07 · 2020-03-11T02:16:53Z

allowedModules.js

@@ -13,7 +13,8 @@ module.exports = {
    ...sharedWhiteList,
    'criteo-direct-rsa-validate',
    'jsencrypt',
-    'crypto-js'
+    'crypto-js',
+    'live-connect'


would be nice to have a comment reference here to what this is - maintainer and link? Btw would be nice for all modules here :)

mkendall07 · 2020-03-11T02:17:36Z

integrationExamples/gpt/liveIntentId_example.html

@@ -0,0 +1,69 @@
+<!--


please drop this file. We need to keep the integration examples clean. This is too difficult for the core team to maintain. Documentation can go into prebid.org if you submit a PR there.

mkendall07 · 2020-03-11T02:21:54Z

also need a package.lock update @melgenek

…ect-module # Conflicts: # package-lock.json

…me was sent with an incorrect param name. PR for this change : LiveIntent/live-connect#4 The effects can be seen in the specs, where the incorrect `wrpn` sent as a pixel is now `wpn`

jankoulaga · 2020-03-17T21:04:54Z

Hi @mkendall07 , @jsnellbaker , how do we get this PR merged and released? We've already skipped two releases, and this PR hasn't seen much action after approval. Anything more we could do to make this happen?

# Conflicts: # package-lock.json # src/utils.js

* Use LiveIntent's liveconnect. Fire an event each time id is resolved. * Rename variables. Simplify imports. * Example page for LiveIntent Id module * Add application id param * Expose all possible configuration options for LiveConnect * Bumping version of live-connect-js to support: - storage manipulation provided by PBJS - ajax calls directly from PBJS * Removed spec whitespace. * Removed spec whitespace. * Removed spec for cookies * Reshuffled variables. * Re-added `findSimilarCookies` spec for further checks * Added a comment on the function to find all cookies that match a certain name. * Fixing setCookie not to include the expires as undefined. * Fixing setCookie not to include the expires as undefined. * Updated live-connect-js to 1.1.1, fixing the bug where the wrapper name was sent with an incorrect param name. PR for this change : LiveIntent/live-connect#4 The effects can be seen in the specs, where the incorrect `wrpn` sent as a pixel is now `wpn` * Removed whitespace. Co-authored-by: janko <janko.ulaga@gmail.com>

Yevhenii Melnyk added 4 commits January 29, 2020 18:01

Use LiveIntent's liveconnect.

8947886

Fire an event each time id is resolved.

Rename variables. Simplify imports.

17a4865

Example page for LiveIntent Id module

b020483

Add application id param

25fb35c

melgenek mentioned this pull request Jan 31, 2020

Add a new param for LiveIntent Id module. Describe the LiveIntent identity resolution. prebid/prebid.github.io#1783

Merged

melgenek requested review from bretg, idettman and jsnellbaker January 31, 2020 09:47

mike-chowla assigned jsnellbaker Feb 3, 2020

mike-chowla added the needs review label Feb 3, 2020

bretg requested review from jlukas79 and mkendall07 and removed request for idettman and bretg February 10, 2020 16:59

jlukas79 approved these changes Feb 12, 2020

View reviewed changes

jsnellbaker requested changes Feb 13, 2020

View reviewed changes

Expose all possible configuration options for LiveConnect

f857325

jankoulaga added 6 commits March 2, 2020 13:01

Merge remote-tracking branch 'origin/master' into liveintent-liveconn…

4e48ad5

…ect-module

Bumping version of live-connect-js to support:

8982917

- storage manipulation provided by PBJS - ajax calls directly from PBJS

Removed spec whitespace.

3e6a1ca

Removed spec whitespace.

879734d

Removed spec for cookies

bdb5586

Reshuffled variables.

3f5cbb2

jsnellbaker reviewed Mar 3, 2020

View reviewed changes

jankoulaga added 4 commits March 3, 2020 18:46

Re-added findSimilarCookies spec for further checks

3ecdc4b

Added a comment on the function to find all cookies that match a cert…

e9c56f9

…ain name.

Fixing setCookie not to include the expires as undefined.

bbcaf6e

Fixing setCookie not to include the expires as undefined.

5813382

jsnellbaker approved these changes Mar 3, 2020

View reviewed changes

jsnellbaker assigned mkendall07 and unassigned jsnellbaker Mar 3, 2020

jsnellbaker added needs 2nd review Core module updates require two approvals from the core team and removed needs review labels Mar 3, 2020

mkendall07 approved these changes Mar 11, 2020

View reviewed changes

mkendall07 added the LGTM label Mar 11, 2020

jankoulaga added 2 commits March 11, 2020 17:25

Merge remote-tracking branch 'origin/master' into liveintent-liveconn…

b07e812

…ect-module # Conflicts: # package-lock.json

Updated live-connect-js to 1.1.1, fixing the bug where the wrapper na…

240732a

…me was sent with an incorrect param name. PR for this change : LiveIntent/live-connect#4 The effects can be seen in the specs, where the incorrect `wrpn` sent as a pixel is now `wpn`

jankoulaga added 2 commits March 17, 2020 22:16

Merge branch 'master' into liveintent-liveconnect-module

e7b81cb

# Conflicts: # package-lock.json # src/utils.js

Removed whitespace.

9941fec

harpere merged commit 9fc9579 into prebid:master Mar 18, 2020

jaiminpanchal27 mentioned this pull request Mar 30, 2020

Live intent id system issue with storageManager #5048

Closed

deepthivenkat mentioned this pull request Jan 27, 2024

Module dependencies on open source libraries #10993

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Liveintent liveconnect module #4803

Liveintent liveconnect module #4803

melgenek commented Jan 31, 2020 •

edited

Loading

bretg commented Feb 10, 2020

jlukas79 left a comment

jsnellbaker commented Feb 13, 2020

jsnellbaker left a comment

jankoulaga commented Feb 13, 2020 •

edited

Loading

bretg commented Feb 14, 2020

jankoulaga commented Feb 14, 2020 •

edited

Loading

jankoulaga commented Feb 19, 2020

jankoulaga commented Mar 3, 2020

jsnellbaker Mar 3, 2020

jankoulaga Mar 3, 2020

jsnellbaker Mar 3, 2020

jankoulaga Mar 3, 2020 •

edited

Loading

jsnellbaker Mar 3, 2020

jankoulaga Mar 3, 2020

jsnellbaker Mar 3, 2020

jankoulaga Mar 3, 2020

jankoulaga Mar 3, 2020

jsnellbaker Mar 3, 2020

ingmarzach commented Mar 5, 2020

mkendall07 left a comment

mkendall07 Mar 11, 2020

mkendall07 Mar 11, 2020

mkendall07 commented Mar 11, 2020

jankoulaga commented Mar 17, 2020

Liveintent liveconnect module #4803

Liveintent liveconnect module #4803

Conversation

melgenek commented Jan 31, 2020 • edited Loading

Type of change

Description of change

Other information

bretg commented Feb 10, 2020

jlukas79 left a comment

Choose a reason for hiding this comment

jsnellbaker commented Feb 13, 2020

jsnellbaker left a comment

Choose a reason for hiding this comment

jankoulaga commented Feb 13, 2020 • edited Loading

bretg commented Feb 14, 2020

jankoulaga commented Feb 14, 2020 • edited Loading

jankoulaga commented Feb 19, 2020

jankoulaga commented Mar 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jankoulaga Mar 3, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ingmarzach commented Mar 5, 2020

mkendall07 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkendall07 commented Mar 11, 2020

jankoulaga commented Mar 17, 2020

melgenek commented Jan 31, 2020 •

edited

Loading

jankoulaga commented Feb 13, 2020 •

edited

Loading

jankoulaga commented Feb 14, 2020 •

edited

Loading

jankoulaga Mar 3, 2020 •

edited

Loading