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.

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

Rubicon: Update segtax logic #4224

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

Conversation

CTMBNara
Copy link
Contributor

No description provided.

@CTMBNara
Copy link
Contributor Author

CTMBNara commented Feb 18, 2025

Reverting reverted PR: #3915

Copy link

Code coverage summary

Note:

  • Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
  • Coverage summary encompasses all commits leading up to the latest one, c6f652c

rubicon

Refer here for heat map coverage report

github.com/prebid/prebid-server/v3/adapters/rubicon/rubicon.go:179:	appendTrackerToUrl			87.5%
github.com/prebid/prebid-server/v3/adapters/rubicon/rubicon.go:195:	Builder					100.0%
github.com/prebid/prebid-server/v3/adapters/rubicon/rubicon.go:207:	updateRequestTo26			92.3%
github.com/prebid/prebid-server/v3/adapters/rubicon/rubicon.go:232:	MakeRequests				80.6%
github.com/prebid/prebid-server/v3/adapters/rubicon/rubicon.go:545:	createImpsToExtMap			100.0%
github.com/prebid/prebid-server/v3/adapters/rubicon/rubicon.go:564:	prepareImpsToExtMap			100.0%
github.com/prebid/prebid-server/v3/adapters/rubicon/rubicon.go:583:	splitMultiFormatImp			61.5%
github.com/prebid/prebid-server/v3/adapters/rubicon/rubicon.go:620:	resolveBidFloor				100.0%
github.com/prebid/prebid-server/v3/adapters/rubicon/rubicon.go:628:	updateImpRpTarget			77.8%
github.com/prebid/prebid-server/v3/adapters/rubicon/rubicon.go:715:	extractDfpAdUnitCode			100.0%
github.com/prebid/prebid-server/v3/adapters/rubicon/rubicon.go:725:	isNotKeyPathError			100.0%
github.com/prebid/prebid-server/v3/adapters/rubicon/rubicon.go:729:	addStringAttribute			100.0%
github.com/prebid/prebid-server/v3/adapters/rubicon/rubicon.go:733:	addStringArrayAttribute			0.0%
github.com/prebid/prebid-server/v3/adapters/rubicon/rubicon.go:737:	updateUserRpTargetWithFpdAttributes	70.0%
github.com/prebid/prebid-server/v3/adapters/rubicon/rubicon.go:767:	updateExtWithIabAndSegtaxAttribute	100.0%
github.com/prebid/prebid-server/v3/adapters/rubicon/rubicon.go:781:	getTaxonomyIdToSegments			86.4%
github.com/prebid/prebid-server/v3/adapters/rubicon/rubicon.go:820:	pickRelevantSegments			100.0%
github.com/prebid/prebid-server/v3/adapters/rubicon/rubicon.go:853:	groupSegments				100.0%
github.com/prebid/prebid-server/v3/adapters/rubicon/rubicon.go:870:	populateFirstPartyDataAttributes	92.9%
github.com/prebid/prebid-server/v3/adapters/rubicon/rubicon.go:898:	isStringArray				100.0%
github.com/prebid/prebid-server/v3/adapters/rubicon/rubicon.go:908:	isBoolArray				100.0%
github.com/prebid/prebid-server/v3/adapters/rubicon/rubicon.go:918:	convertToStringArray			100.0%
github.com/prebid/prebid-server/v3/adapters/rubicon/rubicon.go:929:	rawJSONToMap				100.0%
github.com/prebid/prebid-server/v3/adapters/rubicon/rubicon.go:937:	mapFromRawJSON				80.0%
github.com/prebid/prebid-server/v3/adapters/rubicon/rubicon.go:946:	contains				100.0%
github.com/prebid/prebid-server/v3/adapters/rubicon/rubicon.go:955:	isVideo					100.0%
github.com/prebid/prebid-server/v3/adapters/rubicon/rubicon.go:964:	isFullyPopulatedVideo			100.0%
github.com/prebid/prebid-server/v3/adapters/rubicon/rubicon.go:969:	resolveNativeObject			88.2%
github.com/prebid/prebid-server/v3/adapters/rubicon/rubicon.go:1001:	setImpNative				82.4%
github.com/prebid/prebid-server/v3/adapters/rubicon/rubicon.go:1030:	MakeBids				92.2%
github.com/prebid/prebid-server/v3/adapters/rubicon/rubicon.go:1130:	mapImpIdToCpmOverride			90.0%
github.com/prebid/prebid-server/v3/adapters/rubicon/rubicon.go:1148:	resolveAdm				87.5%
github.com/prebid/prebid-server/v3/adapters/rubicon/rubicon.go:1163:	cmpOverrideFromBidRequest		100.0%
github.com/prebid/prebid-server/v3/adapters/rubicon/rubicon.go:1172:	updateBidExtWithMetaNetworkId		82.4%
total:									(statements)				84.8%

@CTMBNara
Copy link
Contributor Author

This error again... But I found the reason: since relevantSegments is a hash-based map, there is no guarantee of the order of the keys, and because of this we return ext.rp.target.XXX entries in random order. And then we fail when comparing json (although in fact all the necessary elements are there, they are just not shown in the error trace for some reason)

Comment on lines +167 to +168
github.com/golang-collections/collections v0.0.0-20130729185459-604e922904d3 h1:zN2lZNZRflqFyxVaTIU61KNKQ9C0055u9CAfpmqUvo4=
github.com/golang-collections/collections v0.0.0-20130729185459-604e922904d3/go.mod h1:nPpo7qLxd6XL3hWJG/O60sR8ZKfMCiIoNap5GvD12KU=
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can I safely delete these lines? The editor told me "The file should not be edited as it's intended to be used by Go tools only." XD

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can safely remove them since you added them as part of this change so they aren't on master. You can also run go mod tidy and that should remove just those two lines as I just tried it.

@bsardo
Copy link
Collaborator

bsardo commented Feb 20, 2025

This error again... But I found the reason: since relevantSegments is a hash-based map, there is no guarantee of the order of the keys, and because of this we return ext.rp.target.XXX entries in random order. And then we fail when comparing json (although in fact all the necessary elements are there, they are just not shown in the error trace for some reason)

Looking at the failed test, isn't segmentId1 supposed to be in this block though?
Screenshot 2025-02-20 at 9 54 08 AM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants