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 Adapter: freewheelssp #2392

Merged
merged 12 commits into from
Nov 15, 2022
Merged

Conversation

mwang-sticky
Copy link
Contributor

No description provided.

@SyntaxNode SyntaxNode changed the title add freewheelssp go adapter New Adapter: freewheelssp Sep 27, 2022
@SyntaxNode SyntaxNode self-requested a review September 27, 2022 16:25
@SyntaxNode SyntaxNode self-assigned this Sep 27, 2022
adapters/freewheelssp/freewheelssp.go Outdated Show resolved Hide resolved
adapters/freewheelssp/freewheelssp.go Outdated Show resolved Hide resolved
adapters/freewheelssp/freewheelssp.go Outdated Show resolved Hide resolved
adapters/freewheelssp/freewheelssp.go Outdated Show resolved Hide resolved
adapters/freewheelssp/freewheelssp.go Outdated Show resolved Hide resolved
adapters/freewheelssp/freewheelssp.go Outdated Show resolved Hide resolved
adapters/freewheelssp/freewheelssp.go Show resolved Hide resolved
adapters/freewheelssp/freewheelssp_test.go Outdated Show resolved Hide resolved
exchange/adapter_builders.go Outdated Show resolved Hide resolved
@bsardo bsardo self-requested a review September 27, 2022 17:37
@bsardo bsardo self-assigned this Sep 27, 2022
@bsardo bsardo requested a review from AlexBVolcy October 10, 2022 17:27
@bsardo bsardo assigned AlexBVolcy and unassigned SyntaxNode Oct 10, 2022
Copy link
Contributor

@AlexBVolcy AlexBVolcy left a comment

Choose a reason for hiding this comment

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

Could you please link your docs PR?


