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

SizeConfig shouldn't merge sizes of different labels together #3226

Closed
eliseumds opened this issue Oct 25, 2018 · 21 comments
Closed

SizeConfig shouldn't merge sizes of different labels together #3226

eliseumds opened this issue Oct 25, 2018 · 21 comments
Assignees
Labels
pinned won't be closed by stalebot

Comments

@eliseumds
Copy link

eliseumds commented Oct 25, 2018

Type of issue

This is a bug.

Description

When resolving the bid/adUnit sizes, Prebid doesn't account for size configs with the same media query but different labels.

Steps to reproduce

With a size config:

// Notice that the media queries are the same
[
  {
    mediaQuery: "(min-width: 768px) and (max-width: 1199px)",
    sizesSupported: [[970, 90], [728, 90], [300, 250]],
    labels: ["myLabelA"]
  },
  {
    mediaQuery: "(min-width: 768px) and (max-width: 1199px)",
    sizesSupported: [[728, 90], [300, 250]],
    labels: ["myLabelB"]
  }
]

and a bid with { labelAny: ['myLabelA'] }, Prebid sends all the five sizes to DFP. I expected it to send only myLabelA sizes to DFP: [[970, 90], [728, 90], [300, 250]].

My use case

I have a bunch of different sections on the website (header, footer, sidebar, fixed ads, in-content, etc) like so:

{
  header: {
    xs: {
      mediaQuery: '(min-width: 576px) and (max-width: 767px)',
      sizes: [[468, 60], [600, 300]]
    },
    sm: {
      mediaQuery: '(max-width: 575px)',
      sizes: [[468, 60], [300, 250]]
    }
  ],
  sidebar: [...]
}

And then in the bids:

// MyAdUnitA
[
  {
    bidder: 'sovrn',
    params: { tagid: '123456' },
    labelAny: ['header-xs', 'header-sm'] },
  },
  // ...
]
@snapwich
Copy link
Collaborator

I think you have the intent of labels backwards. Labels on an adUnit don't decide which size configs are active, it's the inverse; size configs determine which labels are active. Also, you should keep in mind that the sizesSupported on sizeConfig don't add sizes or specify additional sizes to send to the bidder, they specify which sizes are valid as a filtering mechanism.

For your use case it doesn't sound like you require the use of labels at all as you're not trying to enable/disable adUnits for certain screen sizes. For your use case, the adUnits you have on the different sections of your website you should list all valid sizes within their mediaTypes.banner.sizes. Then within the sizeConfig you should list all the valid sizes for a certain screen size across all of your adUnits. Since it applies a filter, the net result will just be each adUnit bidding on the sizes they have listed in adUnit.mediaTypes.banner.sizes minus the ones not listed in sizesSupported for the matching mediaQuery. No sizes listed under sizesSupported but not within the adUnit.mediaTypes.banner.sizes will be added, which I think is the behavior you're intending.

Let me know if this makes sense or if I'm misunderstanding your issue.

@snapwich snapwich self-assigned this Oct 25, 2018
@bretg
Copy link
Collaborator

bretg commented Oct 29, 2018

Agree with @snapwich . A couple other points maybe worth clarifying:

  • mediaQuery in sizeConfig is the only condition -- sizesSupported and labels are pure outputs.
  • mediaQueries in sizeConfig must be designed so they do not overlap. There should not be a scenario where there are duplicate mediaQueries. The intended use is for a publisher to define screensizes into buckets along the lines of "phone is < X width", "X < tablet < Y", "Y < display".
  • labels are only useful in sizeConfig to suppress/allow certain bids. e.g. if a particular bidder only has mobile demand, so shouldn't be called for display. To suppress/allow whole adunits, use the supportedSizes filter.
  • labels can also be set programmatically, so if the way sizeConfig works doesn't support your model, you can set them in your javascript however you'd like and pass them in on requestBids().

Please see http://prebid.org/dev-docs/conditional-ad-units.html#what-if-some-bidders-should-be-skipped-for-some-devices

@eliseumds
Copy link
Author

I understand what the original intention was, but it's confusing because of the implicit fact that only mediaQuery is used. At the end, we don't have much flexibility for sizes. Besides, my use case seems quite common.

@bretg labels are useful to filter out sizes as well, not just for ignoring bids. And passing the sizes in requestBids means that I wouldn't be able to batch these requests.

It'd be great if Prebid warned the user saying that duplicate media queries were passed in sizeConfig. Or even safer: warn after they have been calculated because there might not be duplicates but overlapping ones.

@bretg
Copy link
Collaborator

bretg commented Nov 1, 2018

I agree that a warning would be appropriate. Happy to take PRs on that.

As for your use case, I suspect the main shortcoming is documentation. Please help me understand how your use case differs from http://prebid.org/dev-docs/conditional-ad-units.html#what-if-some-bidders-should-be-skipped-for-some-devices .

i.e. why did you feel duplicate mediaQueries were necessary? Our model is meant to be simple:

  1. define a non-overlapping set of mediaQueries that defines a set of screen sizes
  2. each screen size defines the valid set of ad sizesSupported
  3. each screen size can optionally define a label
  4. sizesSupported acts as a filter for AdUnit sizes
  5. labels can be used filter out bids

Where would you propose a change to this?

@eliseumds
Copy link
Author

eliseumds commented Nov 2, 2018

I can't have the same set of sizes per media query because they vary depending on where my ads are placed on the page.

I was overlapping media queries because I believed the set of labels I passed together with each one of them to the sizeConfig would both act as filters, together. This is useful when you have ads in different sections of your page, and different sizes depending on the window size, for example:

Ad ScreenSize.large ScreenSize.medium ScreenSize.small
Top 970x90 728x90 728x90 468x60 486x60 320x100 300x250 300x100 300x50
Sidebar 300x600 300x250 (nothing) (nothing)
In-content 728x90 468x60 468x60 486x60 320x100 300x250 300x100 300x50

I'm sorry if I'm missing something here.

@GLStephen
Copy link
Collaborator

GLStephen commented Nov 2, 2018

We have encountered similar challenges in this area. The DFP approach is to map a full config to an ad unit. With merging of media queries you cannot create size variations for an ad unit that activated by screen size without duplicating significant chunks of config. As far as I can tell there is no way to solve the above scenario without duplicating ad unit config and dynamically activating it. At which point you may as well generate the bid config almost. If instead the active media queries even overlapping turned on labels and the set of labels filtered sizes on the ad unit then the expressiveness would be significantly greater and more flexible.

@GLStephen
Copy link
Collaborator

GLStephen commented Nov 2, 2018

@bretg @eliseumds

There is a caveat here, that maybe there is some other way ,maybe I don't understand something or maybe this is really supported but after a few long looks I don't think it's really meeting the more complex needs. My gut is that right now the size config is behaving in an unexpected way and has a smell, and likely a deficiency, even if it technically meets the original goal of the implementation.

After some more discussion internally here I would propose two changes:

  1. let them overlap and to essentially add labels to the active set of labels. Essentially, like a shortcut to adding them manually where the concept of "overlapping" is non-existent.
  2. allow labels set on the BIDs to filter the sizes

Then make the sent bidRequest size config for a given bid request the intersection of the BID label, the ADUNIT label and the MEDIA QUERY label that are applied and TOSS the sizes along the way. We see a fair number of multi-size adunits now and the size config is what determines what the upstream bidder uses. Given this the exact same BIDDER PARAMS might be sent in multiple cases.

This becomes a waterfall:

Size Config
Label: MOBILE max 350px wide: 300x250,728,90 <-superset config, page level availability
Label: MOBILE-TOP max 350px wide: 300x250 <- adunit level config for THIS size
Label: TABLET-TOP min 351px max 1024px wide: 728x90 <- adunit level config for THIS size

AdUnits
Adunit: on MOBILE <- adunit is ACTIVE at the MOBILE sizes and always when mobile
Bid 1: blank = any <- bid is ACTIVE at all labels but only MOBILE since the adunit is, no change to sizes
Bid 2: MOBILE-TOP <- bid is ACTIVE at the MOBILE-TOP sizes and ONLY when MOBILE-TOP label exists.
Bid 3: TABLET-TOP <- bid is ACTIVE at the TABLET-TOP sizes and ONLY when TABLET-TOP label exists.

Resulting size for Bid 2 is 300x250 only. Bid 3 is only 728x90 and only on tablet. I don't know of a way to create this simplicity and clarity of config in the current system.

The problem we're encountering is if an adunit is multi-size then instances where it has variable sizes at different mediaQueries often mismatch. The current system appears to be a complex way to map a name to a mediaQuery and relies on the bidder params to adjust the sizes somehow, or duplicate configs. A couple of changes would make it so that a much smaller config footprint would work.

This make it closer to what I believe is a much clearer and more flexible approach that creates a size config at all sizes with a name. Which is then applied.

My preferred system would be:

  1. Size config that has a label. It includes all media queries in it and all valid sizes for the adunit
  2. That config label can be applied at any level Adunit, or Bid. The tree is traversed to determine which single label is active.
  3. No sizes at a mediaQuery indicates not active.
  4. Manually added labels are treated is a "no size" change label and just determine if a unit is active.
  5. Allow, labelAny and labelAll to allow for an adunit to require one label, but be modified by another.

This would allow for more semantic label:

Must be mobile which allows this broad set of sizes.
Then you could name size configs by the set of sizes or most likely placements. LARGE, FULL-LEADERBOARD, etc.
You could then set an adunit to be "labelAll: mobile" and a bid to be "labelAny: FULL-LEADERBOARD" then something like 970x90 is available for the top unit, but only on desktop sizes.

I'm riffing a bit, but I think there is some ways to improve the flexibility here significantly.

@bretg
Copy link
Collaborator

bretg commented Nov 9, 2018

Sorry, but can you guys please put together actual PBJS sizeConfigs and AdUnits showing the problem?

There's still a disconnect somewhere because I don't really feel your pain yet other than that this works differently than DFP. I follow that the current sizesSupported model means that you can't get away with just one PBJS AdUnit that's radically different across screen sizes. That seems ok -- it's arguably better to break out separate PBJS AdUnits for maintainability anyhow. I get that you have complicated ad models, but there are ways to implement it within the current system that seem straightforward.

Take the example @eliseumds posted. This is a nice table that translates easily to the current model.

  • 3 media queries.
    • width >= BIG: defines label=large and sizesSupported 970x90, 728x90, 300x600, 300x250, 468x60
    • BIG < width < SMALL: defines label=medium and sizesSupported 728x90, 468x60
    • width <= SMALL: defines label small and sizesSupported 486x60, 320x100, 300x250, 300x100, 300x50
  • 7 AdUnits
    • code="/top": mediaTypes.banner.sizes=970x90, 728x90, 468x60, 320x100, 300x250, 300x100, 300x50. LabelAll="large"
    • code="/top": mediaTypes.banner.sizes=728x90, 468x60. LabelAll="medium"
    • code="/top": mediaTypes.banner.sizes=468x60, 320x100, 300x250, 300x100, 300x50. LabelAll="small".
    • code="/sidebar". mediaTypes.banner.sizes=300x600, 300x250. LabelAll="large"
    • code="/incontent". mediaTypes.banner.sizes=728x90, 468x60. LabelAll="large"
    • code="/incontent". mediaTypes.banner.sizes=468x60. LabelAll="medium"
    • code="/incontent". mediaTypes.banner.sizes=486x60, 320x100,300x250, 300x100, 300x50. LabelAll="small"

This approach does not seem unduly burdensome given the complexity of the requirements.

If we're going to change the sizeConfig model, it would be towards the DFP model, where sizeConfig is per AdUnit rather than global. Fortunately, we have a committee headed by @mkendall07 who has the power to make that decision. But it would help that committee if there was a very specific example of actual PBJS code that is horribly ugly because of the current design. And IMO, the example @eliseumds posted is not so bad... just requires separate AdUnits.

benjaminclot added a commit to benjaminclot/prebid.github.io that referenced this issue Nov 13, 2018
@bretg
Copy link
Collaborator

bretg commented Nov 13, 2018

@GLStephen, @eliseumds - can you imagine your scenarios being supported if we supported a per-adunit SizeConfig?

@GLStephen
Copy link
Collaborator

GLStephen commented Nov 13, 2018

@bretg I think that it would. That approach would be the most flexible and align more closely with GPT responsive config. I'd have to think a bit more about turning adunits off and on which is handled well now in every instance we've tried. Labels could be relegated to flags and would align with the merged, non-overlapping approach. Then sizes at the bidder level would be bidder specific and would allow for the size variability that often differs from the off/on decision of an adunit being active.

@bretg
Copy link
Collaborator

bretg commented Nov 13, 2018

@mkendall07 - consider this 3 community votes for supporting a per-adunit override for sizeConfig. We should spend some time spec'ing the interaction.

@bretg bretg assigned mkendall07 and unassigned snapwich Nov 13, 2018
@eliseumds
Copy link
Author

eliseumds commented Nov 13, 2018

@bretg I believe that per-bid size configs would make it easier to understand and more powerful.

bretg pushed a commit to prebid/prebid.github.io that referenced this issue Nov 14, 2018
* Warning for labels not being filters of sizeConfig

Trying to shed a little light on prebid/Prebid.js#3226

* Update publisher-api-reference.md
@benjaminclot
Copy link
Contributor

benjaminclot commented Nov 14, 2018

@bretg I concur @GLStephen and @eliseumds, although configs will be more painful to write, they will indeed be more powerful.

@stale
Copy link

