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

update AJA adaptor: support native format #3504

Merged
merged 5 commits into from
Mar 7, 2019

Conversation

naoto0822
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?
  • Other

Description of change

  • support native format

Other information

doc PR is prebid/prebid.github.io#1126

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.

Hi so far it looks good to me,

However, the test params for all three formats provided in the ajaBidAdapter.md do not solicit valid bid responses.

Including the new one for native.

Can you please provide me with information on how to ensure test params do return bid responses, as well as update the MD accordingly?

@naoto0822 naoto0822 force-pushed the feature/aja-support-native branch from 1ecb12e to 2862bd7 Compare February 7, 2019 08:37
@naoto0822
Copy link
Contributor Author

naoto0822 commented Feb 7, 2019

@robertrmartinez
Thanks for reviewing this PR!

However, the test params for all three formats provided in the ajaBidAdapter.md do not solicit valid bid responses.
Including the new one for native.

Sorry for providing invalid params.
So i updated the ajaBidAdapter.md for all format.

Please re-review that adaptor receive bid response.

@naoto0822
Copy link
Contributor Author

The following was displayed in my test.

image

image

image

@robertrmartinez
Copy link
Collaborator

@naoto0822 I still am only getting a bid back for the video ad unit you provided:

image

image

image

Is there some other configuration I am missing?

@naoto0822
Copy link
Contributor Author

@robertrmartinez
I modified the inventory configuration.
Please check that API return bid response.

@robertrmartinez
Copy link
Collaborator

@naoto0822 Thanks!

I am getting a valid bid response for all now.

I have a question regarding the cickUrl AJA is setting for it's native ads.

The bid response I am getting has a clickUrl of almost 4,000 characters.

image

Is this normal? I can see this causing issues with GPT as the Key Value pair might reach their maximum very soon causing other ad units to be filtered out possibly!

Can you explain to me what AJA's stance on this is?

@naoto0822
Copy link
Contributor Author

@robertrmartinez
Thanks for reply!

I am getting a valid bid response for all now.

That's great !

The bid response I am getting has a clickUrl of almost 4,000 characters.
Is this normal? I can see this causing issues with GPT as the Key Value pair might reach their maximum very soon causing other ad units to be filtered out possibly!

I didn's sure that GPT as the key-value have max length.
How long are key-value max length?

Can you explain to me what AJA's stance on this is?

Yes, this is normal and there were no problem with my test.....
AJA's clickUrl is encrypted after received from DSP. So clickUrl is long.

@robertrmartinez
Copy link
Collaborator

@naoto0822

Ok I dug into this a little more and found a related issue #2902

Specifically this comment, which confirms my suspicions about DFP Dropping KVPS when they get too large:
image

I spoke with the Prebid.org team and we have made this a priority to get a solution for this into PRebid-Core code.

The plan is to get it done in the next week or so.

Until then, we will keep this PR on hold and merge it into the same release as the Solution related to #2902

@naoto0822
Copy link
Contributor Author

@robertrmartinez
Thanks for teaching me DFP spec and related issue.

I spoke with the Prebid.org team and we have made this a priority to get a solution for this into PRebid-Core code.
The plan is to get it done in the next week or so.
Until then, we will keep this PR on hold and merge it into the same release as the Solution related to #2902

OK, i understand.
Thank you for modifying Prebid-Core code for this problem.

@stale
Copy link

stale bot commented Feb 28, 2019

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 Feb 28, 2019
@stale stale bot removed the stale label Mar 4, 2019
@naoto0822
Copy link
Contributor Author

@robertrmartinez
Hi.
I merged current master to this branch and tested using placeholders ( http://prebid.org/dev-docs/show-native-ads.html#sending-asset-placeholders ).

So i updated the ajaBidAdapter.md.
Please re-review this PR.

@naoto0822
Copy link
Contributor Author

@robertrmartinez
Hi.

Until then, we will keep this PR on hold and merge it into the same release as the Solution related to #2902

I guess that #2902 was resolved by #3573.
Is This PR still onhold status?

@robertrmartinez
Copy link
Collaborator

@naoto0822 Sorry for the delay!!

I have re-reviewed and will merge now!

Thank you for your patience !!

@robertrmartinez robertrmartinez self-requested a review March 7, 2019 19:09
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.

👍 LGTM

@robertrmartinez robertrmartinez merged commit 7ce5df6 into prebid:master Mar 7, 2019
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