Debug warnings #1724

VeronikaSolovei9 · 2021-02-25T06:10:54Z

No description provided.

errortypes/code.go

VeronikaSolovei9 · 2021-02-25T06:28:34Z

exchange/bidder.go

@@ -87,6 +87,8 @@ type pbsOrtbSeatBid struct {
 	// if len(bids) > 0, this will become response.seatbid[i].ext.{bidder} on the final OpenRTB response.
 	// if len(bids) == 0, this will be ignored because the OpenRTB spec doesn't allow a SeatBid with 0 Bids.
 	ext json.RawMessage
+
+	warnings map[openrtb_ext.BidderName][]openrtb_ext.ExtBidderError


Warnings are actually Errors. Should I use errortypes.Warning instead? If yes - I need to add Code in it:

type Warning struct { Message string Code int }

I agree it's a bit strange to see warnings as a collection of errors. This only a concern in code with the type name. I'm not sure its worth having two separate but identical data structures with different names, but I don't feel too strongly if you prefer to see ExtBidderWarning.

It is reasonable to me that warnings are a subset of go "errors". If you need to define Warning in more than one package, then I would just add it to errortypes. If it appears in just one place, I don't have a strong feeling on it.

Too be clear, I don't have an issue with warnings map[openrtb_ext.BidderName][]openrtb_ext.ExtBidderError appearing in multiple places, just the idea of defining a Warning struct multiple places.

This new(or updated) Warning should also have json properties mapping.

type ExtBidderError struct { Code int `json:"code"` Message string `json:"message"` }

Please advice. I'll do what you guys think is better.

I lean towards just using ExtBidderError and using the Code value to determine the type. A "warning" can also be called a "non-fatal error", it is a problem, just not a problem severe enough to require aborting the processing. A new struct that is identical to an old struct should really just be used when type safety is an important concern. There is a large channel (communicating issues out to the requester) where we want to treat them interchangeably, to a degree, and just a few places where we want to separate them out. I am open to being convinced otherwise.

I'm confused.
Code in ExtBidderError is an error number like UnknownWarningCode = 10999 and it doesn't have severity level.
Code in Warning is Severity level and Warning doesn't have integer error code.

Do you mean I need to "consolidate" these structures?

I reviewed again and I see the warnings are their own thing now in the code. I suppose it depends on future plans, if we want to reuse some of the error machinery for warnings in the future, then we should keep them consolidated. If this is the extent we want to have warnings, then we can consider making them their own struct.

Per morning discussion I renamed ExtBidderError to more generic ExtBidderMessage.
Should we leave it all as it is now?

I am good with that.

hhhjort

LGTM

exchange/bidder.go

endpoints/openrtb2/amp_auction.go

exchange/exchange.go

VeronikaSolovei9 · 2021-03-09T04:20:49Z

All comments addressed, code refactored as discussed with team

hhhjort

LGTM

SyntaxNode

I successfully performed end to end testing with the auction endpoint for account and bidder debug warnings. Does this include AMP and Video endpoint warning wire-up?

I wasn't able to get a CCPA warning for the auction endpoint. Do you want to wire that up now or in a follow-up PR?

exchange/exchange_test.go

VeronikaSolovei9 · 2021-03-10T23:38:16Z

I tested it for video and auction endpoints, but not for AMP.
What is the use case for CCPA warning?

SyntaxNode · 2021-03-11T03:17:25Z

What is the use case for CCPA warning?

The CCPA string is invalid. It is removed from the request and a warning is returned to the publisher. An example invalid is the literal string "invalid".

VeronikaSolovei9 · 2021-03-15T10:16:39Z

Still need to add more tests for AMP endpoint

…nings

VeronikaSolovei9 · 2021-03-16T07:48:57Z

Ready for review

hhhjort

LGTM

exchange/exchange.go

endpoints/openrtb2/amp_auction_test.go

endpoints/openrtb2/auction_test.go

exchange/exchange.go

SyntaxNode · 2021-03-17T20:17:02Z

endpoints/openrtb2/auction.go

@@ -179,6 +180,7 @@ func (deps *endpointDeps) Auction(w http.ResponseWriter, r *http.Request, _ http
 		RequestType:  labels.RType,
 		StartTime:    start,
 		LegacyLabels: labels,
+		Warnings:     warnings,


To Other Reviewers: This is necessary evil right now since HoldAuction is also composing the endpoint response. We'll need to separate these behaviors in the future, but that's too much work to do here.

exchange/exchange_test.go

SyntaxNode · 2021-03-18T16:00:48Z

endpoints/openrtb2/amp_auction_test.go

+		{nil, false, nil},
+		{nil, true, tc2Wrnings},
+		{&openrtb_ext.ExtRegs{USPrivacy: "invalid"}, true, tc3Wrnings},
+		{&openrtb_ext.ExtRegs{USPrivacy: "1NYN"}, false, tc4Wrnings},


Nice. I think we can go one step further to using named field setters and embedding the expected warnings:

testData := []inputTest{ { regs: nil, invalidConsentURL: false, expectedWarnings: nil, }, { regs: nil, invalidConsentURL: true, expectedWarnings: map[openrtb_ext.BidderName][]openrtb_ext.ExtBidderMessage{ openrtb_ext.BidderReservedGeneral: {invalidCCPAWarning}, }, }, { regs: &openrtb_ext.ExtRegs{USPrivacy: "invalid"}, invalidConsentURL: true, expectedWarnings: map[openrtb_ext.BidderName][]openrtb_ext.ExtBidderMessage{ openrtb_ext.BidderReservedGeneral: {invalidCCPAWarning, invalidConsentWarning}, openrtb_ext.BidderAppnexus: {bidderWarning}, }, }, { regs: &openrtb_ext.ExtRegs{USPrivacy: "1NYN"}, invalidConsentURL: false, expectedWarnings: map[openrtb_ext.BidderName][]openrtb_ext.ExtBidderMessage{ openrtb_ext.BidderAppnexus: {bidderWarning}, }, }, }

SyntaxNode · 2021-03-18T16:01:40Z

endpoints/openrtb2/amp_auction_test.go

+type mockAmpExchangeWarnings struct{}
+
+func (m *mockAmpExchangeWarnings) HoldAuction(ctx context.Context, r exchange.AuctionRequest, debugLog *exchange.DebugLog) (*openrtb.BidResponse, error) {
+	//m.lastRequest = r.BidRequest


Please remove commented out code.

SyntaxNode · 2021-03-18T16:04:00Z

exchange/exchange.go

+	}
+
+	for _, warning := range r.Warnings {
+		//typedWarning := warning.(*errortypes.Warning)


Please remove commented out code.

VeronikaSolovei9 commented Feb 25, 2021

View reviewed changes

errortypes/code.go Show resolved Hide resolved

VeronikaSolovei9 commented Feb 25, 2021

View reviewed changes

SyntaxNode assigned SyntaxNode and hhhjort Feb 25, 2021

SyntaxNode requested a review from hhhjort February 25, 2021 18:22

DGarbar mentioned this pull request Mar 1, 2021

Support MultiBid feature prebid/prebid-server-java#1169

Merged

VeronikaSolovei9 force-pushed the debug_warnings branch from 60596aa to 340f9f5 Compare March 3, 2021 08:24

hhhjort previously approved these changes Mar 4, 2021

View reviewed changes

SyntaxNode reviewed Mar 4, 2021

View reviewed changes

exchange/bidder.go Show resolved Hide resolved

exchange/bidder.go Outdated Show resolved Hide resolved

exchange/bidder.go Outdated Show resolved Hide resolved

endpoints/openrtb2/amp_auction.go Show resolved Hide resolved

exchange/exchange.go Outdated Show resolved Hide resolved

VeronikaSolovei9 dismissed hhhjort’s stale review via 135730e March 8, 2021 06:18

hhhjort previously approved these changes Mar 9, 2021

View reviewed changes

SyntaxNode reviewed Mar 10, 2021

View reviewed changes

exchange/exchange_test.go Outdated Show resolved Hide resolved

exchange/exchange_test.go Outdated Show resolved Hide resolved

VeronikaSolovei9 dismissed hhhjort’s stale review via 21f2fc5 March 10, 2021 23:30

VeronikaSolovei9 force-pushed the debug_warnings branch from ae5620e to a2fe01b Compare March 15, 2021 10:20

Debug warnings

2f66d76

VeronikaSolovei9 force-pushed the debug_warnings branch from a2fe01b to 2f66d76 Compare March 15, 2021 10:31

Added unit tests for AMP endpoint for bidder specific and general war…

c4bf4a3

…nings

hhhjort previously approved these changes Mar 16, 2021

View reviewed changes

SyntaxNode reviewed Mar 17, 2021

View reviewed changes

Unit test fixes

285b352

VeronikaSolovei9 dismissed hhhjort’s stale review via 285b352 March 18, 2021 00:13

Veronika Solovei added 2 commits March 17, 2021 17:19

Unit test fixe

5f2ee2d

Unit test fix

2a79a61

SyntaxNode reviewed Mar 18, 2021

View reviewed changes

yevhenii-viktorov mentioned this pull request Dec 13, 2021

Adds implementation for DebugWarnings prebid/prebid-server-java#1626

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Debug warnings #1724

Debug warnings #1724

VeronikaSolovei9 commented Feb 25, 2021

VeronikaSolovei9 Feb 25, 2021

SyntaxNode Feb 25, 2021

hhhjort Feb 26, 2021

hhhjort Feb 26, 2021

VeronikaSolovei9 Feb 26, 2021

hhhjort Mar 1, 2021

VeronikaSolovei9 Mar 2, 2021 •

edited

Loading

hhhjort Mar 2, 2021

VeronikaSolovei9 Mar 3, 2021

hhhjort Mar 4, 2021

hhhjort left a comment

VeronikaSolovei9 commented Mar 9, 2021

hhhjort left a comment

SyntaxNode left a comment

VeronikaSolovei9 commented Mar 10, 2021

SyntaxNode commented Mar 11, 2021

VeronikaSolovei9 commented Mar 15, 2021

VeronikaSolovei9 commented Mar 16, 2021

hhhjort left a comment

SyntaxNode Mar 17, 2021

SyntaxNode Mar 18, 2021

SyntaxNode Mar 18, 2021

SyntaxNode Mar 18, 2021

Debug warnings #1724

Debug warnings #1724

Conversation

VeronikaSolovei9 commented Feb 25, 2021

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

VeronikaSolovei9 Mar 2, 2021 • 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

hhhjort left a comment

Choose a reason for hiding this comment

VeronikaSolovei9 commented Mar 9, 2021

hhhjort left a comment

Choose a reason for hiding this comment

SyntaxNode left a comment

Choose a reason for hiding this comment

VeronikaSolovei9 commented Mar 10, 2021

SyntaxNode commented Mar 11, 2021

VeronikaSolovei9 commented Mar 15, 2021

VeronikaSolovei9 commented Mar 16, 2021

hhhjort 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

VeronikaSolovei9 Mar 2, 2021 •

edited

Loading