stale bot commented Nov 28, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Nov 28, 2018
@bretg bretg added the pinned won't be closed by stalebot label Dec 3, 2018
@stale stale bot removed the stale label Dec 3, 2018
@benjaminclot
Copy link
Contributor

benjaminclot commented May 28, 2019

@bretg @mkendall07 @snapwich

I've recently come accross a new "pull your hair out" use case and what may be a solution.

MediaQuery Banner Size
(min-width: 1200px) 728x90
(min-width: 768px) and (max-width: 1199.98px) 468x60
(min-width: 320px) and (max-width: 767.98px) 300x250
MediaQuery MPU Size
(min-width: 992px) 300x250

Both the banner and the MPU are on the same page and I don't want bidRequests to include the 300x250 size for the banner when the screen is 1000px wide.
I'll let you ponder over that one for a bit... (hint: sizesSupported isn't your friend)

🕑

So... here's what's working.

// I split the MediaQueries by format and make sure they don't overlap for the same format
sizeConfig: [
  {
    mediaQuery: '(min-width: 1200px)',
    sizesSupported: [[728, 90]],
    labels: ['w1200'],
  }, {
    mediaQuery: '(min-width: 768px) and (max-width: 1199.98px)',
    sizesSupported: [[468, 60]],
    labels: ['w768'],
  }, {
    mediaQuery: '(min-width: 320px) and (max-width: 767.98px)',
    sizesSupported: [[300, 250]],
    labels: ['w320'],
  }, {
    mediaQuery: '(min-width: 992px)',
    sizesSupported: [[300, 250]],
    labels: ['w992'],
  },
]

...

// The tricky part: declaring the same adunit twice with different rules
adUnits: [
  {
    code: 'banner',
    labelAny: ['w1200', 'w768'],
    mediaTypes: {
      banner: {
        sizes: [[728, 90], [468, 60]],
      },
    },
    bids: [
      {
        bidder: 'appnexus',
        params: {
          placementId: 111111,
        },
      },
    ],
  }, {
    code: 'banner',
    labelAny: ['w320'],
    mediaTypes: {
      banner: {
        sizes: [[300, 250]],
      },
    },
    bids: [
      {
        bidder: 'appnexus',
        params: {
          placementId: 222222,
        },
      },
    ],
  }, {
    code: 'mpu',
    labelAny: ['w992'],
    mediaTypes: {
      banner: {
        sizes: [[300, 250]],
      },
    },
    bids: [
      {
        bidder: 'appnexus',
        params: {
          placementId: 333333,
        },
      },
    ],
  },
]

So I guess my question is: is this working "by design"? Are adunits purposely not deduplicated by code? (which is fine by me)

@jdelhommeau
Copy link
Collaborator

to add to @benjaminclot's point, ability to declare same adUnit code multiple time and prebid then merging all response under same adUnit is pretty useful in numerous cases. It allows to circumvent some limitation from prebid, such as different mediaType declaration per bidder for same adUnit, different size per bidder for same adUnit, etc.
I made some test few weeks ago on this and this is working perfectly, although I don't think this is an intended behavior. Would be great for it to be an official feature of prebid to ensure sustainability over time.

@GLStephen
Copy link
Collaborator

@jdelhommeau @benjaminclot we are using this technique and it seems to work. We're working with 100s of sites and the variety of configurations is impressive to say the least. The option to declare size configs in a more GAM/GPT sort of way would ultimately be ideal, but this approach of double declaration allows us to implement some scenarios we are encountering more easily.

@bretg It would be great to have an official stance on support for this so a "bug fix" doesn't get merged one day that obliterates it accidentally.

@bretg
Copy link
Collaborator

bretg commented Jul 17, 2019

I didn't know that matching adunits with the same code would be merged, but I just confirmed it myself too. Cool. @mkendall07 -- let's make sure we leave this as a feature.

Anyhow, having "duplicate" ad units that support different bidders/params was the intention all along -- the intention was that labels would choose between which of the adunits was relevant. If the merging of adunits is useful, seems fine to me.

I'll take an item to update prebid.org with examples along these lines.

@bretg
Copy link
Collaborator

bretg commented Sep 25, 2019

@eliseumds , @benjaminclot , and @GLStephen - please take a look at #4129. There are two rather different approaches outlined there. Seeking your opinion about the direction.

@GLStephen
Copy link
Collaborator

@bretg I responded in the #4129

@bretg
Copy link
Collaborator

bretg commented Apr 12, 2020

Closing this issue- advanced sizemapping is now a thing -- http://prebid.org/dev-docs/modules/sizeMappingV2.html

@bretg bretg closed this as completed Apr 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pinned won't be closed by stalebot
Projects
None yet
Development

No branches or pull requests

7 participants