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

AppNexus AST Debug Auction #3025

Closed
wants to merge 3 commits into from
Closed

Conversation

aneuway2
Copy link
Contributor

Type of change

  • Feature

Description of change

The following feature enhancement adds in the ability for a publisher or developer to request an AppNexus AST debug auction in the response back from AppNexus alongside any bids.

This feature is disabled by default and can be enabled by adding a debug object in parallel to the bids requested (which is the preferred way of running a debug auction):

{
  bidder: 'appnexus',
  params: {
    placementId: 123456789
  },
  debug: {
    enabled: true,
    dongle: 'QWERTY',
    member_id: 958,
    debug_timeout: 3000
  }
}

The following also may be set with a combination of the following URL parameters (and sample values) that must be used in conjunction with pbjs_debug=true as a URL parameter:

  • ast_debug=true
  • ast_dongle=QWERTY
  • ast_debug_member=958
  • ast_debug_timeout=1200

(e.g http://example.com/?pbjs_debug=true&ast_debug=true&ast_dongle=QWERTY&ast_debug_member=882&ast_debug_timeout=1200)

When either of the above is enabled, a debug auction will be shown in the Prebid console log statements along with relevant details about the auction that took place. e.g:

Running member 958 debug using no permissions
Impbus 4280.bm-impbus.prod.nym2:8001
	Header Referer: http://example.com?pbjs_debug=true&ast_debug=true&ast_dongle=sss
	Site Domain: example.com
1772142023491463321 - Predicted engagement_rate_id: 2 (view) imps 54.87%
1772142023491463321 - Predicted engagement_rate_id: 3 (view_over_total) imps 51.57%
1772142023491463321 - Predicted engagement_rate_id: 8 (predicted_100pv1s_display_view_rate) imps 47.06%

@mkendall07
Copy link
Member

This pull request introduces 1 alert when merging 32c6c80 into d7f3f90 - view on LGTM.com

new alerts:

  • 1 for Useless conditional

Comment posted by LGTM.com

@jaiminpanchal27 jaiminpanchal27 self-requested a review August 29, 2018 20:59
@jaiminpanchal27 jaiminpanchal27 self-assigned this Aug 29, 2018
@mkendall07
Copy link
Member

This pull request introduces 1 alert when merging 32c6c80 into fa78634 - view on LGTM.com

new alerts:

  • 1 for Useless conditional

Comment posted by LGTM.com

Copy link
Member

@mkendall07 mkendall07 left a comment

Choose a reason for hiding this comment

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

I'm not sure we want to introduce bid adapters to being able to read query params. That's a lot of stuff for us to keep track of per bidder. I wonder if we could do a cookie for this instead?

@aneuway2
Copy link
Contributor Author

Hey @mkendall07 I based this approach off of audienceNetwork's anhb_testmode query string parameter which toggles a "test" mode: https://github.com/prebid/Prebid.js/blob/master/modules/audienceNetworkBidAdapter.js#L112

The query string parameters in this case are meant more to override what would be set via a parameter on the bid request--so it would still be part of the documentation with the URL parameter as the alternative. (maybe this is the approach that should be taken to ensure tests/etc work as expected?)

I could move to a cookie approach, but that makes this a bit more complex for users to access though imo (the process of setting the cookie might also need to be set multiple times if the publisher has multiple domains)

@stale
Copy link

stale bot commented Sep 14, 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 Sep 14, 2018
@aneuway2
Copy link
Contributor Author

I plan to revise based on #3067

@mkendall07
Copy link
Member

@aneuway2 let's chat before you update.

@stale
Copy link

stale bot commented Sep 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 Sep 28, 2018
@aneuway2
Copy link
Contributor Author

aneuway2 commented Oct 3, 2018

Closing this PR in favor of #3152

@aneuway2 aneuway2 closed this Oct 3, 2018
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.

3 participants