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

New STAQ analytics adapter #3772

Merged
merged 19 commits into from
Jun 5, 2019
Merged

Conversation

mquirion
Copy link
Contributor

Type of change

  • Bugfix
  • 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?
  • [x ] Other - Analytics Adapter

Description of change

New STAQ Analytics Adapter to gather Prebid.js event data and forward to STAQ's reporting platform.

For any changes that affect user-facing APIs or example code documented on http://prebid.org, please provide:

Other information

@sumit116
Copy link
Contributor

@mquirion Please remove package-lock.json changes. You can rebase the file with master branch.

Copy link
Contributor

@sumit116 sumit116 left a comment

Choose a reason for hiding this comment

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

Hi,
Thanks for submitting your adaptor. I tested your adaptor on hello-world page and found some changes required in your adaptor files, mentioned in my comments. Can you please make the necessary changes and let me know if you have any questions.

provider: 'staq',
options: {
host: , // HOST URL of site. Optional. Only required for whitelisting.
url: 'localhost:3000', // REQUIRED host URL for delivery of information to STAQ
Copy link
Contributor

Choose a reason for hiding this comment

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

Please provide staging/dev or any other url which would allow me to test the request and response.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sumit116 thanks for your review.

We don't currently have an outside facing url that we leave standing. Would it be possible to select a period of time when we could stand this up? Or should I just go ahead and work with our ops folks to stand up a long-living staging endpoint?

Copy link
Contributor

@sumit116 sumit116 May 3, 2019

Choose a reason for hiding this comment

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

@mquirion, how long can you keep the quick-server standing? If its a whole day, you can set it up on Monday and update me with the details. If its for a shorter duration, then we have to coordinate something definite since our timezones are different (I work in IST).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sumit116 actually, you can use the following...

options: { url: 'headerbidding.staging.staqdata.com', connId: '1234' }

And that will get you hitting our staging server which we can leave open for a while. Thank you.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for providing these credentials; I gave them a try and found they were working. I saw the events get sent to the staging endpoint.

modules/staqAnalyticsAdapter.js Outdated Show resolved Hide resolved

function buildRequestTemplate(connId) {
const url = utils.getTopWindowUrl();
const ref = utils.getTopWindowReferrer();
Copy link
Contributor

Choose a reason for hiding this comment

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

getTopWindowReferrer() is also depreciated and needs to be replaced.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated to use new ref code

modules/staqAnalyticsAdapter.js Outdated Show resolved Hide resolved
if (analyticsAdapter.context.queue) {
analyticsAdapter.context.queue.push(events);
if (eventType === CONSTANTS.EVENTS.BID_WON) {

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add some code here. I can suggest to return an object { auctionId: args.auctionId, size: args.width + 'x' + args.height };

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated to handle win and update response event to indicate it won

@sumit116
Copy link
Contributor

@mquirion Can you provide docs PR link. If not created, please create one.

@sumit116
Copy link
Contributor

sumit116 commented May 8, 2019

@mquirion, please give an update on #3772 (comment)

Copy link
Collaborator

@jsnellbaker jsnellbaker left a comment

Choose a reason for hiding this comment

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

Hi @mquirion

@sumit116 is ooo for a bit, so I wanted to jump in here. I took a look through the adapter and it largely seems good. There are a few areas where some commented code/tests could be removed (I noted the areas in the feedback below). Once that is addressed, we should be good to merge.

modules/staqAnalyticsAdapter.js Outdated Show resolved Hide resolved
provider: 'staq',
options: {
host: , // HOST URL of site. Optional. Only required for whitelisting.
url: 'localhost:3000', // REQUIRED host URL for delivery of information to STAQ
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for providing these credentials; I gave them a try and found they were working. I saw the events get sent to the staging endpoint.

test/spec/modules/staqAnalyticsAdapter_spec.js Outdated Show resolved Hide resolved
// let ev = JSON.parse(secondCallArgs);
// console.log('AUCTION WON EVENT SHAPE ' + JSON.stringify(ev));
// let firstEv = JSON.parse(ajaxStub.firstCall.args)
// console.log('FIRST CALL ARGS ' + JSON.stringify(firstEv));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you remove this commented code if it's no longer needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jsnellbaker thanks for your review and your help. I think I've got everything you asked for cleared up.

test/spec/modules/staqAnalyticsAdapter_spec.js Outdated Show resolved Hide resolved
test/spec/modules/staqAnalyticsAdapter_spec.js Outdated Show resolved Hide resolved
test/spec/modules/staqAnalyticsAdapter_spec.js Outdated Show resolved Hide resolved
test/spec/modules/staqAnalyticsAdapter_spec.js Outdated Show resolved Hide resolved
@mquirion
Copy link
Contributor Author

@mquirion Can you provide docs PR link. If not created, please create one.

@sumit116 @jsnellbaker I'm not entirely clear on what this is a request for.

@jaiminpanchal27
Copy link
Collaborator

@mquirion
Please submit a PR to the docs repo to add your analytics adapter so your analytics adapter's will appear on the this page. Thank you for contributing

Copy link
Collaborator

@jsnellbaker jsnellbaker left a comment

Choose a reason for hiding this comment

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

@mquirion Thanks for making the code updates to the main file and the unit tests. From that perspective, we look good now.

If you can submit a docs PR (see comments above), then we can merge this PR.

@mquirion
Copy link
Contributor Author

mquirion commented Jun 5, 2019

@mquirion Can you provide docs PR link. If not created, please create one.

Hi @sumit116 here's the PR for the doc update. Thanks for your help on this!

prebid/prebid.github.io#1341

@jsnellbaker jsnellbaker merged commit 81932cd into prebid:master Jun 5, 2019
ghost pushed a commit to devunrulymedia/Prebid.js that referenced this pull request Jun 7, 2019
* initial dev

* fix staq adapter name

* fix hello world staq call

* get hello world working again

* add user agent collection

* fix some unite tests

* Add STAQ Analytics Adapter doc

* clean up hello world

* fix tests to play nice with browserstack

* fix around issues with browserstack and deep equals of objects

* dump variable env testing since we can't mod user agent stuff in browserstack

* Update STAQ adapter to stop using deprecated utils for referrer

* remove package-lock.json changes via master rebase

* improve call frequency for ref util

* change ajax content type

* adjust ajax request to not expect whitelisting

* remove superflous commented-out code
VideoReach pushed a commit to VideoReach/Prebid.js that referenced this pull request Aug 1, 2019
* initial dev

* fix staq adapter name

* fix hello world staq call

* get hello world working again

* add user agent collection

* fix some unite tests

* Add STAQ Analytics Adapter doc

* clean up hello world

* fix tests to play nice with browserstack

* fix around issues with browserstack and deep equals of objects

* dump variable env testing since we can't mod user agent stuff in browserstack

* Update STAQ adapter to stop using deprecated utils for referrer

* remove package-lock.json changes via master rebase

* improve call frequency for ref util

* change ajax content type

* adjust ajax request to not expect whitelisting

* remove superflous commented-out code
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants