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

Conversant Analytics Adapter: initial release #8981

Merged
merged 4 commits into from
Oct 6, 2022

Conversation

AaronColbyPrice
Copy link
Contributor

Type of change

  • Bugfix

  • [x ] Feature

  • New 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

Initial release of Conversant Prebid Analytics Adapter
Documentation: prebid/prebid.github.io#4011

Other information

AaronColbyPrice and others added 2 commits September 9, 2022 13:55
Sync'ing from upstream
…alytics adapter

- commits squashed to 1 checkin
- testing
- documentation
- throttling
@AaronColbyPrice
Copy link
Contributor Author

@robertrmartinez I'm curious if you have an ETA on the review for this. My team is trying to figure out when we can announce some marketing material and I'm not sure what I can tell them.

@robertrmartinez
Copy link
Collaborator

@AaronColbyPrice

Will give it a thorough review tonight!

@AaronColbyPrice
Copy link
Contributor Author

@robertrmartinez great thanks so much. Didn't mean to rush you, I know you are busy. Just trying to figure out what to tell my product manager :)

Copy link
Collaborator

@robertrmartinez robertrmartinez left a comment

Choose a reason for hiding this comment

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

Looks fine, just a couple minor things + a question.

sampleRate: 100
}
})
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

think we can remove this from the example page.

import CONSTANTS from '../src/constants.json';
import {getGlobal} from '../src/prebidGlobal.js';
import adapterManager from '../src/adapterManager.js';
import * as utils from '../src/utils.js';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Better to just import the needed utils instead of all here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks to just be a couple log utils, deepAccess, and isInteger. So very small amount of the util lib! So just import those specifically.

const cnvrAdUnit = cnvrHelper.createAdUnit();
// Initialize bids with bidderCode
adUnit.bids.forEach(bid => {
cnvrAdUnit.bids[bid.bidder] = cnvrHelper.initializeBidDefaults();
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is possible that a publisher has more than one bid for a given bidder within an adUnit.

{
   code: 'some-ad-unit-code',
   mediaTypes: {
      banner: {
         sizes: [300,250, 728,90]
      }
   },
   bids: [
      { bidder: 'ix', params: { siteId: 1234, size: [300,250] } },
      { bidder: 'ix', params: { siteId: 1234, size: [728,90] } }
   ]
}

I think in this case, this code will always just set it to whatever the last ix bid is in here.

Same for lower in the nobids and bidsrecieved parts, it will add an event to eventCodes array, but overwrite the bidPayload with the last entry in it.

I guess if you do not care and do not want to track such scenario, or just want the last bid in the array to send it's data it is fine.

But maybe something to look into. I know with my experience I see many pubs having several bids in the bids array for the same bidders.

modules/conversantAnalyticsAdapter.js Show resolved Hide resolved
@AaronColbyPrice
Copy link
Contributor Author

@robertrmartinez fixed all issues noted

Copy link
Collaborator

@robertrmartinez robertrmartinez left a comment

Choose a reason for hiding this comment

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

Nice! Looks good

@robertrmartinez robertrmartinez merged commit d1ac4f2 into prebid:master Oct 6, 2022
@AaronColbyPrice AaronColbyPrice deleted the cnvr_analytics-adapter branch November 7, 2022 23:56
JacobKlein26 pushed a commit to nextmillenniummedia/Prebid.js that referenced this pull request Feb 9, 2023
* cnvr_analytics-adapter - First implementation of Conversant Prebid Analytics adapter
- commits squashed to 1 checkin
- testing
- documentation
- throttling

* cnvr_analytics-adapter - cicircle fix

* cnvr_analytics-adapter - code review tweaks, handle multiple bids from 1 bidder for 1 adslot
jorgeluisrocha pushed a commit to jwplayer/Prebid.js that referenced this pull request May 23, 2023
* cnvr_analytics-adapter - First implementation of Conversant Prebid Analytics adapter
- commits squashed to 1 checkin
- testing
- documentation
- throttling

* cnvr_analytics-adapter - cicircle fix

* cnvr_analytics-adapter - code review tweaks, handle multiple bids from 1 bidder for 1 adslot
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants