-
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
Prebid core: fix native trackers for Prebid Server; simplify native ORTB logic #8748
Conversation
@jbartek25 fyi |
66200ee
to
d9b3070
Compare
@dgirardi Apologies for a question not directly related to this PR but I'm getting confused. I'm the maintainer of the Improve Digital bid adapter. We recently migrated our adapter to oRTB and so I was eagerly waiting for #8086 (thank you for all the effort) to be merged. Now as part of #8701 I already started implementing logic to allow publishers to override the default native params via |
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.
One issue and a question.
- in file
native.js
at line 483 instead of
if (key in NATIVE_KEYS_THAT_ARE_NOT_ASSETS) continue;
change to
if (NATIVE_KEYS_THAT_ARE_NOT_ASSETS.includes(key)) continue;
This is a leftover bug by me.
Regarding the trackers: I see that trackers are fired in PUC , file nativeORTBTrackerManager.js
... how is it supposed to work? Were your changes only related to legacy behaviour?
@jbartek25 before releasing any change I'd wait for the PUC to be relased and the documentation to be published... |
@musikele, with the PUC version that's currently out, it would send a message that triggers The PUC native PR wanted to move that logic to the PUC. For that to work it had to be merged & shipped first I think (it's something I missed when we were discussing the rollout). If you still want to go that route you would need to copy the logic in this PR over, it does not seem worth it to me - considering also that apparently some publishers prefer to not use the PUC (and do this instead). The PUC PR needs updates either way - it should fix the native tracker logic (like in here), or remove it. |
@jbartek25 there's also #8738 - and I am looking for adapters like yours to convert with it! (example of what I am doing with them, and documentation). It would automatically support native with the same logic used by the Prebid Server adapter, including this PR and #8086, as well as any other future updates. If you trust your test suite, or are willing to help me with verification in that PR, I can include your adapter - I am hoping to find a couple so that they can work as reference implementations. |
Yes, sure :-). Our adapter's test suite is pretty extensive + I can help with testing as needed. |
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.
Tested with jstracker, eventtracker and everything should work for legacy native (that is the one everybody's using).
In general, you cannot use mediatype.native.ortb until the PUC changes are adapted to support ortb responses in templates.
…RTB logic (prebid#8748) * Prebid core: simplify native ORTB logic * Remove nativeMapper; do not mix ORTB request and response * fix lint * set bidRequest.nativeParams after ortb -> legacy conversion * Fix native trackers for native ortb responses (including Prebid Server) * remove redundant native request generation from PBS adapter * use includes instead of in
…RTB logic (prebid#8748) * Prebid core: simplify native ORTB logic * Remove nativeMapper; do not mix ORTB request and response * fix lint * set bidRequest.nativeParams after ortb -> legacy conversion * Fix native trackers for native ortb responses (including Prebid Server) * remove redundant native request generation from PBS adapter * use includes instead of in
Type of change
Description of change
A followup to #8086, this:
javascriptTrackers
.nativeOrtbRequest
property that adapters can use instead of manually convertingbidRequest.nativeParams
(orbidRequest.mediaTypes.native
). I think this makes more sense because all requests derived from an ad unit will result in the same ORTB native request object.This is also in preparation of #8738, where I plan to re-use this logic for all ORTB adapters (including the PBS adapter, which currently duplicates a lot of the logic).
FYI @bretg @musikele; with this, the guidance to adapters that want to deal with native in ORTB should be to pick
bidRequest.nativeOrtbRequest
(instead ofconvertLegacyNativeRequestToOrtb(validBidRequests)
).