Set updated tmax value to bidder's MakeRequest implementation

[onkarvhanumante]

Reducing the amount of time bidders have to compensate for the processing time used by PBS to,

• fetch a stored request (if needed)
• validate the OpenRTB request and split it into multiple requests sanitized for each bidder.
• time needed by PBS to prepare the auction response

[VeronikaSolovei9]

Added comments related to code optimization and better structuring.

Just want to confirm:
-do duration values in type TmaxAdjustments struct should be validated?
-do duration values in type TmaxAdjustments struct should be converted to Duration?
-will these values somewhow interact with timeout calculated earlier at the endpoint start?

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()
	}

[VeronikaSolovei9]

In config

TmaxAdjustments   TmaxAdjustments `mapstructure:"tmax_adjustments"`

is not a pointer.
Here in the AuctionRequest TmaxAdjustments is a pointer.
Do you think a little optimization makes sense here? Add this evaluation before assigning it here:

tmax := &deps.cfg.TmaxAdjustments
if !deps.cfg.TmaxAdjustments.Enabled {
	tmax = nil
}
....
auctionRequest := &exchange.AuctionRequest{
    TmaxAdjustments:            tmax,
}

It will simplify further checks from:

 if bidRequestOptions.tmaxAdjustments != nil && bidRequestOptions.tmaxAdjustments.Enabled && bidRequestOptions.tmaxAdjustments.BidderResponseDurationMin != 0 {...}

to

if bidRequestOptions.tmaxAdjustments != nil  && bidRequestOptions.tmaxAdjustments.BidderResponseDurationMin != 0 {...}

Basically bidRequestOptions.tmaxAdjustments.Enabled will not be needed.

[onkarvhanumante]

TmaxAdjustments is being passed/ planned to passed to various downstream functions and these functions will have access to tmaxAdjustments.Enabled field.

Also consider a scenario, if in future if following code was changed. Then if downstream function don't have tmaxAdjustments.Enabled check then behaviour of downstream function might change

tmax := &deps.cfg.TmaxAdjustments 
if !deps.cfg.TmaxAdjustments.Enabled { 
  tmax = nil 
}

[VeronikaSolovei9]

This block of code looks like a good candidate to be evaluated once at the server start up. These parameters come with the config and will not change per-request.

For the exchange.AuctionRequest you may use another, more convenient structure with some pre-processed values from configs.

Another point toward this idea is that you use same exact structure you used for config representation.
These structs should be separated (model-view-presenter design pattern). Look at the Activities example in exchange.AuctionRequest.

[onkarvhanumante]

This block of code looks like a good candidate to be evaluated once at the server start up. These parameters come with the config and will not change per-request.

0 is default value for BidderResponseDurationMin, .BidderNetworkLatencyBuffer and .PBSResponsePreparationDuration. So we cannot add them at server startup or validate them to return error if 0.

By checking these values !=0, we make sure that deadline from context are extracted and reduced only if needed. This saves us with some time

[VeronikaSolovei9]

Sorry, this is not what I mean, I didn't express myself clear. My general proposal here is to preprocess data from deps.cfg.TmaxAdjustments and save it to a new structure that will be passed to the endpoints.
Here is a draft with changes: VeronikaSolovei9#4
Please see my commit on top of your changes: VeronikaSolovei9@9965326

[onkarvhanumante]

@VeronikaSolovei9 thanks for the draft. For now I will defer to using config.TmaxAdjustments struct. Lets build out this feature and then we can do this enhancement. Being said that, lets see what other reviewers has feedback on this

[VeronikaSolovei9]

Why do you want to defer it? This change is simple and makes sense, it is more optimal than current approach and it's all done in draft so you see how it works already. It's better to do everything right at the beginning (especially simple things like this), rather then do it in the follow up PR that can possibly never happen or will be forgotten.

[onkarvhanumante]

@VeronikaSolovei9 fb19f5c makes the changes

[onkarvhanumante]

@VeronikaSolovei9 for this #2930 (review),

-do duration values in type TmaxAdjustments struct should be validated?

Zero is the default values of TmaxAdjustments fields. Also data type of TmaxAdjustments fields is uint which implies values can't be less than 0 and marshalling of config (pbs.yml) at start up would failed. So no additional validation is needed

-do duration values in type TmaxAdjustments struct should be converted to Duration?

these values are duration (amount of time) specified in ms

prebid-server/config/config.go

Lines 1545 to 1562 in 23fffa1

	// Tmax Adjustments enables PBS to estimate the tmax value for bidders, indicating the allotted time for them to respond to a request.
	// It's important to note that the calculated tmax is just an estimate and will not be entirely precise.
	// PBS will calculate the bidder tmax as follows:
	// bidderTmax = request.tmax - reqProcessingTime - BidderNetworkLatencyBuffer - PBSResponsePreparationDuration
	// Note that reqProcessingTime is time taken by PBS to process a given request before it is sent to bid adapters and is computed at run time.
	type TmaxAdjustments struct {
	// Enabled indicates whether bidder tmax should be calculated and passed on to bid adapters
	Enabled bool `mapstructure:"enabled"`
	// BidderNetworkLatencyBuffer accounts for network delays between PBS and bidder servers.
	// A value of 0 indicates no network latency buffer should be accounted for when calculating the bidder tmax.
	BidderNetworkLatencyBuffer uint `mapstructure:"bidder_network_latency_buffer_ms"`
	// PBSResponsePreparationDuration accounts for amount of time required for PBS to process all bidder responses and generate final response for a request.
	// A value of 0 indicates PBS response preparation time shouldn't be accounted for when calculating bidder tmax.
	PBSResponsePreparationDuration uint `mapstructure:"pbs_response_preparation_duration_ms"`
	// BidderResponseDurationMin is the minimum amount of time expected to get a response from a bidder request.
	// PBS won't send a request to the bidder if the bidder tmax calculated is less than the BidderResponseDurationMin value
	BidderResponseDurationMin uint `mapstructure:"bidder_response_duration_min_ms"`
	}

-will these values somewhow interact with timeout calculated earlier at the endpoint start?
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() }

yes timeout deadline earlier at the endpoint start is used further to get elapsed time for determining remaining duration

[VeronikaSolovei9]

I tried to test it locally with negative values. You are right, uint cannot be negative. Here is my pbs.json and unmarshalled values in config (this is a result of "underflow": maxInt minus valueFromConfig):

Also do you think sum of all durations in tmaxAdjustment should not exceed general request timeout?

[VeronikaSolovei9]

We discussed the validation question offline. It will be possibly added in future PRs due to the team decision that validation is optional here.

[VeronikaSolovei9]

Thank you for the changes, LGTM