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: OwnAdX #2850

Merged
merged 9 commits into from
Jul 26, 2023
Merged

Conversation

ownAdx-prebid
Copy link
Contributor

No description provided.

go.sum Outdated Show resolved Hide resolved
@ownAdx-prebid
Copy link
Contributor Author

ownAdx-prebid commented Jun 21, 2023 via email

@ownAdx-prebid
Copy link
Contributor Author

ownAdx-prebid commented Jun 21, 2023 via email

@ownAdx-prebid
Copy link
Contributor Author

ownAdx-prebid commented Jun 23, 2023 via email

@ownAdx-prebid
Copy link
Contributor Author

ownAdx-prebid commented Jun 26, 2023 via email

@onkarvhanumante
Copy link
Contributor

Hello Prebid Team, can you please update us why we stuck here,if any changes will required so please let me know. Thank you

@ownAdx-prebid, we are following up this url related change http://{{.Host}}-prebid.owadx.com/bidder/bid/{{.AccountID}}/000?token={{.SourceId}}

@Sonali-More-Xandr
Copy link
Contributor

Sonali-More-Xandr commented Jun 27, 2023

@ownAdx-prebid Please add dev-docs here

@ownAdx-prebid
Copy link
Contributor Author

@ownAdx-prebid Please add dev-docs here

We are submitting the OwnadX documentation with PR id #4687

@bretg
Copy link
Contributor

bretg commented Jun 30, 2023

docs PR prebid/prebid.github.io#4687

@onkarvhanumante
Copy link
Contributor

prebid/prebid.github.io#4687

@ownAdx-prebid thanks for keeping patience with code reviews. Agree that few adapter today make use of dynamic subdomain. But this usage is being discouraged and prebid team in future will be working to avoid usage of dynamic subdomain in endpoint url.

Major concerns with such usage are,

  • security concerns
    The security aspect is alleviated by using a fixed top level domain. Due to the potential harm to hosts, we are strict in this requirement. We are working towards fixing the few adapters currently in violation.
  • connection performance
    The connection performance advice is for your benefit. Client specific subdomains prevent Prebid Server from reusing connections across your clients which results in more connections needed for your adapter. The issue gets worse the more successful you are in attracting new clients. Hosts may choose to disable your adapter due to this behavior.

Best practice is for your domain to be fixed and any parameters specified in the path or query.

@ownAdx-prebid
Copy link
Contributor Author

prebid/prebid.github.io#4687

@ownAdx-prebid thanks for keeping patience with code reviews. Agree that few adapter today make use of dynamic subdomain. But this usage is being discouraged and prebid team in future will be working to avoid usage of dynamic subdomain in endpoint url.

Major concerns with such usage are,

  • security concerns
    The security aspect is alleviated by using a fixed top level domain. Due to the potential harm to hosts, we are strict in this requirement. We are working towards fixing the few adapters currently in violation.
  • connection performance
    The connection performance advice is for your benefit. Client specific subdomains prevent Prebid Server from reusing connections across your clients which results in more connections needed for your adapter. The issue gets worse the more successful you are in attracting new clients. Hosts may choose to disable your adapter due to this behavior.

Best practice is for your domain to be fixed and any parameters specified in the path or query.

Thank you for details.
We will make changes according to it and will back to you

@ownAdx-prebid
Copy link
Contributor Author

Hello @onkarvhanumante
We are change dynamic host to static host. Let me know any more changes.

Copy link
Contributor

@onkarvhanumante onkarvhanumante left a comment

Choose a reason for hiding this comment

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

@ownAdx-prebid added some suggestions

@onkarvhanumante
Copy link
Contributor

@onkarvhanumante can you elaborate more about point should consider appending err if check on line 202 is true

imps, impExts, err := getImpressionsAndImpExt(request.Imp)
if len(imps) == 0 {

apologies, I meant to refer 102 line

@ownAdx-prebid
Copy link
Contributor Author

Hello @onkarvhanumante
We make changes as you suggest .

@onkarvhanumante
Copy link
Contributor

onkarvhanumante commented Jul 18, 2023

@ownAdx-prebid requesting to address opened comments

@ownAdx-prebid
Copy link
Contributor Author

@onkarvhanumante can you elaborate more about point should consider appending err if check on line 202 is true

imps, impExts, err := getImpressionsAndImpExt(request.Imp)
if len(imps) == 0 {

apologies, I meant to refer 102 line

We make changes for it

@ownAdx-prebid
Copy link
Contributor Author

#discussion_r1263495180
We make changes for it.

@onkarvhanumante
Copy link
Contributor

@ownAdx-prebid ownAdx-prebid:NewAdapter/OwnAdX branch seems out of sync with prebid master. Resulting in failure of Adapter semgrep checks. Requesting sync your branch with master

@ownAdx-prebid
Copy link
Contributor Author

Hello @onkarvhanumante
We sync with prebid master and done changes for #discussion_r1266640622

@onkarvhanumante
Copy link
Contributor

@ownAdx-prebid requesting to address or provide views on

@ownAdx-prebid
Copy link
Contributor Author

ownAdx-prebid commented Jul 20, 2023

@onkarvhanumante

@onkarvhanumante
Copy link
Contributor

@onkarvhanumante

r1263550459 is not Mandatory. But more recommended way with latest ortb specs

@onkarvhanumante onkarvhanumante merged commit 33714f2 into prebid:master Jul 26, 2023
4 checks passed
@bretg bretg mentioned this pull request Jun 25, 2024
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