Conversation
…ring result function.
# Conflicts: # extra/modules/rule-engine/src/main/java/org/prebid/server/hooks/modules/rule/engine/core/request/result/functions/filter/FilterBiddersFunction.java
osulzhenko
left a comment
There was a problem hiding this comment.
pls split spec into:
- base (abstract)
- general
- aliases (optional)
- validation
- analytics
also add metrics verification:
success.no-invocation
success.noop
call
success.update
pom.xml
Outdated
| <configuration> | ||
| <systemPropertyVariables> | ||
| <launchContainers>false</launchContainers> | ||
| <launchContainers>true</launchContainers> |
| @JsonProperty("seatnonbid") | ||
| BidRejectionReason seatNonBid = BidRejectionReason.REQUEST_BLOCKED_OPTIMIZED; | ||
|
|
||
| @JsonProperty("ifSyncedId") |
There was a problem hiding this comment.
we shouldn't change java code
| import static org.prebid.server.functional.model.config.Stage.PROCESSED_AUCTION_REQUEST | ||
| import static org.prebid.server.functional.util.PBSUtils.randomString | ||
|
|
||
| class RuleSets { |
| List<Object> datacenters | ||
| List<Object> sources | ||
| List<Object> sids | ||
| Object pct |
src/test/groovy/org/prebid/server/functional/model/config/RuleEngineFunctionArgs.groovy
Show resolved
Hide resolved
| assert !getAnalyticResults(bidResponse) | ||
|
|
||
| where: | ||
| requestedUfpUser << [new User(data: null), new User(data: [null]), //todo empty |
| getDefaultBidRequestWithMultiplyBidders().tap { | ||
| updateBidRequestWithTraceVerboseAndReturnAllBidStatus(it) | ||
| site.content = new Content(data: [null]) //todo PLEASE TAKE A LOOK | ||
| site.ext = new SiteExt(data: null) | ||
| }, | ||
| getDefaultBidRequestWithMultiplyBidders().tap { | ||
| updateBidRequestWithTraceVerboseAndReturnAllBidStatus(it) | ||
| site.ext = new SiteExt(data: null) | ||
| }, | ||
| getDefaultBidRequestWithMultiplyBidders(APP).tap { | ||
| updateBidRequestWithTraceVerboseAndReturnAllBidStatus(it) | ||
| app.content = new Content(data: [null]) //todo PLEASE TAKE A LOOK | ||
| app.ext = new AppExt(data: null) | ||
| }, |
| it.ruleSets[0].modelGroups[0].schema = [new RuleEngineModelSchema(function: GPP_SID_AVAILABLE)] | ||
| } | ||
|
|
||
| and: "Account with disabled or without rules engine" |
There was a problem hiding this comment.
and: "Account with rules engine". Same for others
| where: | ||
| distributionChannel | bidRequestClosure | ||
| SITE | { String domain -> | ||
| BidRequest.getDefaultBidRequest(distributionChannel).tap { |
There was a problem hiding this comment.
mb getDefaultBidRequestWithMultiplyBidders?
| assert !getAnalyticResults(bidResponse) | ||
|
|
||
| and: "Logs should contain error" | ||
| def logs = pbsServiceWithRulesEngineModule.getLogsByTime(startTime) |
| PaaFormat paaFormat | ||
| @JsonProperty("alternatebiddercodes") | ||
| AlternateBidderCodes alternateBidderCodes | ||
| Map kvps |
There was a problem hiding this comment.
@JsonProperty("kvps")
Map<String, String> keyValuePairs
| "adapters.${AMX}.endpoint": "$networkServiceContainer.rootUri/auction".toString()] | ||
| protected static final Map<String, String> OPENX_ALIAS_CONFIG = ["adapters.${OPENX}.aliases.${OPENX_ALIAS}.enabled" : "true", | ||
| "adapters.${OPENX}.aliases.${OPENX_ALIAS}.endpoint": "$networkServiceContainer.rootUri/auction".toString()] | ||
| protected static final String PB_RULE_ENGINE_MODULE_NAME_CODE = "pb-rule-engine" |
There was a problem hiding this comment.
replase with ModuleName.PB_RULE_ENGINE
| protected final static String CALL_METRIC = "modules.module.%s.stage.%s.hook.%s.call" | ||
| protected final static String FAILER_METRIC = "modules.module.%s.stage.%s.hook.%s.failure" | ||
| protected final static String NOOP_METRIC = "modules.module.%s.stage.%s.hook.%s.success.noop" | ||
| protected final static String UPDATE_METRIC = "modules.module.%s.stage.%s.hook.%s.success.update" |
There was a problem hiding this comment.
it's always PB_RULE_ENGINE module for this spec
| Stage stage | ||
| String name | ||
| String version | ||
| List<RulesEngineModelGroups> modelGroups |
| updateBidRequestWithGeoCountry(it) | ||
| updateBidRequestWithTraceVerboseAndReturnAllBidStatus(it) |
There was a problem hiding this comment.
you have 125 uses of getDefaultBidRequestWithMultiplyBidders, 36 uses of updateBidRequestWithGeoCountry & 124 - updateBidRequestWithTraceVerboseAndReturnAllBidStatus. So why not combine them?
| AD_UNIT_CODE_IN("adUnitCodeIn"), | ||
| DEVICE_TYPE("deviceType"), | ||
| DEVICE_TYPE_IN("deviceTypeIn"), | ||
| BID_PRICE("bidPrice") |
| 1 | TRUE as String | ||
| 0 | FALSE as String |
There was a problem hiding this comment.
1 | 'TRUE'
0 | 'FALSE'
| assert seatNonBid.nonBid[0].statusCode == REQUEST_BIDDER_REMOVED_BY_RULE_ENGINE_MODULE | ||
|
|
||
| where: | ||
| percent << [100, PBSUtils.getRandomNumber(100)] |
There was a problem hiding this comment.
Is 100% really that special? and why [100; 0x7fffffff)?
| "Field 'domains' is required and has to be an array of strings") | ||
|
|
||
| where: | ||
| distributionChannel | bidRequestClosure |
There was a problem hiding this comment.
can be simplified:
bidRequestClosure << [ getDefaultBidRequestWithMultiplyBidders(SITE).tap {
it.site.publisher = new Publisher(id: PBSUtils.randomString, domain: PBSUtils.randomString)
updateBidRequestWithTraceVerboseAndReturnAllBidStatus(it)
} ]
same for others
There was a problem hiding this comment.
also, why not make a methods that sets Publisher or domain based on distributionChannel? Like setAccountId. Then you will only need distributionChannel in where part
| def "PBS should exclude bidder when adUnitCodeIn match with condition"() { | ||
| given: "Default bid request with multiply bidders" | ||
| def randomString = PBSUtils.randomString | ||
| def bidRequest = bidRequestClosure(randomString) |
There was a problem hiding this comment.
m.b. generate bidRequest before Closure and only then apply change?
There was a problem hiding this comment.
what if?
def randomString = PBSUtils.randomString
def bidRequest = getDefaultBidRequestWithMultiplyBidders().tap {
imp[0].tagId = PBSUtils.randomString
imp[0].ext.gpid = PBSUtils.randomString
imp[0].ext.data = new ImpExtContextData(pbAdSlot: PBSUtils.randomString)
imp[0].ext.prebid.storedRequest = new PrebidStoredRequest(id: PBSUtils.randomString)
}
def randomString = getImpUnitCode(bidRequest.imp[0], code)
getImpUnitCode(Imp imp, ImpUnitCode) {
switch ()
}
enum ImpUnitCode {
TAG_ID
GRID
DATA
STORED_REQUEST
}
# Conflicts: # src/test/groovy/org/prebid/server/functional/model/request/auction/Prebid.groovy
| protected static Map<String, String> getRequestCorrectionSettings(Endpoint endpoint = OPENRTB2_AUCTION, Stage stage = PROCESSED_AUCTION_REQUEST) { | ||
| ["hooks.${PB_REQUEST_CORRECTION.code}.enabled": "true", | ||
| "hooks.host-execution-plan" : encode(ExecutionPlan.getSingleEndpointExecutionPlan(endpoint, PB_REQUEST_CORRECTION, [stage]))] |
| private static void updateImpWithOpenXAndAmxAndOpenXAliasBidder(Bidder bidder) { | ||
| bidder.tap { | ||
| openx = Openx.defaultOpenx | ||
| amx = new Amx() | ||
| openxAlias = Openx.defaultOpenx | ||
| } | ||
| } | ||
|
|
||
| private static void getAliasAndAmxAndOpenXAndWithoutGenericBidder(Bidder bidder) { | ||
| bidder.tap { | ||
| alias = new Generic() | ||
| generic = null | ||
| amx = new Amx() | ||
| openx = Openx.defaultOpenx | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
m.b.? And make it protected on RuleEngineBaseSpec for getDefaultBidRequestWithMultiplyBidders
private static void updateBidderImp(Imp imp, List<BidderName> bidders = MULTI_BID_ADAPTERS) {
imp.ext.prebid.bidder.tap {
openx = bidders.contains(OPENX) ? Openx.defaultOpenx : null
openxAlias = bidders.contains(OPENX_ALIAS) ? Openx.defaultOpenx : null
amx = bidders.contains(AMX) ? new Amx() : null
generic = bidders.contains(GENERIC) ? new Generic() : null
alias = bidders.contains(ALIAS) ? new Generic() : null
}
}
This also allow u remove magic numbers
bidResponse.ext.seatnonbid.size() == 3/2
| AD_UNIT_CODE("adUnitCode"), | ||
| AD_UNIT_CODE_IN("adUnitCodeIn"), | ||
| DEVICE_TYPE("deviceType"), | ||
| DEVICE_TYPE_IN("deviceTypeIn"), |
There was a problem hiding this comment.
what about bidPrice?
Allows response rules to be set up to trigger on bid response prices. e.g. bidPrice["gt", 5.00, "EUR"] would return true if the bid response is greater than 5 EUR.
Left as an example of possible future functions.
There was a problem hiding this comment.
BidPrice will be later, currently it's in low priority
| Stage stage | ||
| String name | ||
| String version | ||
| List<RulesEngineModelGroup> modelGroups |
There was a problem hiding this comment.
timestamp?
ruleSets[].timestamp - an optional string indicating when this ruleset was last updated. Again - for troubleshooting. Format is ISO 8601 format.
|
|
||
| static RulesEngineModelGroup createRulesModuleGroup() { | ||
| new RulesEngineModelGroup().tap { | ||
| it.weight = PBSUtils.getRandomNumber(0, 100) |
There was a problem hiding this comment.
PBSUtils.getRandomNumber(1, 100)
Could be flaky:
Weight must be greater than zero
| assert seatNonBid.nonBid[0].statusCode == REQUEST_BIDDER_REMOVED_BY_RULE_ENGINE_MODULE | ||
|
|
||
| where: | ||
| distributionChannel << [SITE, APP, DOOH] |
There was a problem hiding this comment.
same for other
DistributionChannel.values()
| assert seatNonBid.nonBid[0].statusCode == REQUEST_BIDDER_REMOVED_BY_RULE_ENGINE_MODULE | ||
|
|
||
| where: | ||
| bidRequestClosure << [ |
There was a problem hiding this comment.
BidRequest createBidRequest(DistributionChannel type, String domain, boolean usePublisher = true) {
def request = getDefaultBidRequestWithMultiplyBidders(type)
switch (type) {
case SITE:
if (usePublisher) request.site.publisher.domain = domain
else request.site.domain = domain
break
case APP:
if (usePublisher) request.app.publisher.domain = domain
else request.app.domain = domain
break
case DOOH:
if (usePublisher) request.dooh.publisher.domain = domain
else request.dooh.domain = domain
break
}
request
}
type | usePublisher
SITE | true
SITE | false
APP | true
APP | false
| assert !getAnalyticResults(bidResponse) | ||
| } | ||
|
|
||
| def "PBS should exclude bidder when adUnitCode match with condition"() { |
There was a problem hiding this comment.
add priority test:
Return the first of: imp.ext.gpid, imp.tagid, imp.ext.data.pbadslot, imp.ext.prebid.storedrequest.id
In validation mode, reject args
There was a problem hiding this comment.
1.Ok
2.We have test for this condition : PBS should reject processing rule engine when #function schema function contain args
| def "PBS should exclude bidder when adUnitCodeIn match with condition"() { | ||
| given: "Default bid request with multiply bidders" | ||
| def randomString = PBSUtils.randomString | ||
| def bidRequest = bidRequestClosure(randomString) |
There was a problem hiding this comment.
what if?
def randomString = PBSUtils.randomString
def bidRequest = getDefaultBidRequestWithMultiplyBidders().tap {
imp[0].tagId = PBSUtils.randomString
imp[0].ext.gpid = PBSUtils.randomString
imp[0].ext.data = new ImpExtContextData(pbAdSlot: PBSUtils.randomString)
imp[0].ext.prebid.storedRequest = new PrebidStoredRequest(id: PBSUtils.randomString)
}
def randomString = getImpUnitCode(bidRequest.imp[0], code)
getImpUnitCode(Imp imp, ImpUnitCode) {
switch ()
}
enum ImpUnitCode {
TAG_ID
GRID
DATA
STORED_REQUEST
}
| assert seatNonBid.nonBid[0].statusCode == REQUEST_BIDDER_REMOVED_BY_RULE_ENGINE_MODULE | ||
| } | ||
|
|
||
| def "PBS shouldn't exclude bidder when percent not match with condition"() { |
There was a problem hiding this comment.
when percent less than zero
| AD_UNIT_CODE("adUnitCode",null), | ||
| AD_UNIT_CODE_IN("adUnitCodeIn","codes"), | ||
| DEVICE_TYPE("deviceType",null), | ||
| DEVICE_TYPE_IN("deviceTypeIn","types"), |
|
|
||
| import org.prebid.server.functional.model.request.auction.Imp | ||
|
|
||
| import static org.prebid.server.functional.model.ModuleName.* |
| impResult.values.analyticsValue == groups.rules.first.results.first.args.analyticsValue | ||
| impResult.values.resultFunction == groups.rules.first.results.first.function.value | ||
| impResult.values.conditionFired == groups.rules.first.conditions.first | ||
| impResult.values.biddersRemoved.sort() == [OPENX, AMX, GENERIC].sort() |
There was a problem hiding this comment.
can be def value
[OPENX, AMX, GENERIC].sort()
| impResult.values.analyticsValue == groups.rules.first.results.first.args.analyticsValue | ||
| impResult.values.resultFunction == groups.rules.first.results.first.function.value | ||
| impResult.values.conditionFired == groups.rules.first.conditions.first | ||
| impResult.values.biddersRemoved.sort() == [OPENX, GENERIC, AMX].sort() |
| } | ||
|
|
||
| protected static String getImpAdUnitCode(Imp imp) { | ||
| if (imp.ext.gpid) { |
| given: "Bid request with multiply imps bidders" | ||
| def bidRequest = getDefaultBidRequestWithMultiplyBidders().tap { | ||
| it.imp.add(Imp.defaultImpression) | ||
| it.imp[1].ext.prebid.bidder.tap { |
There was a problem hiding this comment.
updateBidderImp?
same for others
|
|
||
|
|
| assert !getAnalyticResults(bidResponse) | ||
|
|
||
| where: | ||
| gdpr | condition |
There was a problem hiding this comment.
not consistent with previous. M.b. in lower and upper cases?
| and: "Analytics result shouldn't contain info about module exclude" | ||
| assert !getAnalyticResults(bidResponse) | ||
| } | ||
|
|
| where: | ||
| gpid | tagId | pbAdSlot | prebidStoredRequest | ||
| PBSUtils.getRandomString() | null | null | null | ||
| null | PBSUtils.getRandomString() | null | null | ||
| null | null | PBSUtils.getRandomString() | null | ||
| null | null | null | new PrebidStoredRequest(id: PBSUtils.getRandomString()) | ||
| PBSUtils.getRandomString() | PBSUtils.getRandomString() | PBSUtils.getRandomString() | new PrebidStoredRequest(id: PBSUtils.getRandomString()) | ||
| null | null | null | new PrebidStoredRequest(id: PBSUtils.getRandomString()) | ||
| null | null | PBSUtils.getRandomString() | null | ||
| null | PBSUtils.getRandomString() | null | null | ||
| PBSUtils.getRandomString() | null | null | null |
There was a problem hiding this comment.
where:
gpid | tagId | pbAdSlot | prebidStoredRequest
PBSUtils.getRandomString() | null | null | null
null | PBSUtils.getRandomString() | null | null
null | null | PBSUtils.getRandomString() | null
null | null | null | new PrebidStoredRequest(id: PBSUtils.getRandomString())
PBSUtils.getRandomString() | PBSUtils.getRandomString() | PBSUtils.getRandomString() | new PrebidStoredRequest(id: PBSUtils.getRandomString())
null | PBSUtils.getRandomString() | PBSUtils.getRandomString() | new PrebidStoredRequest(id: PBSUtils.getRandomString())
null | null | PBSUtils.getRandomString() | new PrebidStoredRequest(id: PBSUtils.getRandomString())
null | null | null | new PrebidStoredRequest(id: PBSUtils.getRandomString())
🔧 Type of changes
✨ What's the context?
What's the context for the changes?
🧠 Rationale behind the change
Why did you choose to make these changes? Were there any trade-offs you had to consider?
🔎 New Bid Adapter Checklist
🧪 Test plan
How do you know the changes are safe to ship to production?
🏎 Quality check