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

[PGE-178206904] Send Transaction ID to STX #1544

Closed
wants to merge 3 commits into from

Conversation

maphe
Copy link

@maphe maphe commented Aug 31, 2022

source.tid is transaction id at the request level
bidReq.ortb2Imp may carry a transaction id at the impression level (imp.ext.tid)

While Prebid is still figuring out how they want to do it, this is following the same implementation as TheTradeDesk did in their own adapter so we know the transaction id is at least consistent with them (since they're the one asking for it in the first place). My understanding is that if ever the Prebid committee changes the spec, they will be the one updating all adapters like they did in the past for other changes.

Followed recommendations provided by contributors here: prebid#8573

PGE-178206904

`source.tid` is transaction id at the request level
`bidReq.ortb2Imp` may carry a transaction id at the impression level (`imp.ext.tid`)

Followed recommendations provided by contributors here: prebid#8573

PGE-178206904
@maphe maphe requested a review from a team August 31, 2022 18:35
@maphe maphe self-assigned this Aug 31, 2022
@maphe maphe requested review from javadreamer, jefftmahoney and balbinas and removed request for a team August 31, 2022 18:35
Copy link

@javadreamer javadreamer left a comment

Choose a reason for hiding this comment

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

No changes per se, just a requested sanity check with STX.

That said. Thank you for including the notes to the github issue with Pat McCann. Very detailed conversation and great questions and general back and forth. 👍

const impression = {};
const impression = { ext: {} };

mergeDeep(impression, bidReq.ortb2Imp);

Choose a reason for hiding this comment

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

Are we sure that we want this - since it merges everrything in from the bid request? Will STX properly ignore what its not expecting to receive in the bid request? Are we sure that there risk is minimal of impression requests being potentially larger than expected (full of additional fields that mean nothing to the exchange?)

I recommend sanity checking, if you haven't already done so, this approach with STX Core, specifically via the universal endpoint. Not opposed per se, but want to make sure supply and exchange are aligned on this change.

Copy link
Author

Choose a reason for hiding this comment

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

Well, it doesn't merge "everything", it merges what Prebid allows the publishers to inject in the requests, from the doc:

ortb2Imp is used to signal OpenRTB Imp objects at the adUnit grain. Similar to the global ortb2 field used for global first party data configuration, but specific to this adunit. The ortb2Imp object currently supports first party data including the Prebid Ad Slot and the interstitial signal.

from which everything lives in the ext field.

I can ask STX but I believe we already have a some fields that STX does not care about but DSPs do, so it would fall into the same category. I think the universal endpoint mostly acts as a passthrough since both input and output are openRTB formatted, allowing us at the adapter level to leverage as much Prebid features as we can and let DSPs read or ignore as they will.

Copy link
Author

Choose a reason for hiding this comment

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

@maphe maphe requested a review from javadreamer September 6, 2022 21:47
@maphe
Copy link
Author

maphe commented Sep 13, 2022

Opened PR on official public repo: prebid#8988

@maphe maphe closed this Sep 13, 2022
@maxime-dupuis maxime-dupuis deleted the mp/PGE-178206904/transaction-id branch July 5, 2023 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants