KEMBAR78
Core: Adding bidLimit to adUnit by mkomorski · Pull Request #13930 · prebid/Prebid.js · GitHub
Skip to content

Conversation

@mkomorski
Copy link
Collaborator

@mkomorski mkomorski commented Sep 24, 2025

Type of change

  • Bugfix

  • Feature

  • New bidder adapter

  • Updated bidder adapter

  • Code style update (formatting, local variables)

  • Refactoring (no functional changes, no api changes)

  • Build related changes

  • CI related changes

  • Does this change affect user-facing APIs or examples documented on http://prebid.org?

  • Other

Description of change

Other information

#9842

@coveralls
Copy link
Collaborator

coveralls commented Sep 24, 2025

Pull Request Test Coverage Report for Build 18194719118

Details

  • 21 of 21 (100.0%) changed or added relevant lines in 2 files are covered.
  • 57 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+33.1%) to 96.244%

Files with Coverage Reduction New Missed Lines %
src/adloader.js 12 78.57%
src/utils.js 45 88.84%
Totals Coverage Status
Change from base Build #3: 33.1%
Covered Lines: 198531
Relevant Lines: 206278

💛 - Coveralls

@mkomorski mkomorski marked this pull request as ready for review September 25, 2025 10:36
@mkomorski mkomorski changed the title Core: Core: Adding bidLimit to adUnit Sep 25, 2025
@mkomorski mkomorski requested a review from dgirardi September 25, 2025 14:25
src/targeting.ts Outdated
// if adUnitBidLimit is set, pass top N number bids
if (adUnitBidLimit) {
const adUnit = auctionManager.getAdUnits().find(au => au.code === bucketKey);
const bidLimit = adUnit?.bidLimit || adUnitBidLimit;
Copy link
Collaborator

Choose a reason for hiding this comment

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

there's some logic one level up that says roughly "use the bid limit only if sendAllBids is enabled". With this change that's short circuited when the ad unit bid limit is set, which I don't think we want.

Since there you also know all ad units involved it makes sense IMO to calculate the bid limit per adunit there, which lets you loop through them one time instead of once per ad unit. For example the adUnitBidLimit parameter passed into here could be made into a map.

@mkomorski mkomorski requested a review from dgirardi October 2, 2025 14:41
src/targeting.ts Outdated
if (!config.getConfig('enableSendAllBids')) return 0;
const bidLimitConfigValue = config.getConfig('sendBidsControl.bidLimit');
return adUnitCodes.reduce((acc, code) => {
const adUnit = auctionManager.getAdUnits().find(au => au.code === code);
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is still O(N^2), which is unnecessary. one single loop over getAdUnits() is enough to find those you're interested in.

@mkomorski mkomorski force-pushed the mkomorski/bids-limit-on-adunit branch from 1053eeb to 476bcbd Compare October 2, 2025 17:22
@patmmccann patmmccann merged commit 038af15 into master Oct 9, 2025
18 of 20 checks passed
@patmmccann patmmccann deleted the mkomorski/bids-limit-on-adunit branch October 9, 2025 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants