-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Adloox (adloox.com) RTD provider, analytic and adserver modules #6174
Conversation
408ef08
to
881ca29
Compare
I am unable to see what CircleCI is complaining about (it wants full read/write access to all my public/private repositories which I am not prepared to do) but tests work as expected for me and coverage for our modules is ~95%:
|
881ca29
to
6d987bc
Compare
60700ba
to
3c1f966
Compare
Hi @jimdigriz , from what I see that's a warning reported by linter, saying:
Tracing it to the source
|
I mentioned this in the 'Other information' above: "these intra-module imports do trip the the eslint's 'prebid/validate-imports' check which I have made a warning for now to highlight them during the review process; the plan is to later to disable those checks pre-merge" If this is the only reason CircleCI is upset, excellent means that everything is good at least on that front! My current plan is to have eslint ignore those errors completely unless an alternative approach is suggested. Thanks |
d5ac550
to
73217b7
Compare
modules/adlooxAdServerVideo.md
Outdated
|
||
Ad Server Video for adloox.com. Contact [email protected] for information. | ||
|
||
Prebid.js does support sending the [`bidWon` event for video](https://github.com/prebid/prebid.github.io/issues/1320) so for this situation this module can be used to synthesise this event for the [Adloox Analytics Adapter](./adlooxAnalyticsAdapter.md). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assume you mean does not support.
This statement is awkward and the purpose of this module is buried well into the documentation. Here's what I think you're trying to say:
"This module provides video events to the Adloox analytics adapter.
Prebid.js doesn't support bidsWon events for in-stream video, and the Instream Video Ads Tracking feature it does offer has limitations.
This module has two modes of operation configurable with the wrap
parameter below:
..."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I rejiggled it in fc2892e, let me know if it makes things clearer. Thanks
|
||
RTD provider for adloox.com. Contact [email protected] for information. | ||
|
||
In addition to populating the ad server key-value targeting, fetched segments (prefixed with `adl_...`) will also populate [First Party Data](https://docs.prebid.org/features/firstPartyData.html), some examples of the segments as described by the Adloox 'Google Publisher Tag Targeting Guidelines' and where they are placed are: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please describe the key-value targeting. Is this an integral part of the service or can it be broken out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I do not understand your question. Do not all RTD modules provide key-value targeting? What are you proposing or hinting could be broken out here?
Our clients make use of the key-value targeting in GAM as that is where (for example) they decide to swap in a house ad when an IVT segment lights up.
I guess I could add a flag whether to make getTargetingData()
a NOOP if this is what you are referring to?
modules/adlooxRtdProvider.md
Outdated
... | ||
|
||
realTimeData: { | ||
auctionDelay: 700, // recommended to be at least 700ms |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not like this recommendation at all. Nor the requirement that waitForIt to be true. Will discuss with the community.
The intention for RTD modules is that they are fast. 50ms. 100ms maybe. 700 is so far out of range it is likely to destroy a publishers ability to get successful bids.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 700ms is not for us, as described in the documentation below that, but as I have not seen publishers differentiate between office leased lines and contended mobile dongles we had to pick something; those that do use smarts would ignore this advice anyway. I settled on this number as there are only really two RTD modules with recommendations to glean from; browsiRtdProvider.js
(1000ms) and jwplayerRtdProvider.js
(100ms).
My understanding is that auctionDelay
is only a deadline, if the bidders (and RTD sources) all return before that time limit then that phase is over and we move on, right?
We definitely do not intend to delay anything by 700ms and we would be just as comfortable with a ~100ms which is enough to cross either the US or the EU...my packet-pushing-hat does raise an eyebrow that a 3G/4G phone could reliably meet that deadline whilst simultaneously pulling in publisher content. Does it? Anyway, more than happy to lower the threshold warning or better still replace it with a warning that we missed the auction instead?
On the waitForIt
note, can you describe the concern as the publisher controls this and not us. I do not understand how an RTD could function if the publisher is not willing to wait at all for it to call out to an API and pull in the segments to use during the auction process? I am missing something here, can you explain what I should be doing instead?
Maybe I am confused and should be using one of and not both waitForIt
or auctionDelay
and this is what I am doing wrong? I understood auctionDelay
to be a NOOP unless you have waitForIt
?
The behaviour I am looking for is that we want a maximum of Xms (auctionDelay
?) and if we miss that our RTD does not get to participate in setting FPD for the auction.
The idea behind This isn't well documented -- I'll come up with something. What we object to in this PR is that your module is specifically requiring at the code level to be one of the "important" passengers. Not allowed. You can ask the publisher in the docs to consider you important. You can show the customer data that the flag makes a difference.
Nope. Clearly the docs are lacking here. AuctionDelay is the required thing.
Thanks for pointing this out. @Asafsham - please lower this value in the Browsi docs. 100ms?
The sub-modules themselves shouldn't look at either value. They are control levers. Your sub-module should let the levers work and not complain about their values. Again, you're welcome to request that publishers set certain values in your documentation, but not enforce that in code.
This is what happens automatically. You get a chance to be a passenger on the bus. Be to the station on time or miss is. Your code should just be as fast as it can be.
|
In addition to Bret's feedback, which thanks for uncovering the gap in our documentation, we spoke with the community and we'd like to request that these are broken out into 3 separate PR's each for its own module. It's fine to denote in documentation that they work in tandem and communicate with one another, but the code itself should be implementable standalone without affecting any other prebid functionality. We'll then provide relevant feedback on each PR for each item in order to keep things clean -- thank you so much! |
Hopefully this will close the documentation gap -- prebid/prebid.github.io#2642 |
Not a problem. Quick question, once I have submitted those PRs and added references to them here, do you want me to close this PR (#6174 )? |
This workaround is needed for the demo to work DO NOT USE IN PRODUCTION!
fb7b601
to
eddc700
Compare
Type of change
N.B. not yet to be merged(!), submitting a PR to start the requests for comments and review process
Description of change
Introduction of analytics, RTD provider and Ad Server for Video modules that support Adloox's publisher services.
Disclosures:
BID_WON
adds the Adloox post-buy verification script (~25kB gzipped fromj.adlooxtracking.com
) and locates it immediately after the ad slot in the DOMp.adlooxtracking.com
) and provides no user orientated segments (only page, slot quality and IVT/fraud related); used to provide a consistent experience with our GPT clients as it also includes observability/tracing for our monitoring purposes which would not be appropriate to be included here.These modules automate the complex tasks an AdOps teams must perform when integrating with Adloox, including the script loading listed in the disclosures above. If the any of the scripts fail to load, it impacts only Adloox's measurement and does not affect the operation of Prebid.js or inventory fill for the publisher.
I do want to say, though early days for us with Prebid.js, it has been a positive experience from both the developer and AdOps perspectives. Our thanks go out to everyone involved in contributing and maintaining an exciting project that makes solving hard problems easy.
Other information
{client,tag,platform}id
,id[0-9]
parameters, ...)events.js
does not support custom events and is a shared event busj.adlooxtracking.com/ads/vast/tag.php
)correlator
related problems you get with GAM tagsgetGlobal().adUnits
in the RTD module; I looked to useauctionManager.getAdUnits()
but it does not seem to be initialised early on enough to have any adunits in there and though supposedly a no-no I see the other RTD modules are usinggetGlobal()
so I just copied them on this one. Is there something better I could be doing here?tag-dev.php
intotag.php
in production on Adloox's infrastructure