Skip to content

Comments

Kargo#1

Open
ssadman22 wants to merge 23 commits intomasterfrom
kargo
Open

Kargo#1
ssadman22 wants to merge 23 commits intomasterfrom
kargo

Conversation

@ssadman22
Copy link

@ssadman22 ssadman22 commented Jun 14, 2022

The Prebid PR exists here: #1
Prebid Kargo Readme: KargoGlobal/prebid.github.io#1

KRAK-3623 Add a prebid s2s adapter to the open source prebid repo

Based on the Prebid guide: https://docs.prebid.org/prebid-server/developers/add-new-bidder-go.html
Most of the code here is generic except for the unit tests and configs.

@ssadman22 ssadman22 requested a review from a team June 14, 2022 18:35
Copy link

@kjavaherpour kjavaherpour left a comment

Choose a reason for hiding this comment

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

This looks good! What else is needed from prebid for this?

@ssadman22
Copy link
Author

This looks good! What else is needed from prebid for this?

Just the documentation which is here KargoGlobal/prebid.github.io#1

Copy link

@juliangan07 juliangan07 left a comment

Choose a reason for hiding this comment

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

questions on mock params.

@juliangan07 juliangan07 requested review from a team and andyrusiecki June 20, 2022 22:28
Copy link
Member

@andyrusiecki andyrusiecki left a comment

Choose a reason for hiding this comment

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

This looks great! Just have a few suggestions for the extensions.

}
},

"required": ["tagid"]
Copy link
Member

Choose a reason for hiding this comment

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

If replacing this extension with adSlot then we can make it optional. Kraken already supports mapping an Imp's TagID (along with the Site.Id) to a Kargo ad slot, but having an explicit adSlot ext field would be more reliable if set.

Copy link
Author

Choose a reason for hiding this comment

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

I believe prebid sets this field as required, so it either needs to specify a parameter or have no parameters. If adSlot is optional, then this field should be left empty since Prebid Server Core will discard them.

Copy link
Author

Choose a reason for hiding this comment

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

Updated with adSlotID

"secure": 1,
"iframebuster": ["ALL"],
"ext": {
"adSlotID": "11523"
Copy link
Author

@ssadman22 ssadman22 Jun 24, 2022

Choose a reason for hiding this comment

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

Each impression will have an Imp.Ext.adSlotID and a Imp.tagid. Since the adSlotID is now required by Prebid S2S, Kraken can check this instead of appending "siteID" + . + "tagId" as shown here for Prebid requests:

"cat": ["IAB13"],
"w": 300,
"h": 300,
"ext": {"mediaType": "banner"}
Copy link
Author

Choose a reason for hiding this comment

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

We will now be sending the mediaType back to prebid as it is required by the adapter

@ssadman22 ssadman22 requested review from a team and andyrusiecki June 24, 2022 16:49
@ssadman22 ssadman22 requested a review from a team June 27, 2022 16:23
@ssadman22 ssadman22 added the enhancement New feature or update to an existing one label Jun 27, 2022
@nickllerandi
Copy link

@ssadman22 We can close this - right?

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

Labels

enhancement New feature or update to an existing one

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants