SizeMappingV2 bug with multiple ad units having the same code

[benjaminclot]

Type of issue

When using SizeMapping, a collateral (and very handy) feature is that having multiple ad units having the same code works, allowing us to write much more readable configs (and splitting mediatypes, etc.).
SizeMappingV2 seems to be introducing a bug where it tries to merge all the configs (I guess?).

PS : I know I could technically merge configs but readability (and backward compatibility) is an issue here.

Description

Responsive rules aren't respected.

Example

http://jsfiddle.net/6f2r5sbj/

Steps to reproduce

This is a sample config

[
  {
    code: 'ad-code-1',
    mediaTypes: {
      banner: {
        sizeConfig: [
          { minViewPort: [0, 0], sizes: [] },
          { minViewPort: [1000, 0], sizes: [[1000, 300], [1000, 90], [970, 250], [970, 90], [728, 90]] },
        ],
      },
    },
    bids: [{
      bidder: 'bidderA',
      params: {
        placementId: 123
      }
    }]
  }, {
    code: 'ad-code-1',
    mediaTypes: {
      banner: {
        sizeConfig: [
          { minViewPort: [0, 0], sizes: [] },
          { minViewPort: [320, 0], sizes: [[320, 100], [320, 50], [300, 50]] },
          { minViewPort: [600, 0], sizes: [] },
        ],
      },
    },
    bids: [{
      bidder: 'bidderA',
      params: {
        placementId: 456
      }
    }]
  },
]

Expected results

On a 1000px+ wide resolution, only 123 should be sent to the bidder.

Actual results

On a 1000px+ wide resolution, all configs (123 and 456) are sent with the sizes from the good viewport.

Other information

Relates to PR #4690

[Fawke]

Hey @benjaminclot,

Thanks for trying out the feature. I think we have got your use case covered here, what do you think of the following setup?

var adUnits = [
      {
        code: 'ad-code-1',
        mediaTypes: {
          banner: {
            sizeConfig: [
              { minViewPort: [0, 0], sizes: [] },
              { minViewPort: [320, 0], sizes: [] },
              { minViewPort: [600, 0], sizes: [[320, 100], [320, 50], [300, 50]] },
              { minViewPort: [1000, 0], sizes: [[1000, 300], [1000, 90], [970, 250], [970, 90], [728, 90]] },
            ],
          },
        },
        bids: [{
          bidder: 'appnexus',
          params: {
            placementId: '123'
          }
        }, {
          bidder: 'appnexus',
          sizeConfig: [
            { minViewPort: [0, 0], relevantMediaTypes: ['none'] },
            { minViewPort: [320, 0], relevantMediaTypes: ['none'] },
            { minViewPort: [600, 0], relevantMediaTypes: ['banner'] },
            { minViewPort: [1000, 0], relevantMediaTypes: ['nonoe'] }
          ],
          params: {
            placementId: 456
          }
        }]
      }
    ];

It does achieve what you are trying to do, but, you pointed out readability and backward compatibility may be an issue.

One of the major motivation for building sizeMappingV2 was to get rid of the multiple adUnit with the same code setup, and it achieves that using bidder level sizeConfig which I showed in the example above.

I'll have to check with other Prebid.js core members to see what they think about this issue, and if they wanna add that same functionality (declaring multiple ad units with same code) to sizeMappingV2.

Anyways, thanks for bringing this up.

[bretg]

I wasn't aware that the AdvancedSizeMapping module didn't allow the use of AdUnits with the same code. However, I do realize that's a feature that some in the Prebid community make extensive use of.

One of the major motivation for building sizeMappingV2 was to get rid of the multiple adUnit with the same code setup

With due respect to @Fawke, I disagree with this statement. The motivation for this feature was to support complicated scenarios where different mediatypes and adunits break into different treatments along different screen size boundaries. AdUnits with the same code was not an explicit part of the goal.

That said, we are where we are. Perhaps we can leave things this way, or perhaps we need to alter the AdvancedSizeMapping module.

Because I'm not deeply familiar with the use cases here, I would like community members like @benjaminclot and @GLStephen to weigh in.

In the meantime, would also like the developers to comment on what it would mean to "Add identical ad units as a feature in sizeMappingV2"

[benjaminclot]

I'm in favor of keeping compatibility so as not to get different behaviors depending on the sizemapping module.

[GLStephen]

The re-declaration of adunits was always a good safety valve for situations where the available config wasn't quite what you wanted or the way to achieve it was difficult to express. I would prefer to have kept the ability.

[Fawke]

@bretg,

With due respect to @Fawke, I disagree with this statement. The motivation for this feature was to support complicated scenarios where different mediatypes and adunits break into different treatments along different screen size boundaries. AdUnits with the same code was not an explicit part of the goal.

I agree with you. My intention was not to get rid of this feature completely. I missed to test this setup since it was an edge case. Now, with my PR linked above, size mapping v2 does support declaration of identical ad units.

My definition of Identical Ad Units - Ad units with the same code but which has different value for mediaTypes object.

@benjaminclot @GLStephen Can you check with your existing setups of identical ad units whether my pr resolves your issue or not. I'm currently working to make the log messages a little more intuitive, so technically, it's still a WIP. But the major issue has been resolved.

@bretg Do we need to document this pattern of Identical Ad Unit setup in the official SizeMappingV2 docs on prebid.org?

[benjaminclot]

@Fawke readibility-wise, it's not a solution for me. Also, I use this "feature" to send different sizes to different bidders having the same responsive rules. Like for example bidderA can handle being sent an array of multiple sizes but bidderB has to be sent only one specific size.

[Fawke]

@benjaminclot Is it possible for you to share an example Ad Unit setup, I can better understand your use case better that way.

[benjaminclot]

Here you go:

{
  code: 'ad-unit-1',
  mediaTypes: {
    banner: {
      sizeConfig: [
        { minViewPort: [0, 0], sizes: [] },
        { minViewPort: [1600, 0], sizes: [[1800, 1000]] }
      ]
    }
  },
  bids: [
    {
      bidder: 'bidderA',
      params: {
        placementId: 123
      }
    }
  ]
}, {
  code: 'ad-unit-1',
  mediaTypes: {
    banner: {
      sizeConfig: [
        { minViewPort: [0, 0], sizes: [] },
        { minViewPort: [1600, 0], sizes: [[1, 1]] }
      ]
    }
  },
  bids: [
    {
      bidder: 'bidderB',
      params: {
        placementId: 456
      }
    }
  ]
}

[Fawke]

@benjaminclot,

Correct me if I'm wrong, in the ad unit setup you pasted above, for viewport size 1600px+, requests should be sent for both the bidders, bidderA and bidderB.

Thant's what's happening in the PR I linked above.

Both the ad units have the same code, but they've different mediaTypes (for the size bucket [1600, 0], the first ad unit has a size [1000, 1000], while the second ad unit has a size [1, 1]). So, they both will go through the sizeConfig filtration process and since we've assumed the viewport size is greater than 1600px, both will participate in the auction. BidderA will be sent size [1000, 1000], while bidderB will be sent size [1, 1]

[benjaminclot]

@Fawke if your PR correctly sends each bidRequest then we're good :)

[bretg]

Opened a docs PR finally bringing 'twin' adunits out of the shadows. prebid/prebid.github.io#1909