[Draft] floors feature implementation #2341

PubMatic-OpenWrap · 2022-08-04T12:03:50Z

Started development for floors feature as mentioned in PRD #1162.
This PR contains changes for floors feature and mainly covers basic implementation of Signalling and Enforcement.

SyntaxNode

Thanks for the contribution. This is a large PR, so I have a large amount of comments. :)

config/config.go

SyntaxNode · 2022-08-04T16:01:05Z

config/config.go

+type PriceFloors struct {
+	Enabled           bool `mapstructure:"enabled"`
+	UseDynamicData    bool `mapstructure:"use_dynamic_data"`
+	EnforceFloorsRate int  `mapstructure:"enforce_floors_rate"`


What measurement applies to the rate?

This global level config, which defines percent chance that PBS should suppress any bids below the matched floor from entering the auction.
e.g. if EnforceFloorsRate = 100, floors enforcement would be applied to all the bids,
if EnforceFloorsRate = 50, floors enforcement would be applied to 50% of the bids

Gotcha. Can you please add a comment explaining that it's a random chance from 0 to 100.

SyntaxNode · 2022-08-04T16:01:35Z

config/config_test.go

+   enabled: true
+   use_dynamic_data: false
+   enforce_floors_rate: 100
+   enforce_deal_floors: true


Please change all values from their default for this test.

Default config is tested in TestDefaults

SyntaxNode · 2022-08-04T16:06:30Z

exchange/floors.go

+		var bidfloor bidFloor
+		bidfloor.bidFloorCur = bidRequest.Imp[i].BidFloorCur
+		if bidfloor.bidFloorCur == "" {
+			bidfloor.bidFloorCur = "USD"


I'm concerned this is a separate hardcoding of "default to USD". Is there a way we can reuse the existing code or tie it in better? Perhaps a default currency constant shared by both?

In case floor currency not provided, can we set it to req.curr[0] to avoid hardcoding ?

Sounds good to me. Just please be sure that req.curr[0] is guaranteed to exist at this point.

yes we can access req.curr[0]

exchange/floors.go

floors/rule.go

SyntaxNode · 2022-08-04T17:10:34Z

floors/rule.go

+			wt2 += 1 << (segNum - comb[j][k])
+		}
+		return wt1 < wt2
+	})


Consider extracting the sort algorithm and testing it separately.

This is getting tested in TestPrepareRuleCombinations(),
Let us know if it serves purpose.

@bsardo what is your opinion?

I agree it would be preferable to separate the sorting out into a separate function. I see that TestPrepareRuleCombinations is verifying the sorting but I think compartmentalizing the sorting algorithm and writing a small unit test specifically for that makes the code a bit easier to read/maintain and allows us to easily establish confidence that the sorting logic is working independent of the rule generation logic.

I do think though that we first need to solve the issue I described in a different comment regarding the rule generation algorithm complexity because it might affect this code.

openrtb_ext/floors.go

SyntaxNode · 2022-08-23T18:14:06Z

config/config.go

@@ -1217,6 +1218,11 @@ func SetupViper(v *viper.Viper, filename string) {
 	v.SetDefault("experiment.adscert.inprocess.domain_renewal_interval_seconds", 30)
 	v.SetDefault("experiment.adscert.remote.url", "")
 	v.SetDefault("experiment.adscert.remote.signing_timeout_ms", 5)
+
+	v.SetDefault("experiment.price_floors.enabled", true)


Please set this to false by default. We'll highlight the option to turn it on in the relevant release notes.

SyntaxNode · 2022-08-23T18:15:16Z

config/config.go

+type PriceFloors struct {
+	Enabled           bool `mapstructure:"enabled"`
+	UseDynamicData    bool `mapstructure:"use_dynamic_data"`
+	EnforceFloorsRate int  `mapstructure:"enforce_floors_rate"`


Gotcha. Can you please add a comment explaining that it's a random chance from 0 to 100.

config/experiment.go

SyntaxNode · 2022-08-23T18:18:30Z

exchange/floors.go

+		var bidfloor bidFloor
+		bidfloor.bidFloorCur = bidRequest.Imp[i].BidFloorCur
+		if bidfloor.bidFloorCur == "" {
+			bidfloor.bidFloorCur = "USD"


Sounds good to me. Just please be sure that req.curr[0] is guaranteed to exist at this point.

SyntaxNode

Thank for replying to the feedback so far. Sorry for the delay in a follow up review. There are more PRs submitted than usual this summer.

I focused this review on logic flow. I left out nitpicks and tests for now. I'd like to see alignment of the floorsExt setting with how bidder specific requests are typically created within the codebase. Please see me other comments for details.

SyntaxNode · 2022-08-23T18:31:15Z

exchange/floors.go

+// signalFloors function does singanlling of floors,
+// Internally validation of floors parameters and validation of rules is done,
+// Based on number of modelGroups and modelWeight, one model is selected and imp.bidfloor and imp.bidfloorcur is updated
+func signalFloors(r *AuctionRequest, floor floors.Floor, conversions currency.Conversions, responseDebugAllow bool) []error {


Thank you for adding the description. You may wish to consider changing the name of the method, perhaps selectFloorsAndModifyImp would be a more descriptive choice?

.. or perhaps updateFloorsInImps? That matches the comment on the call to this method.

renamed function to selectFloorsAndModifyImp()

SyntaxNode · 2022-08-23T20:04:19Z

exchange/exchange.go

@@ -293,6 +301,12 @@ func (e *exchange) HoldAuction(ctx context.Context, r AuctionRequest, debugLog *
 	var bidResponseExt *openrtb_ext.ExtBidResponse
 	if anyBidsReturned {

+		//If floor enforcement config enabled then filter bids
+		adapterBids, bidRejections := enforceFloors(&r, adapterBids, e.floor, conversions, responseDebugAllow)


If it's easier to return a slice of errors instead of bidRejections, feel free to go for it.

returned slice of errors

SyntaxNode · 2022-08-23T20:14:00Z

exchange/floors.go

+			seatBids, rejections = enforceFloorToBids(r.BidRequestWrapper.BidRequest, seatBids, conversions, enforceDealFloors)
+		}
+		requestExt.SetPrebid(prebidExt)
+		err = r.BidRequestWrapper.RebuildRequest()


enforceFloors is only called from HoldAuction. It's odd to see a change to the request happening here after it's already been sent to the bidder. I also find it confusing that prebidExt would be modified in floors.ShouldEnforce.

All code which modifies a request for a specific bidder is contained in cleanOpenRTBRequest. Could you please refactor this code to move this logic there? ... except for enforceFloorToBids as since that operates on the bids it needs to stay in this location.

refer #2341 (comment)

floors/enforce.go

SyntaxNode · 2022-08-23T20:22:21Z

floors/enforce.go

+	if floorExt.Enforcement.EnforcePBS == nil {
+		floorExt.Enforcement.EnforcePBS = new(bool)
+	}
+	*floorExt.Enforcement.EnforcePBS = shouldEnforce


Why does this update the floorExt in addition to returning the value? Can we just use the return value and not modify the request?

This update is done to handle below case:
When Enforcement.EnforcePBS is not defined and based on EnforceRate enforcement needs to be done or skipped,
In that case this value is updated with appropriate value.
And to update prebidExt due to this case requestExt.SetPrebid(prebidExt) is done in enforceFloors()

No other code in this PR check for EnforcePBS. Is this therefore a signal downstream to bidders so they know what behavior PBS performed? @bretg checking on the spec intention here.

SyntaxNode · 2022-08-23T20:25:51Z

floors/floors.go

+	}
+
+	floorData.ModelGroups, floorModelErrList = validateFloorModelGroups(floorData.ModelGroups)
+	if len(floorData.ModelGroups) == 0 {


Wouldn't we also want to report validation errors for the model groups which were rejected? We could send them down as warnings.

SyntaxNode · 2022-08-23T20:29:13Z

floors/validate.go

+	"github.com/prebid/prebid-server/openrtb_ext"
+)
+
+func validateFloorRules(Schema openrtb_ext.PriceFloorSchema, delimiter string, RuleValues map[string]float64) []error {


Please rename to something which indicates the rule values are modified. I would normally expect this to just return a list of errors, but it's also modifying the floors and filtering our invalid ones. Better yet, consider returning a new map of rules instead of modifying the original collection - and reassign after.

Renamed function to selectValidFloorModelGroups()

SyntaxNode · 2022-08-23T20:30:23Z

floors/floors.go

+		floorData.ModelGroups = selectFloorModelGroup(floorData.ModelGroups, rand.Intn)
+	}
+
+	if floorData.ModelGroups[0].Schema.Delimiter == "" {


Consider assigning a model group variable, since there is only one left at this point in the code. Something like:

modelGroup := floorData.ModelGroups[0]

SyntaxNode · 2022-08-23T20:31:46Z

floors/rule.go

+
+func updateImpExtWithFloorDetails(matchedRule string, imp *openrtb2.Imp, floorVal float64) {
+	imp.Ext, _ = jsonparser.Set(imp.Ext, []byte(`"`+matchedRule+`"`), "prebid", "floors", "floorRule")
+	imp.Ext, _ = jsonparser.Set(imp.Ext, []byte(fmt.Sprintf("%.4f", floorVal)), "prebid", "floors", "floorRuleValue")


You may not use jsonparser.Set. it is not production ready code and has caused panics for us in the past.

You need to change this to use the standard Go json library. We've tried to use jsonparser.Setbefore and it seg faulted on us in certain cases.

SyntaxNode · 2022-08-23T20:35:40Z

openrtb_ext/floors.go

+		return *Floors.Enabled
+	}
+	return true
+}


Please reorganized these structs such that the "parent" struct is listed before the "child" struct. I would expect to see PriceFloorRules declared first and for PriceFloorSchema to be declared after PriceFloorModelGroup, etc..

reorganised structures

mansinahar · 2022-08-29T12:15:22Z

exchange/floors.go

+		return errs
+	}
+	prebidExt := requestExt.GetPrebid()
+	if floor != nil && floor.IsFloorEnabled() && prebidExt.Floors.GetEnabled() && prebidExt.Floors != nil {


Shouldn't the prebidExt.Floors != nil check be before prebidExt.Floors.GetEnabled()?

mansinahar · 2022-08-29T12:54:38Z

floors/floors.go

+	ENFORCE_RATE_MAX       int    = 100
+)
+
+type Config struct {


What's the rationale behind having a separate config struct that is similar to the PriceFloors struct in experiment.go? Why can't we just define that struct here and use it in experiment.go as well?

To implement below interface this structure was introduced

mansinahar · 2022-08-29T12:56:15Z

floors/floors.go

+	return fc.EnforceDealFloors
+}
+
+type Floor interface {


What's the rationale behind this interface? Do you envision any other implementations of this interface?

no other implementation.
If we do not use Interface, then we might have to return config struct member values

mansinahar · 2022-08-29T17:06:13Z

floors/rule.go

+	} else {
+		skipRate = RootSkipRate
+	}
+	return skipRate > f(SKIP_RATE_MAX+1)


Should this be >= ? Consider a skipRate of 100. In that case we should be skipping enforcing this feature 100% of the time but if the random number selected would be 100 then this would return false

mansinahar · 2022-08-29T17:17:14Z

floors/validate.go

+	"github.com/prebid/prebid-server/openrtb_ext"
+)
+
+func validateFloorRules(Schema openrtb_ext.PriceFloorSchema, delimiter string, RuleValues map[string]float64) []error {


Agree with @SyntaxNode's comments. Can you also please rename the input parameters so that the first letter is in lower case? This is just to follow Go's convention of naming for parameters

mansinahar · 2022-08-29T17:34:35Z

floors/validate.go

+		if len(parsedKey) != len(Schema.Fields) {
+			// Number of fields in rule and number of schema fields are not matching
+			errs = append(errs, fmt.Errorf("Invalid Floor Rule = '%s' for Schema Fields = '%v'", key, Schema.Fields))
+			delete(RuleValues, key)


Looks like you might have missed a continue statement here. If it's an invalid rule, we don't want to execute logic on L20 and L21, right?

mansinahar · 2022-08-29T17:48:13Z

floors/rule.go

+	return skipRate > f(SKIP_RATE_MAX+1)
+}
+
+func findRule(RuleValues map[string]float64, delimiter string, desiredRuleKey []string, numFields int) (string, bool) {


Nitpick: Please rename RuleValues parameter to ruleValues in order to follow Go's naming convention

mansinahar · 2022-08-30T13:13:23Z

exchange/exchange.go

@@ -955,6 +969,7 @@ func (e *exchange) makeExtBidResponse(adapterBids map[openrtb_ext.BidderName]*pb
 		bidResponseExt.Debug = &openrtb_ext.ExtResponseDebug{
 			HttpCalls:       make(map[openrtb_ext.BidderName][]*openrtb_ext.ExtHttpCall),
 			ResolvedRequest: r.ResolvedBidRequest,
+			UpdatedRequest:  r.UpdatedBidRequest,


I agree with @SyntaxNode here. I think of resolved requests as the final request generated by PBS after adding/updating parts of the request based on certain logic. This includes merging stored requests and IMHO should also include changes made by the price floor feature.

Also, in addition to it being an expensive field as @SyntaxNode pointed out, it will affect network bandwidth as well as your response gets bigger.

bsardo · 2022-08-30T18:39:54Z

Hello @pm-jaydeep-mohite, thanks for contributing! Just for future reference, if you don't mind, please merge with master instead of force pushing after your PR has been assigned for review. Force pushing makes it hard to figure out what has changed since the initial review which leads to a lot of extra time spent by our team on the delta reviews, especially for larger PRs. 🙂

Removed interfaces used to get config values and used config structure to get values

SyntaxNode

Thank you for the updates and your patience for a re-review. Looking a lot better IMHO after the changes. I'm in the middle of reviewing tests and comparing the expected behavior against the spec. I hope to finish that mid next week.

SyntaxNode · 2022-09-16T17:46:18Z

config/experiment.go

@@ -26,6 +28,17 @@ const (
 // Experiment defines if experimental features are available
 type Experiment struct {
 	AdCerts ExperimentAdsCert `mapstructure:"adscert"`
+	// Floors feature configuration


Nitpick: This comment does not seem necessary.

removed comment

SyntaxNode · 2022-09-16T17:46:56Z

config/experiment.go

+type PriceFloors struct {
+	Enabled        bool `mapstructure:"enabled"`
+	UseDynamicData bool `mapstructure:"use_dynamic_data"`
+	// Floors enforcement rate, values can be 0 to 100,


Nitpick: Please follow Go standards and begin the comment with the type name, ie:
// EnforceFloorsRate values can be 0 ...

will update at other places as well

SyntaxNode · 2022-09-16T17:48:11Z

exchange/exchange.go

@@ -955,6 +969,7 @@ func (e *exchange) makeExtBidResponse(adapterBids map[openrtb_ext.BidderName]*pb
 		bidResponseExt.Debug = &openrtb_ext.ExtResponseDebug{
 			HttpCalls:       make(map[openrtb_ext.BidderName][]*openrtb_ext.ExtHttpCall),
 			ResolvedRequest: r.ResolvedBidRequest,
+			UpdatedRequest:  r.UpdatedBidRequest,


I'm cool with that.

SyntaxNode · 2022-09-16T17:55:49Z

exchange/floors.go

+	"github.com/prebid/prebid-server/openrtb_ext"
+)
+
+// Check for Floors enforcement for deals,


Nitpick: Please follow Go standards and begin each comment with the type of the thing the comment applies to. I won't make this any more comments for this nitpick if you could please apply the pattern throughout the PR.

Example in this case would be
``// checkDealsForEnforcement checks for floors enforcement...`

SyntaxNode · 2022-09-16T17:57:39Z

exchange/floors.go

+	}
+
+	for bidderName, seatBid := range seatBids {
+		eligibleBids := make([]*pbsOrtbBid, 0)


Performance Nitpick: Consider setting the initial capacity to len(seatBid.bids) to try and avoid array resizing. ie:
make([]*pbsOrtbBid, 0, len(seatBid.bids))

SyntaxNode · 2022-09-16T18:12:35Z

floors/floors.go

+	MODEL_WEIGHT_MAX_VALUE int    = 100
+	MODEL_WEIGHT_MIN_VALUE int    = 0
+	ENFORCE_RATE_MIN       int    = 0
+	ENFORCE_RATE_MAX       int    = 100


Nitpick: In Go, it's common to use CamelCase for constants instead of CAPS. ie DEFAULT_DELIMITER -> DefaultDelimiter. This is largely due to the capital letter at the start of the variable name indicating if the type is package private or exported. If no other package needs these constants, please start with a lower case letter.

updated to use CamelCase

SyntaxNode · 2022-09-16T18:15:34Z

floors/rule.go

+
+func updateImpExtWithFloorDetails(matchedRule string, imp *openrtb2.Imp, floorVal float64) {
+	imp.Ext, _ = jsonparser.Set(imp.Ext, []byte(`"`+matchedRule+`"`), "prebid", "floors", "floorRule")
+	imp.Ext, _ = jsonparser.Set(imp.Ext, []byte(fmt.Sprintf("%.4f", floorVal)), "prebid", "floors", "floorRuleValue")


You need to change this to use the standard Go json library. We've tried to use jsonparser.Setbefore and it seg faulted on us in certain cases.

SyntaxNode · 2022-09-16T18:16:58Z

floors/rule.go

+			wt2 += 1 << (segNum - comb[j][k])
+		}
+		return wt1 < wt2
+	})


@bsardo what is your opinion?

SyntaxNode · 2022-09-16T18:18:02Z

floors/validate.go

+			delete(ruleValues, key)
+			continue
+		}
+		delete(ruleValues, key)


Since delete will always happen, should it be moved up before the if statement?

SyntaxNode · 2022-09-16T18:19:43Z

floors/validate.go

+		errs = append(errs, fmt.Errorf("Invalid SkipRate at root level = '%v'", floorExt.SkipRate))
+	}
+	return errs
+}


validateFloorSkipRates will only return a single error. I recommend to change the return type to the singular error and the overall flow to:

if .. return fmt.Errorf(..)) if ... return fmt.Errorf(..)) return nil

It looks like we're still returning a slice of errors. If the intention is to return a single error I agree with @SyntaxNode's recommendation.
If the intention is to return multiple errors then the first return nested inside the first if block should be deleted.

SyntaxNode · 2022-09-16T18:28:31Z

We recently fixed a security issue with a dependency. Please merge from master to resolve.

PubMatic-OpenWrap · 2022-09-26T08:24:44Z

Thank you for the updates and your patience for a re-review. Looking a lot better IMHO after the changes. I'm in the middle of reviewing tests and comparing the expected behavior against the spec. I hope to finish that mid next week.

Thank you for reviewing changes,
Let us know once done with test case review so that can push all the changes in single commit

PubMatic-OpenWrap · 2022-09-26T08:30:50Z

Hello @pm-jaydeep-mohite, thanks for contributing! Just for future reference, if you don't mind, please merge with master instead of force pushing after your PR has been assigned for review. Force pushing makes it hard to figure out what has changed since the initial review which leads to a lot of extra time spent by our team on the delta reviews, especially for larger PRs. 🙂

Force pushing is done to commit changes after rebasing feature branch with latest master branch, which is done to view feature changes commit by commit on top of latest master.

If this approach is not feasible then we can merge changes from master.
@bsardo Please let me know your opinion.

bsardo · 2022-09-26T17:58:26Z

Hi @PubMatic-OpenWrap, we're familiar with rebasing and recommend using it when working on a PR in draft mode. However, once the review process begins, we strongly prefer merging with master because it keeps the PR history in tact and makes it much easier for reviewers to identify what has changed since their last review. This is especially important with larger PRs. As the PR get closer to its final state, the last thing we want to do is have to re-review 3k lines of code because of a force push when we may only be expecting a few small changes based on recent comments that can easily be observed with a merge with master. I hope this helps.

bsardo

Hi @PubMatic-OpenWrap,
I left a bunch of comments mostly around meeting the requirements. I'd also like to direct your attention to one comment in particular that involves the design. Please see my comment about O(2^N) complexity. I believe this should probably be resolved before making some of the other changes to the signaling logic.

Also it seems like you might have made some changes locally and forgot to push them to the PR based on your answers to some of @SyntaxNode's comments. You'll need to merge with master as well to resolve some recently introduced conflicts.

I still need to give the enforcement piece a closer look. I will do that shortly but I wanted to share this other feedback with you in the interim.

Thanks!

bsardo · 2022-10-11T03:51:29Z

exchange/floors.go

+		return errs
+	}
+	prebidExt := requestExt.GetPrebid()
+	if floor.Enabled && prebidExt.Floors != nil && prebidExt.Floors.GetEnabled() {


The requirements indicate that floors config can be specified at the host, account and request level. I don't see any account config changes in this PR. Is that something you were planning to add in a future PR?

Changes are done, will be pushed in next commit

bsardo · 2022-10-11T03:59:20Z

openrtb_ext/floors.go

+// PriceFloorRules defines the contract for bidrequest.ext.prebid.floors
+type PriceFloorRules struct {
+	FloorMin    float64                `json:"floormin,omitempty"`
+	FloorMinCur string                 `json:"floormincur,omitempty"`


Is this supposed to be float64 according to v2 of the schema?

according to v2 of schema float data type is mentioned,
should we use float32 or float64 ?

bsardo · 2022-10-11T13:40:51Z

openrtb_ext/floors.go

+	FloorMin    float64                `json:"floormin,omitempty"`
+	FloorMinCur string                 `json:"floormincur,omitempty"`
+	SkipRate    int                    `json:"skiprate,omitempty"`
+	Location    *PriceFloorEndpoint    `json:"location,omitempty"`


This is listed as a string with possible values ‘request’, ‘fetch’ or ‘noData’.

bsardo · 2022-10-11T13:45:07Z

openrtb_ext/floors.go

+	EnforcePBS    *bool `json:"enforcepbs,omitempty"`
+	FloorDeals    *bool `json:"floordeals,omitempty"`
+	BidAdjustment bool  `json:"bidadjustment,omitempty"`


Why is BidAdjustment not a pointer like the others? I see the default value is true while it is false for the pointer fields.

bsardo · 2022-10-11T13:50:57Z

openrtb_ext/floors.go

+type PriceFloorModelGroup struct {
+	Currency     string             `json:"currency,omitempty"`
+	ModelWeight  int                `json:"modelweight,omitempty"`
+	DebugWeight  int                `json:"debugweight,omitempty"` // Added for Debug purpose, shall be removed


Are we removing this field?

bsardo · 2022-10-20T16:13:59Z

floors/rule.go

+	value := CATCH_ALL
+	if isMobileDevice(request.Device.UA) {
+		value = Phone
+	} else if isTabletDevice(request.Device.UA) {
+		value = Tablet
+	}
+	return value


According to the requirements, "*" should only be returned if request.Device.UA does not exist so perhaps there should be an early return statement here:

if request.Device == nil || len(request.Device.UA) == 0 { return CATCH_ALL }

That nil check is probably needed anyway otherwise isMobileDevice and isTabletDevice could potentially try to access a field on a nil value.

bsardo · 2022-10-20T16:17:08Z

floors/rule.go

+	} else if isTabletDevice(request.Device.UA) {
+		value = Tablet
+	}
+	return value


The requirements also indicate that if request.Device.UA is present but it is not mobile or tablet then it should return "desktop":

if device.ua is present, resolve deviceType to: 'phone' if UA matches one of these patterns: "Phone", "iPhone", "Android.*Mobile", "Mobile.*Android" else 'tablet' if UA matches one of these: "tablet", "iPad", "Windows NT.*touch", "touch.*Windows NT", "Android" else 'desktop'

added desktop

bsardo · 2022-10-20T16:19:02Z

floors/rule.go

+}
+
+func isMobileDevice(userAgent string) bool {
+	isMobile, err := regexp.MatchString("(?i)Phone|iPhone|Android|Mobile", userAgent)


This regexp doesn't appear to match the requirements, particularly the android patterns:
'phone' if UA matches one of these patterns: "Phone", "iPhone", "Android.*Mobile", "Mobile.*Android"

bsardo · 2022-10-20T16:20:19Z

floors/rule.go

+}
+
+func isTabletDevice(userAgent string) bool {
+	isTablet, err := regexp.MatchString("(?i)tablet|iPad|Windows NT", userAgent)


This regexp doesn't appear to match the requirements, particularly the Windows NT and Android patterns:
else 'tablet' if UA matches one of these: "tablet", "iPad", "Windows NT.*touch", "touch.*Windows NT", "Android"

bsardo · 2022-10-20T16:27:14Z

floors/rule.go

+		if gptSlot != "" {
+			value = gptSlot
+		}


What are we supposed to return as the implied else condition here if gptSlot is an empty string? Currently we are returning an empty string but is that correct? I see that getpbsadslot(imp) returns "*" so should we be doing the same here?
I see that most of these functions will default to returning "*" which it appears you're doing so these values would at least match the wildcard in the generated rule combinations.

If gptSlot is an empty string then we are returning "*"

…mp object after floors signalling and enforcement

bsardo · 2023-06-05T18:17:18Z

Closing. This PR is no longer needed since the logic is covered by a series of other floors PRs.

SyntaxNode reviewed Aug 4, 2022

View reviewed changes

SyntaxNode self-assigned this Aug 4, 2022

bsardo requested a review from mansinahar August 15, 2022 17:31

bsardo assigned mansinahar Aug 15, 2022

SyntaxNode reviewed Aug 23, 2022

View reviewed changes

pm-jaydeep-mohite added 3 commits August 29, 2022 18:32

DRAFT: Changes for floors feature

428e4a3

Addressed review comments

6594e71

Addressed review comments on 30 Aug 2022

288e22c

pm-jaydeep-mohite force-pushed the floors branch from 70658a2 to 288e22c Compare August 30, 2022 13:16

mansinahar reviewed Aug 30, 2022

View reviewed changes

Addressed review comments,

4f82d9f

Removed interfaces used to get config values and used config structure to get values

PubMatic-OpenWrap changed the title ~~[WIP]: Changes for floors feature~~ floors feature implementation Sep 5, 2022

PubMatic-OpenWrap requested review from SyntaxNode and mansinahar and removed request for SyntaxNode and mansinahar September 5, 2022 10:48

bsardo assigned bsardo and unassigned mansinahar Sep 13, 2022

bsardo requested review from bsardo and removed request for mansinahar September 13, 2022 17:14

Replaced pbAdSlot with adUnitCode as per latest rule dimensions table

0208da5

pm-viral-vala mentioned this pull request Sep 16, 2022

OTT-656: Changes for addressing review comments on PR raised to merge changes in prebid master branch PubMatic-OpenWrap/prebid-server#361

Merged

4 tasks

SyntaxNode reviewed Sep 16, 2022

View reviewed changes

bsardo reviewed Oct 20, 2022

View reviewed changes

bretg mentioned this pull request Nov 18, 2022

Upgrade bid adjustments feature #2085

Closed

ShriprasadM mentioned this pull request Dec 2, 2022

Add support for Logging rejected bid scenarios using Prebid Analytics Module #2466

Closed

pm-jaydeep-mohite and others added 7 commits December 5, 2022 14:29

Merge branch 'master' into floors

9958b1c

Addressed review comments and implemented dynmaic fetch mechanism

643f238

Added code to update imp.ext.prebid for floors details

b7a0eae

Merge branch 'master' into floors

3030e5d

Updated resolved request after updates from floor module

4c095d4

Made RebuildRequestExt(), RebuildImp() public to update req.ext and i…

f913189

…mp object after floors signalling and enforcement

In case of multiple modelgroups, use valid group for signalling

60b7c49

bsardo assigned AlexBVolcy and unassigned SyntaxNode Feb 1, 2023

pm-jaydeep-mohite and others added 4 commits February 6, 2023 10:14

Merge branch 'master' into floors

dba8860

Fixed changes required due to latest master merge

668bcdd

Added supoort for dynamic fetch and metrics

d0228c2

Moved enforcement related code into floors module

a969f61

PubMatic-OpenWrap changed the title ~~floors feature implementation~~ [Draft] floors feature implementation Feb 7, 2023

bsardo marked this pull request as draft February 7, 2023 18:13

Updated test cases for EnforceFloors

c8a629e

bsardo closed this Jun 5, 2023

[Draft] floors feature implementation #2341

[Draft] floors feature implementation #2341

Conversation

PubMatic-OpenWrap commented Aug 4, 2022 • edited Loading

SyntaxNode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SyntaxNode Aug 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SyntaxNode left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsardo commented Aug 30, 2022

SyntaxNode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PubMatic-OpenWrap commented Aug 4, 2022 •

edited

Loading

SyntaxNode Aug 23, 2022 •

edited

Loading

SyntaxNode left a comment •

edited

Loading