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

Remove ambiguity from sizeMappingV2 log messages #5103

Closed
Fawke opened this issue Apr 9, 2020 · 0 comments · Fixed by #5283
Closed

Remove ambiguity from sizeMappingV2 log messages #5103

Fawke opened this issue Apr 9, 2020 · 0 comments · Fixed by #5283
Assignees
Labels
intent to implement pinned won't be closed by stalebot

Comments

@Fawke
Copy link
Contributor

Fawke commented Apr 9, 2020

Type of issue

With the merge of this PR, #5062, sizeMappingV2 supports ad unit declarations with the same code. Here's an example of ad unit with same code:

[
  {
    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
      }
    }]
  },
]

Find more details about this pattern here: #5044, #3226 (comment)

Earlier, log messages in sizeMappingV2 assumed ad unit code to be a unique identifier. But, after introduction of 'identical' ad unit pattern, that's no longer the case. This leads to some degree of ambiguity in log messages. For example, for the above ad unit, and screen size, 1000px, the second ad unit should be disabled.

Here's the generated log messages:
Screenshot 2020-04-02 at 4 52 41 PM

Reading it, it's impossible to figure out which ad unit got disabled, the first one or the second one.

Potential Solution

Add identifier in paranthesis which would be unique. For example, for the above ad units, the first ad unit will read ad-code-1 (1) and second ad unit will read ad-code-1 (2)

If anyone has any better idea, please do share with me.

@Fawke Fawke self-assigned this Apr 9, 2020
@Fawke Fawke added the pinned won't be closed by stalebot label Apr 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
intent to implement pinned won't be closed by stalebot
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant