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.

Sign up for GitHub

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

Introduce SeatNonBid in hookoutcome #3416

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

Introduce SeatNonBid in hookoutcome #3416

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
8678237
Introduce SeatNonBid in hookoutcome
ashishshinde-pubm Jan 6, 2024
acef141
Merge branch 'master' of github.com:ashishshinde-pubm/prebid-server i…
ashishshinde-pubm Jan 6, 2024
c6525bc
Move the call to nonbids.Append in defer
ashishshinde-pubm Jan 18, 2024
47b5c6f
Merge branch 'master' of github.com/prebid-server into SNBR_HookStage
ashishshinde-pubm Jan 19, 2024
ffe2748
Address review comments
ashishshinde-pubm Jan 21, 2024
72e625e
introduce NewNonBid method
ashishshinde-pubm Jan 23, 2024
423a30f
Merge master branch
ashishshinde-pubm Jan 31, 2024
9450e40
Merge branch 'master'
ashishshinde-pubm Apr 22, 2024
6cd54de
fix unit tests
ashishshinde-pubm Apr 23, 2024
2dac953
Merge branch 'master' of github.com:ashishshinde-pubm/prebid-server i…
ashishshinde-pubm Jul 15, 2024
dbaff7e
fix UTs
ashishshinde-pubm Jul 15, 2024
35f2d02
Merging latest master
Pubmatic-Dhruv-Sonone Oct 18, 2024
e62b33a
Fix merge conflicts
Pubmatic-Dhruv-Sonone Oct 18, 2024
33bf0e5
Fixing UT
Pubmatic-Dhruv-Sonone Oct 18, 2024
fd14752
Merge remote-tracking branch 'upstream/master' into SNBR_HookStage
Pubmatic-Dhruv-Sonone Nov 6, 2024
dfc26e7
Reverting the usage of jsonutil
Pubmatic-Dhruv-Sonone Nov 22, 2024
f952f90
Fixing typo
Pubmatic-Dhruv-Sonone Dec 22, 2024
956c51e
Using foundErrors flag instead of ao.Response nil check
Pubmatic-Dhruv-Sonone Dec 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 32 additions & 29 deletions endpoints/openrtb2/amp_auction.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ func (deps *endpointDeps) AmpAuction(w http.ResponseWriter, r *http.Request, _ h
// We can respect timeouts more accurately if we note the *real* start time, and use it
// to compute the auction timeout.
start := time.Now()
seatNonBid := &openrtb_ext.NonBidsWrapper{}

hookExecutor := hookexecution.NewHookExecutor(deps.hookExecutionPlanBuilder, hookexecution.EndpointAmp, deps.metricsEngine)

Expand All @@ -138,6 +139,12 @@ func (deps *endpointDeps) AmpAuction(w http.ResponseWriter, r *http.Request, _ h
activityControl := privacy.ActivityControl{}

defer func() {
// if AmpObject.AuctionResponse is nil then collect nonbids from all stage outcomes and set it in the AmpObject.SeatNonBid
// Nil AmpObject.AuctionResponse indicates the occurrence of a fatal error.
if ao.AuctionResponse == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a more direct way for checking the occurrence of a fatal error instead of needing to explain the significance of an empty response in a comment? Hopefully one we can reuse in other parts of the code instead of copying/pasting this comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One direct way would be to call below 2 lines after every return statement.
seatNonBid.Append(getNonBidsFromStageOutcomes(hookExecutor.GetOutcomes()))
ao.SeatNonBid = seatNonBid.Get()

But currently there are around 6/7 return statement in Auction() & AmpAuction() function. To avoid the repetitive code, added this block in defer statement.
Thoughts ??

Copy link
Contributor

Choose a reason for hiding this comment

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

I think defer block is a good place for this.
Can we rely on the errors in ao? Looks like fatal errors should be added in ao in case of every error in the endpoint, like: ao.Errors = append(ao.Errors, rejectErr)

Same for equivalent logic in "auction.go"

Copy link
Collaborator

Choose a reason for hiding this comment

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

You could perhaps have a foundErrors boolean flag that starts false, and then is set to true whenever errors are detected. Then use this flag in the defer rather than checking for nil, resulting in a more straightforward way to satisfy the conditional, and futureproofing against any possible change where either an error happens after ao.AuctionResponse is set or it stays nil despite no error.

Copy link
Contributor

Choose a reason for hiding this comment

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

Used foundError instead of ao.Response nil check.

seatNonBid.Append(getNonBidsFromStageOutcomes(hookExecutor.GetOutcomes()))
ao.SeatNonBid = seatNonBid.Get()
}
deps.metricsEngine.RecordRequest(labels)
deps.metricsEngine.RecordRequestTime(labels, time.Since(start))
deps.analytics.LogAmpObject(&ao, activityControl)
Expand All @@ -164,7 +171,7 @@ func (deps *endpointDeps) AmpAuction(w http.ResponseWriter, r *http.Request, _ h
// Process reject after parsing amp request, so we can use reqWrapper.
// There is no body for AMP requests, so we pass a nil body and ignore the return value.
if rejectErr != nil {
labels, ao = rejectAmpRequest(*rejectErr, w, hookExecutor, reqWrapper, nil, labels, ao, nil)
labels, ao = rejectAmpRequest(*rejectErr, w, hookExecutor, reqWrapper, nil, labels, ao, nil, seatNonBid)
return
}

Expand Down Expand Up @@ -267,8 +274,9 @@ func (deps *endpointDeps) AmpAuction(w http.ResponseWriter, r *http.Request, _ h
var response *openrtb2.BidResponse
if auctionResponse != nil {
response = auctionResponse.BidResponse
seatNonBid.Append(auctionResponse.SeatNonBid)

}
ao.SeatNonBid = auctionResponse.GetSeatNonBid()
ao.AuctionResponse = response
rejectErr, isRejectErr := hookexecution.CastRejectErr(err)
if err != nil && !isRejectErr {
Expand All @@ -288,15 +296,21 @@ func (deps *endpointDeps) AmpAuction(w http.ResponseWriter, r *http.Request, _ h
glog.Errorf("/openrtb2/amp Critical error: %v", err)
ao.Status = http.StatusInternalServerError
ao.Errors = append(ao.Errors, err)
if ao.AuctionResponse != nil {
// this check ensures that we collect nonBids from stageOutcomes only once.
// there could be a case where ao.AuctionResponse nil and reqWrapper.RebuildRequest returns error
seatNonBid.Append(getNonBidsFromStageOutcomes(hookExecutor.GetOutcomes()))
ao.SeatNonBid = seatNonBid.Get()
}
Comment on lines +318 to +323
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this needed here? It duplicates logic from defer block

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in defer, we are collecting non-bids only when ao.AuctionResponse is nil.
This is one place, where we can get the ao.AuctionResponse as non-nil and defer will not collect the non-bids.

return
}

if isRejectErr {
labels, ao = rejectAmpRequest(*rejectErr, w, hookExecutor, reqWrapper, account, labels, ao, errL)
labels, ao = rejectAmpRequest(*rejectErr, w, hookExecutor, reqWrapper, account, labels, ao, errL, seatNonBid)
return
}

labels, ao = sendAmpResponse(w, hookExecutor, auctionResponse, reqWrapper, account, labels, ao, errL)
labels, ao = sendAmpResponse(w, hookExecutor, auctionResponse, reqWrapper, account, labels, ao, errL, seatNonBid)
}

func rejectAmpRequest(
Expand All @@ -308,12 +322,13 @@ func rejectAmpRequest(
labels metrics.Labels,
ao analytics.AmpObject,
errs []error,
seatNonBid *openrtb_ext.NonBidsWrapper,
) (metrics.Labels, analytics.AmpObject) {
response := &openrtb2.BidResponse{NBR: openrtb3.NoBidReason(rejectErr.NBR).Ptr()}
ao.AuctionResponse = response
ao.Errors = append(ao.Errors, rejectErr)

return sendAmpResponse(w, hookExecutor, &exchange.AuctionResponse{BidResponse: response}, reqWrapper, account, labels, ao, errs)
return sendAmpResponse(w, hookExecutor, &exchange.AuctionResponse{BidResponse: response}, reqWrapper, account, labels, ao, errs, seatNonBid)
}

func sendAmpResponse(
Expand All @@ -325,6 +340,7 @@ func sendAmpResponse(
labels metrics.Labels,
ao analytics.AmpObject,
errs []error,
seatNonBid *openrtb_ext.NonBidsWrapper,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this passed as a pointer / by reference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need to pass it as pointer. corrected it.

) (metrics.Labels, analytics.AmpObject) {
var response *openrtb2.BidResponse
if auctionResponse != nil {
Expand Down Expand Up @@ -352,6 +368,8 @@ func sendAmpResponse(
glog.Errorf("/openrtb2/amp Critical error unpacking targets: %v", err)
ao.Errors = append(ao.Errors, fmt.Errorf("Critical error while unpacking AMP targets: %v", err))
ao.Status = http.StatusInternalServerError
seatNonBid.Append(getNonBidsFromStageOutcomes(hookExecutor.GetOutcomes()))
Copy link
Contributor

Choose a reason for hiding this comment

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

Related to my above comment, seatNonBid could be nil here.. leading to a confusing situation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, made correction. Now passing the seatNonBid as a copy.

ao.SeatNonBid = seatNonBid.Get()
return labels, ao
}
for key, value := range bidExt.Prebid.Targeting {
Expand Down Expand Up @@ -380,7 +398,7 @@ func sendAmpResponse(
}
// Now JSONify the targets for the AMP response.
ampResponse := AmpResponse{Targeting: targets}
ao, ampResponse.ORTB2.Ext = getExtBidResponse(hookExecutor, auctionResponse, reqWrapper, account, ao, errs)
ao, ampResponse.ORTB2.Ext = getExtBidResponse(hookExecutor, auctionResponse, reqWrapper, account, ao, errs, seatNonBid)

ao.AmpTargetingValues = targets

Expand All @@ -406,6 +424,7 @@ func getExtBidResponse(
account *config.Account,
ao analytics.AmpObject,
errs []error,
seatNonBid *openrtb_ext.NonBidsWrapper,
) (analytics.AmpObject, openrtb_ext.ExtBidResponse) {
var response *openrtb2.BidResponse
if auctionResponse != nil {
Expand Down Expand Up @@ -435,6 +454,7 @@ func getExtBidResponse(
Warnings: warnings,
}

stageOutcomes := hookExecutor.GetOutcomes()
// add debug information if requested
if reqWrapper != nil {
if reqWrapper.Test == 1 && eRErr == nil {
Expand All @@ -446,7 +466,6 @@ func getExtBidResponse(
}
}

stageOutcomes := hookExecutor.GetOutcomes()
ao.HookExecutionOutcome = stageOutcomes
modules, warns, err := hookexecution.GetModulesJSON(stageOutcomes, reqWrapper.BidRequest, account)
if err != nil {
Expand All @@ -462,8 +481,12 @@ func getExtBidResponse(
}
}

setSeatNonBid(&extBidResponse, reqWrapper, auctionResponse)

// collect seatNonBid from all stage-outcomes and set in the response.ext.prebid
seatNonBid.Append(getNonBidsFromStageOutcomes(stageOutcomes))
ao.SeatNonBid = seatNonBid.Get()
if returnAllBidStatus(reqWrapper) {
setSeatNonBid(&extBidResponse, ao.SeatNonBid)
}
return ao, extBidResponse
}

Expand Down Expand Up @@ -843,23 +866,3 @@ func setTrace(req *openrtb2.BidRequest, value string) error {

return nil
}

// setSeatNonBid populates bidresponse.ext.prebid.seatnonbid if bidrequest.ext.prebid.returnallbidstatus is true
func setSeatNonBid(finalExtBidResponse *openrtb_ext.ExtBidResponse, request *openrtb_ext.RequestWrapper, auctionResponse *exchange.AuctionResponse) bool {
if finalExtBidResponse == nil || auctionResponse == nil || request == nil {
return false
}
reqExt, err := request.GetRequestExt()
if err != nil {
return false
}
prebid := reqExt.GetPrebid()
if prebid == nil || !prebid.ReturnAllBidStatus {
return false
}
if finalExtBidResponse.Prebid == nil {
finalExtBidResponse.Prebid = &openrtb_ext.ExtResponsePrebid{}
}
finalExtBidResponse.Prebid.SeatNonBid = auctionResponse.GetSeatNonBid()
return true
}
Loading
Loading