if response.StatusCode != http.StatusOK {
return nil, []error{&errortypes.BadServerResponse{
Message: fmt.Sprintf("Unexpected status code: %d. Run with request.debug = 1 for more info", response.StatusCode),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to cover this error case with tests?

@AlexBVolcy
Copy link
Contributor

There is a PR #2370 that is being merged today that will cause conflicts to new adapter PRs.

To resolve these conflicts, you'll need to do the following:

In freewheelssp.go, update the Builder() function signature to include a new parameter server config.Server

In freewheelssp_test.go, update any test that includes a call to Builder() to pass a config.Server object, to that call (i.e. config.Server{ExternalUrl: "http://hosturl.com", GvlID: 1, DataCenter: "datacenter"})

After the merge, you can look at other adapters to see how the server object is implemented as an example.

Copy link
Contributor

@SyntaxNode SyntaxNode left a comment

Choose a reason for hiding this comment

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

Looks good. Thank you for the fixes.

FYI: Prebid Server has 6 characters of guaranteed bidder name uniqueness. For freehweelssp that would be freewh. I'm sharing this because your adapter name is freewheelssp instead of just freewheel, so if you plan to add other adapters within the freewheel family in the future (ie freewheel123), the targeting keys may not be unique between them. Not a problem, just wanted you to be aware.

- video
userSync:
redirect:
url: "https://ads.stickyadstv.com/pbs-user-sync?gdpr={{.GDPR}}&gdpr_consent={{.GDPRConsent}}&us_privacy={{.USPrivacy}}&r={{.RedirectURL}}"
Copy link
Contributor

Choose a reason for hiding this comment

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

I tested the user sync endpoint and discovered it returns html instead of a 302 redirect. Should be defined as an iframe sync instead of a redirect?

FW-113142 modify code according review (prebid#2)

FW-113142  1.503-bid-resposne. 2.Add server to builder (prebid#3)

* modify code according review

* FW-113142   1.503-bid-resposne.  2.Add server to builder
@bsardo
Copy link
Collaborator

bsardo commented Oct 19, 2022

Hi @mwang-sticky, we'll check out your changes shortly. Also, if you don't mind, could you please avoid force pushing after the PR has been reviewed? We prefer you just push another commit with your changes as this helps the reviewers save a lot of time since they often only need to review the delta. And if conflicts arise they can be resolved by merging with master instead of rebasing with a force push 🙂. Thanks again for your contribution.

Copy link
Contributor

@AlexBVolcy AlexBVolcy left a comment

Choose a reason for hiding this comment

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

This looks good, but I still don't see a link to your docs PR, could you please add this as a comment on this PR

@mwang-sticky
Copy link
Contributor Author

mwang-sticky commented Oct 20, 2022

@bsardo @AlexBVolcy
Sorry for force commit to resolve some conflict,
I add the doc request: prebid/prebid.github.io#4094
however our prebid js still use freeweel-ssp which is not supported by GAM, then prebid-go has changed to new biddercode freewheelssp, please tell me how can I solve this difference, does it impact prebid integration?

  1. should I change the filename and default biddercode to freewheelssp, and add aliasCode: freewheel-ssp

I look forward your help.

@bsardo
Copy link
Collaborator

bsardo commented Oct 20, 2022

Hi @mwang-sticky, we strongly prefer prebid.js and PBS use the same bidder codes. If you use different biddercodes it confuses publishers and leads to confusing documentation.

It sounds like you want to move away from freewheel-ssp to freewheelssp with one of the main reasons being the new name is supported by GAM. Can you confirm?

If your long term goal is to standardize on freewheelssp, then we recommend one of the following two courses of action:

  1. If you plan to update prebid.js with the new name freewheelssp in the immediate future, then you should be ok just supporting freewheelssp in PBS since you'll effectively be forcing publishers to update to the new name even if they just want to use prebid.js with an imminent prebid.js update.
  2. If you plan to update prebid.js with the new name eventually (say in 2023 or beyond), then we recommend adding an alias in PBS so publishers can easily switch between the two names until prebid.js catches up. In order to do this, you would create a hardcoded alias where you have two bidder names of type openrtb_ext.BidderName and a single adapter of type adapters.Builder that maps to each of the two bidder names. See PR#2302 as an example of how to add a hardcoded alias in PBS.

I hope this helps. If I misunderstood your intention, let me know.

@mwang-sticky
Copy link
Contributor Author

mwang-sticky commented Oct 24, 2022

It sounds like you want to move away from freewheel-ssp to freewheelssp with one of the main reasons being the new name is supported by GAM. Can you confirm?

Yes, some of our publishers asked us to change the old biddercode.

If your long term goal is to standardize on freewheelssp, then we recommend one of the following two courses of action:

Thanks for your good suggestion. I add the old bidder code in go adapter.
Our js team has add the new bidder name, however because some of our clients has used the old prebid js, then we don't want to impact a lot. prebid/Prebid.js#9093

Does this change work for prebid go/js integration? because the bidder names are conflicted

@bsardo
Copy link
Collaborator

bsardo commented Oct 25, 2022

Hi @mwang-sticky, I'm sorry I'm not able to confirm your PBS and prebid.js integration works as that is outside of the scope of our team. I do believe though that as long as prebid.js and PBS support the same bidder parameters for your bidder and each has support for both bidder codes, the integration should work.

@mwang-sticky
Copy link
Contributor Author

@bsardo
As the biddercode is conflicted, how can I to solve this error

bidders_validate_test.go:62: Error Trace: bidders_validate_test.go:62 Error: "10" is not less than or equal to "6" Test: TestBidderUniquenessGatekeeping
Thanks,

@SyntaxNode
Copy link
Contributor

@bsardo As the biddercode is conflicted, how can I to solve this error

bidders_validate_test.go:62: Error Trace: bidders_validate_test.go:62 Error: "10" is not less than or equal to "6" Test:TestBidderUniquenessGatekeeping Thanks,

This is to ensure the bidder name is unique enough in targeting keys. The uniqueness constraint is not usually important for aliases. So long as a bid from freewheelssp and freewheel-ssp are the same, then you can add an exception along side Triplelift and Adkernel. ie:

if bidder != BidderTripleliftNative && bidder != BidderAdkernelAdn && bidder != BidderFreewheelSSPOld {

@mwang-sticky
Copy link
Contributor Author

if bidder != BidderTripleliftNative && bidder != BidderAdkernelAdn && bidder != BidderFreewheelSSPOld {
I updated this, should I update other info?

Copy link
Contributor

@AlexBVolcy AlexBVolcy 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 ready to approve, but @bsardo I just want to confirm how we want to go about PRs that are throwing this gofmt error in the Github validation?

@bsardo
Copy link
Collaborator

bsardo commented Nov 1, 2022

Hi @mwang-sticky, can you merge with master when you get a chance? Your code is failing the go 1.19 validation checks due to go fmt changes that were introduced with the recent upgrade to go 1.19 on master.

@mwang-sticky
Copy link
Contributor Author

@bsardo I synced the master to my branch, thanks for your reminder.

AlexBVolcy
AlexBVolcy previously approved these changes Nov 2, 2022
@bsardo bsardo requested review from VeronikaSolovei9 and removed request for bsardo November 8, 2022 18:29
@bsardo bsardo assigned VeronikaSolovei9 and unassigned bsardo Nov 8, 2022

type RequestImpExt struct {
ZoneId int `json:"zoneId"`
}
Copy link
Contributor

@VeronikaSolovei9 VeronikaSolovei9 Nov 9, 2022

Choose a reason for hiding this comment

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

No need ot specify imp ext here. It's already specified in openrtb_ext/imp_freewheelssp.go

adapters/freewheelssp/freewheelssp.go Outdated Show resolved Hide resolved
Copy link
Contributor

@VeronikaSolovei9 VeronikaSolovei9 left a comment

Choose a reason for hiding this comment

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

Thank you for addressing comments, lgtm

Copy link
Contributor

@AlexBVolcy AlexBVolcy left a comment

Choose a reason for hiding this comment

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

LGTM

@bsardo bsardo merged commit d912d2a into prebid:master Nov 15, 2022
shunj-nb pushed a commit to ParticleMedia/prebid-server that referenced this pull request Jan 31, 2023
Co-authored-by: Kai Zhang <kaizhang@freewheel.com>
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.

5 participants