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

Remove unsupported functions by IE, add support for floor price #1

Closed
wants to merge 7 commits into from

Conversation

Boris-Tang
Copy link
Member

@Boris-Tang Boris-Tang commented Oct 24, 2021

https://zube.io/delta-projects/dev/c/299
fix problem mentioned in prebid#7564
Note:

  1. need review to see if getFallbackBidFloor is really needed.
  2. need both @xi-guan-dev-delta and @rikdru approve this PR.
  3. in order to minimise the commits. This PR will not be merged into branch deltaprojects. Instead we might want to apply cherry pick from deltaprojects.

@Boris-Tang
Copy link
Member Author

@xi-guan-dev-delta, could you tell me if bidderParams is actually needed and how it should be used?

}
const wonPrice = Math.round(cpm * 1000000);
const wonPriceMacroPatten = /\$\{AUCTION_PRICE:B64\}/g;

Choose a reason for hiding this comment

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

Looks like the { and } is redundant, maybe rm it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like the { and } is redundant, maybe rm it?

'adm': '<iframe id="dsp_iframe_228285197" src="https//abc/${AUCTION_PRICE:B64}&creative_id=123"></script>',
.
It depends on the Macro we are using, from above test the macro is ${AUCTION_PRICE:B64}

Choose a reason for hiding this comment

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

True, I see, then we should keep it ~

@xi-guan-dev-delta
Copy link

@xi-guan-dev-delta, could you tell me if bidderParams is actually needed and how it should be used?

The bidderParams is to fill the imp.ext field, which is for exchange-specific extensions. If our dogfight don't support it / we don't have a plan to support it, then it's not needed. Otherwise, yes. To use it, need to specify it at where the bid unit is defined. Check below example or the unit test at here

bids: [{
    bidder: 'deltaprojects',
    params: {
      publisherId: '4'   //required,
      bidderParams: {...}
    }
  }]

@Boris-Tang
Copy link
Member Author

OK. now I understand. It sounds like an extra flexibility you want to give to the publishers. Currently we don't have any specific logic for supporting that in dogifght.
I'd suggest to remove this for now. This minimise the interface to take care and it might be very difficult to remove if some publishers overuse it.

@xi-guan-dev-delta
Copy link

OK. now I understand. It sounds like an extra flexibility you want to give to the publishers. Currently we don't have any specific logic for supporting that in dogifght. I'd suggest to remove this for now. This minimise the interface to take care and it might be very difficult to remove if some publishers overuse it.

Yes, sure ~

@Boris-Tang
Copy link
Member Author

@xi-guan-dev-delta getFallbackBidFloor keeps the old logic to define the floor price. But now prebid officially support the floor price model. I think we might don't need our own way to define the bid floor any more ?

@rikdru
Copy link

rikdru commented Oct 25, 2021

I'm experimenting with https://github.com/deltaprojects/Prebid.js/blob/master/integrationExamples/gpt/adUnitFloors.html and can get it to respect the price floors internally using gulp --modules=currency,deltaprojectsBidAdapter,priceFloors serve-fast (internally as in it doesn't let us win if we bid below the bidfloor) but we should really not bid at all right? It looks like the bidfloor isn't propagated to the bidrequest. Have you tried this @Boris-Tang?

@Boris-Tang
Copy link
Member Author

I'm experimenting with https://github.com/deltaprojects/Prebid.js/blob/master/integrationExamples/gpt/adUnitFloors.html and can get it to respect the price floors internally using gulp --modules=currency,deltaprojectsBidAdapter,priceFloors serve-fast (internally as in it doesn't let us win if we bid below the bidfloor) but we should really not bid at all right? It looks like the bidfloor isn't propagated to the bidrequest. Have you tried this @Boris-Tang?

No, I didn't try this. But according to your description, the dogfight logic might have some problem. I will also try this.

@rikdru
Copy link

rikdru commented Oct 25, 2021

I'm experimenting with https://github.com/deltaprojects/Prebid.js/blob/master/integrationExamples/gpt/adUnitFloors.html and can get it to respect the price floors internally using gulp --modules=currency,deltaprojectsBidAdapter,priceFloors serve-fast (internally as in it doesn't let us win if we bid below the bidfloor) but we should really not bid at all right? It looks like the bidfloor isn't propagated to the bidrequest. Have you tried this @Boris-Tang?

No, I didn't try this. But according to your description, the dogfight logic might have some problem. I will also try this.

I think dogfight is probably fine, but when I looked in the network tab at the request the adapter made to dogfight I didn't see any floor so that's why we still bid even though I set the floor to $100.

@xi-guan-dev-delta
Copy link

@xi-guan-dev-delta getFallbackBidFloor keeps the old logic to define the floor price. But now prebid officially support the floor price model. I think we might don't need our own way to define the bid floor any more ?

Seems starts the official support from 5.x. Not sure about the back capability. To remove it, need to specify we only support prebid 5.x version

@Boris-Tang
Copy link
Member Author

@xi-guan-dev-delta getFallbackBidFloor keeps the old logic to define the floor price. But now prebid officially support the floor price model. I think we might don't need our own way to define the bid floor any more ?

Seems starts the official support from 5.x. Not sure about the back capability. To remove it, need to specify we only support prebid 5.x version

There is no publisher integrated with the current implementation. We don't need to consider the capability for this version, and it will be the first version of delta adapter which might be included in the coming 5.x version of prebid.

@rikdru
Copy link

rikdru commented Oct 25, 2021

I'm experimenting with https://github.com/deltaprojects/Prebid.js/blob/master/integrationExamples/gpt/adUnitFloors.html and can get it to respect the price floors internally using gulp --modules=currency,deltaprojectsBidAdapter,priceFloors serve-fast (internally as in it doesn't let us win if we bid below the bidfloor) but we should really not bid at all right? It looks like the bidfloor isn't propagated to the bidrequest. Have you tried this @Boris-Tang?

No, I didn't try this. But according to your description, the dogfight logic might have some problem. I will also try this.

I think dogfight is probably fine, but when I looked in the network tab at the request the adapter made to dogfight I didn't see any floor so that's why we still bid even though I set the floor to $100.

The current method works if the publisher explicitly sets '*|banner|*': 100 for example in the floor price config. I was hoping that it would be an implicit catch all for all sizes but it looks like it isn't. Maybe it's good enough since they're mentioning "If your bid adapter cannot handle size specific floors, use ‘*’ to retrieve catch-all size floor if defined by the publisher or floor provider"

@Boris-Tang
Copy link
Member Author

I'm experimenting with https://github.com/deltaprojects/Prebid.js/blob/master/integrationExamples/gpt/adUnitFloors.html and can get it to respect the price floors internally using gulp --modules=currency,deltaprojectsBidAdapter,priceFloors serve-fast (internally as in it doesn't let us win if we bid below the bidfloor) but we should really not bid at all right? It looks like the bidfloor isn't propagated to the bidrequest. Have you tried this @Boris-Tang?

No, I didn't try this. But according to your description, the dogfight logic might have some problem. I will also try this.

I think dogfight is probably fine, but when I looked in the network tab at the request the adapter made to dogfight I didn't see any floor so that's why we still bid even though I set the floor to $100.

The current method works if the publisher explicitly sets '|banner|': 100 for example in the floor price config. I was hoping that it would be an implicit catch all for all sizes but it looks like it isn't. Maybe it's good enough since they're mentioning "If your bid adapter cannot handle size specific floors, use ‘*’ to retrieve catch-all size floor if defined by the publisher or floor provider"

It is the publisher's responsibility to make sure their price set up covers all case. Like you said we don't support floor per size.

Copy link

@rikdru rikdru left a comment

Choose a reason for hiding this comment

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

LGTM, let's see what they have to say.

Copy link

@xi-guan-dev-delta xi-guan-dev-delta left a comment

Choose a reason for hiding this comment

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

LGTM

@Boris-Tang
Copy link
Member Author

the changes have been made on branch deltaprojects. close this PR now.

@Boris-Tang Boris-Tang closed this Oct 26, 2021
@Boris-Tang Boris-Tang deleted the bug-fix-zube-299 branch October 28, 2021 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants