Add `BidderResponseDurationMin` check #2848

onkarvhanumante · 2023-06-15T10:48:08Z

PR makes changes to add Tmax.BidderResponseDurationMin check in auction workflow. As per this change, PBS won't send request to bidder server if remaining duration less than BidderResponseDurationMin

- As of now, `bidderRequestStartTime` is part of `ExtraRequestInfo`. The ExtraRequestInfo is further passed as details to Adapter's MakeRequest() implementation. - We don't want to pass bidderRequestStartTime details to `MakeRequest()`. So remove bidderRequestStartTime from ExtraRequestInfo and add it in bidRequestOptions.

exchange/bidder_test.go

addresses: #2848 (comment)

gargcreation1992

lgtm!

The base branch was changed.

This reverts commit ba2c1a3.

The pbsRequestStartTime implies start point of auction handler endpoint. But received value is the start time of bidderRequest loop of GetAllBids[1]. So rename pbsRequestStartTime to bidderRequestStartTime Links: [1] https://github.com/prebid/prebid-server/blob/master/exchange/exchange.go#L698

onkarvhanumante · 2023-07-25T07:17:36Z

@Sonali-More-Xandr @gargcreation1992, all dependent PR are merged. However, they added some merge conflicts. So updating PR by pushing following commits to resolve conflicts. PTAL and review in following order:

gargcreation1992 · 2023-07-25T12:56:39Z

exchange/bidder.go

@@ -190,11 +193,11 @@ func (bidder *bidderAdapter) requestBid(ctx context.Context, bidderRequest Bidde
 		dataLen = len(reqData) + len(bidderRequest.BidderStoredResponses)
 		responseChannel = make(chan *httpCallInfo, dataLen)
 		if len(reqData) == 1 {
-			responseChannel <- bidder.doRequest(ctx, reqData[0], reqInfo.BidderRequestStartTime)
+			responseChannel <- bidder.doRequest(ctx, reqData[0], bidRequestOptions.bidderRequestStartTime, bidRequestOptions.tmaxAdjustments)
 		} else {


Could we combine two arguments and pass one argument bidRequestOptions instead?

prebid-server/exchange/bidder.go

Lines 65 to 72 in 1b6ebea

type bidRequestOptions struct {

accountDebugAllowed bool

headerDebugAllowed bool

addCallSignHeader bool

bidAdjustments map[string]float64

tmaxAdjustments *TmaxAdjustmentsPreprocessed

bidderRequestStartTime time.Time

}

bidRequestOptions holds different values which are not needed by doRequest. IMO better to pass only values those are needed

gargcreation1992

requesting one minor change. Rest lgtm!

guscarreon · 2023-07-25T20:48:13Z

exchange/tmax_adjustments.go

@@ -46,6 +47,9 @@ func (b *bidderTmaxCtx) Deadline() (deadline time.Time, ok bool) {
 	deadline, ok = b.ctx.Deadline()
 	return
 }
+func (b *bidderTmaxCtx) Until(t time.Time) time.Duration {
+	return time.Until(t)
+}

 func getBidderTmax(ctx bidderTmaxContext, requestTmaxMS int64, tmaxAdjustments TmaxAdjustmentsPreprocessed) int64 {


Can we write a short description of the logic behing the calculations this function performs?

bd1c910 adds description

guscarreon · 2023-07-25T21:35:14Z

exchange/tmax_adjustments.go

@@ -46,6 +47,9 @@ func (b *bidderTmaxCtx) Deadline() (deadline time.Time, ok bool) {
 	deadline, ok = b.ctx.Deadline()
 	return
 }
+func (b *bidderTmaxCtx) Until(t time.Time) time.Duration {
+	return time.Until(t)


Nitpick: Are we planning to add more functionality into Until in the future? If not, given that it doesn't access nor works with bidderTmaxCtx, maybe we can get rid of it and simply use the time library function. "time" is already listed in the imports of exchange/bidder.go so we wouldn't need the extra import either.

741 func hasShorterDurationThanTmax(ctx bidderTmaxContext, tmaxAdjustments TmaxAdjustmentsPreprocessed) bool { 742 if tmaxAdjustments.IsEnforced { 743 if deadline, ok := ctx.Deadline(); ok { 744 overheadNS := time.Duration(tmaxAdjustments.BidderNetworkLatencyBuffer+tmaxAdjustments.PBSResponsePreparationDuration) * time.Millisecond 745 bidderTmax := deadline.Add(-overheadNS) 746 747 - remainingDuration := ctx.Until(bidderTmax).Milliseconds() + remainingDuration := time.Until(bidderTmax).Milliseconds() 748 return remainingDuration < int64(tmaxAdjustments.BidderResponseDurationMin) 749 } 750 } 751 return false 752 } exchange/bidder.go

This could also be the case of RemainingDurationMS. Unless this is for testing purposes and I'm missing the big picture.

time.Until() internally calls time.Now(). Its difficult to test code path when value returned from time.Now() is used. By using ctx.Until() we can mock this return value.

Refer TestHasShorterDurationThanTmax unit test.

prebid-server/exchange/bidder_test.go

Line 3156 in 1b6ebea

ctx := &mockBidderTmaxCtx{startTime: startTime, deadline: deadline, now: now, ok: true}

In the test, mockBidderTmaxCtx is passed as implementer for biddetTmaxCtx. So during unit test mockBidderTmaxCtx.Until() is called.

prebid-server/exchange/bidder_test.go

Lines 3081 to 3083 in 1b6ebea

func (m *mockBidderTmaxCtx) Until(t time.Time) time.Duration {

return t.Sub(m.now)

}

By using ctx.Until() with mockBidderTmaxCtx, we can easily control the return value for testing purposes. This approach enables us to test specific code paths that were previously difficult time.Until().

How about avoiding time.Now() entirely by storing the openrtb2.BidRequest.TMax field in the tmaxAdjustments parameter and using it for the calculation. I believe both calculations and test cases could be simplified:

741 - func hasShorterDurationThanTmax(ctx bidderTmaxContext, tmaxAdjustments TmaxAdjustmentsPreprocessed) bool { + // hasShorterDurationThanTmax returns true if there's enough tmax to make the roundtrip to the bid server + // doing a calculation that involves the original bid request tmax + func hasShorterDurationThanTmax(tmaxAdjustments TmaxAdjustmentsPreprocessed) bool { 742 if tmaxAdjustments.IsEnforced { 743 - if deadline, ok := ctx.Deadline(); ok { 744 overheadNS := time.Duration(tmaxAdjustments.BidderNetworkLatencyBuffer+tmaxAdjustments.PBSResponsePreparationDuration) * time.Millisecond 745 - bidderTmax := deadline.Add(-overheadNS) 746 747 - remainingDuration := ctx.Until(bidderTmax).Milliseconds() + remainingDuration := tmaxAdjustments.BidRequestTmax - overheadNS 748 return remainingDuration < int64(tmaxAdjustments.BidderResponseDurationMin) 749 - } 750 } 751 return false 752 } exchange/bidder.go

We would have to copy the openrtb2.BidRequest.TMax into tmaxAdjustments.BidRequestTmax and recalculate it's value inside func (e *exchange) makeAuctionContext(ctx context.Context, needsCache bool) (exchange/exchange.go line 646) if necessary.

Do you think we can pull this off?

We need to get the elapsed time since start of auction handler function.
For this we are using, deadline context intialised at the start. Refer following:

prebid-server/endpoints/openrtb2/auction.go

Lines 208 to 213 in 35db00f

timeout := deps.cfg.AuctionTimeouts.LimitAuctionTimeout(time.Duration(req.TMax) * time.Millisecond)

if timeout > 0 {

var cancel context.CancelFunc

ctx, cancel = context.WithDeadline(ctx, start.Add(timeout))

defer cancel()

}

This elapsed time from deadline context is fetched using ctx.Deadline() and then BidderNetworkLatencyBuffer, PBSResponsePreparationDuration is deducted from resultant deadline to get bidderTmax.

I am not sure how using openrtb2.BidRequest.TMax we can get elapsed time

guscarreon · 2023-07-26T17:40:29Z

exchange/tmax_adjustments.go

@@ -47,6 +47,8 @@ func (b *bidderTmaxCtx) Deadline() (deadline time.Time, ok bool) {
 	deadline, ok = b.ctx.Deadline()
 	return
 }
+
+// returns the remaining duration until the specified time


Super nitpick: Can we use the usual Golang convention of prefixing the comment with the name of the function?

guscarreon · 2023-07-27T02:24:41Z

exchange/tmax_adjustments.go

@@ -46,6 +47,9 @@ func (b *bidderTmaxCtx) Deadline() (deadline time.Time, ok bool) {
 	deadline, ok = b.ctx.Deadline()
 	return
 }
+func (b *bidderTmaxCtx) Until(t time.Time) time.Duration {
+	return time.Until(t)


How about avoiding time.Now() entirely by storing the openrtb2.BidRequest.TMax field in the tmaxAdjustments parameter and using it for the calculation. I believe both calculations and test cases could be simplified:

741 - func hasShorterDurationThanTmax(ctx bidderTmaxContext, tmaxAdjustments TmaxAdjustmentsPreprocessed) bool { + // hasShorterDurationThanTmax returns true if there's enough tmax to make the roundtrip to the bid server + // doing a calculation that involves the original bid request tmax + func hasShorterDurationThanTmax(tmaxAdjustments TmaxAdjustmentsPreprocessed) bool { 742 if tmaxAdjustments.IsEnforced { 743 - if deadline, ok := ctx.Deadline(); ok { 744 overheadNS := time.Duration(tmaxAdjustments.BidderNetworkLatencyBuffer+tmaxAdjustments.PBSResponsePreparationDuration) * time.Millisecond 745 - bidderTmax := deadline.Add(-overheadNS) 746 747 - remainingDuration := ctx.Until(bidderTmax).Milliseconds() + remainingDuration := tmaxAdjustments.BidRequestTmax - overheadNS 748 return remainingDuration < int64(tmaxAdjustments.BidderResponseDurationMin) 749 - } 750 } 751 return false 752 } exchange/bidder.go

We would have to copy the openrtb2.BidRequest.TMax into tmaxAdjustments.BidRequestTmax and recalculate it's value inside func (e *exchange) makeAuctionContext(ctx context.Context, needsCache bool) (exchange/exchange.go line 646) if necessary.

Do you think we can pull this off?

onkarvhanumante added the do not port label Jun 15, 2023

Sonali-More-Xandr self-assigned this Jun 15, 2023

Sonali-More-Xandr previously approved these changes Jun 16, 2023

View reviewed changes

onkarvhanumante assigned gargcreation1992 Jun 19, 2023

gargcreation1992 reviewed Jun 19, 2023

View reviewed changes

exchange/bidder_test.go Outdated Show resolved Hide resolved

gargcreation1992 reviewed Jun 19, 2023

View reviewed changes

exchange/bidder_test.go Outdated Show resolved Hide resolved

gargcreation1992 reviewed Jun 19, 2023

View reviewed changes

exchange/bidder_test.go Outdated Show resolved Hide resolved

onkarvhanumante added a commit that referenced this pull request Jun 21, 2023

rename unit test

b19451a

addresses: #2848 (comment)

gargcreation1992 previously approved these changes Jun 22, 2023

View reviewed changes

bsardo added the blocked label Jun 28, 2023

onkarvhanumante changed the base branch from tmax/update-bidder-tmax to master July 25, 2023 06:03

oh0387 added 7 commits July 25, 2023 12:34

Revert "Prefer bidRequestOptions for bidderRequestStartTime"

ce318ef

This reverts commit ba2c1a3.

Merge branch 'master' into tmax/add-bidder-resp-min-check

3b6a8be

remove redundant check

eabc2e5

add TmaxAdjustmentsPreprocessed input param to doRequest

69a0a6a

add bidderResponseDurationMin check in doRequestImpl

f5f684b

move bidderRequestStartTime to bidRequestOptions struct

1b6ebea

onkarvhanumante force-pushed the tmax/add-bidder-resp-min-check branch from ec51657 to 1b6ebea Compare July 25, 2023 07:14

onkarvhanumante removed the blocked label Jul 25, 2023

Sonali-More-Xandr previously approved these changes Jul 25, 2023

View reviewed changes

gargcreation1992 reviewed Jul 25, 2023

View reviewed changes

guscarreon reviewed Jul 25, 2023

View reviewed changes

add description Until function

bd1c910

onkarvhanumante dismissed Sonali-More-Xandr’s stale review via bd1c910 July 26, 2023 12:17

onkarvhanumante requested review from guscarreon, gargcreation1992 and Sonali-More-Xandr July 26, 2023 12:22

guscarreon reviewed Jul 27, 2023

View reviewed changes

prefix method name in comment

2bf4022

gargcreation1992 approved these changes Jul 31, 2023

View reviewed changes

Sonali-More-Xandr approved these changes Jul 31, 2023

View reviewed changes

onkarvhanumante merged commit 8ffa1c6 into master Jul 31, 2023

onkarvhanumante deleted the tmax/add-bidder-resp-min-check branch July 31, 2023 11:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `BidderResponseDurationMin` check #2848

Add `BidderResponseDurationMin` check #2848

onkarvhanumante commented Jun 15, 2023

gargcreation1992 left a comment

onkarvhanumante commented Jul 25, 2023 •

edited

Loading

gargcreation1992 Jul 25, 2023

onkarvhanumante Jul 26, 2023 •

edited

Loading

gargcreation1992 left a comment

guscarreon Jul 25, 2023

onkarvhanumante Jul 26, 2023

guscarreon Jul 25, 2023

onkarvhanumante Jul 26, 2023

guscarreon Jul 27, 2023

onkarvhanumante Jul 27, 2023 •

edited

Loading

guscarreon Jul 26, 2023

onkarvhanumante Jul 27, 2023

guscarreon Jul 27, 2023

	type bidRequestOptions struct {
	accountDebugAllowed bool
	headerDebugAllowed bool
	addCallSignHeader bool
	bidAdjustments map[string]float64
	tmaxAdjustments *TmaxAdjustmentsPreprocessed
	bidderRequestStartTime time.Time
	}

	func (m *mockBidderTmaxCtx) Until(t time.Time) time.Duration {
	return t.Sub(m.now)
	}

	timeout := deps.cfg.AuctionTimeouts.LimitAuctionTimeout(time.Duration(req.TMax) * time.Millisecond)
	if timeout > 0 {
	var cancel context.CancelFunc
	ctx, cancel = context.WithDeadline(ctx, start.Add(timeout))
	defer cancel()
	}

Add BidderResponseDurationMin check #2848

Add BidderResponseDurationMin check #2848

Conversation

onkarvhanumante commented Jun 15, 2023

gargcreation1992 left a comment

Choose a reason for hiding this comment

onkarvhanumante commented Jul 25, 2023 • edited Loading

Choose a reason for hiding this comment

onkarvhanumante Jul 26, 2023 • edited Loading

Choose a reason for hiding this comment

gargcreation1992 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

onkarvhanumante Jul 27, 2023 • 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

Add `BidderResponseDurationMin` check #2848

Add `BidderResponseDurationMin` check #2848

onkarvhanumante commented Jul 25, 2023 •

edited

Loading

onkarvhanumante Jul 26, 2023 •

edited

Loading

onkarvhanumante Jul 27, 2023 •

edited

Loading