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

Bizzclick: adapter update, add new host param #3347

Merged
merged 5 commits into from
Jan 18, 2024

Conversation

BizzClick
Copy link
Contributor

@BizzClick BizzClick commented Dec 12, 2023

Hi, the adapter was updated with new sourceId and host params.
Related PR in prebid.github.io

Copy link

Code coverage summary

Note:

  • Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
  • Coverage summary encompasses all commits leading up to the latest one, 95c41ac

bizzclick

Refer here for heat map coverage report

github.com/prebid/prebid-server/v2/adapters/bizzclick/bizzclick.go:21:	Builder				100.0%
github.com/prebid/prebid-server/v2/adapters/bizzclick/bizzclick.go:33:	getHeaders			100.0%
github.com/prebid/prebid-server/v2/adapters/bizzclick/bizzclick.go:56:	MakeRequests			83.3%
github.com/prebid/prebid-server/v2/adapters/bizzclick/bizzclick.go:89:	getImpressionExt		85.7%
github.com/prebid/prebid-server/v2/adapters/bizzclick/bizzclick.go:106:	buildEndpointURL		100.0%
github.com/prebid/prebid-server/v2/adapters/bizzclick/bizzclick.go:115:	checkResponseStatusCodes	100.0%
github.com/prebid/prebid-server/v2/adapters/bizzclick/bizzclick.go:137:	MakeBids			100.0%
github.com/prebid/prebid-server/v2/adapters/bizzclick/bizzclick.go:183:	getMediaTypeForImp		100.0%
total:									(statements)			96.0%

@bretg
Copy link
Contributor

bretg commented Dec 14, 2023

As noted in the docs PR, asking for publishers to enter the geographic region is not the right way to do this.

For Prebid.js, you really ought to be paying for a global traffic management solution.
For Prebid Server, a solution is to have the host company deploy a different endpoint in different regions. See #3311

Since the host parameter is optional, I won't insist, but I'm going to request that you consider how difficult it might be for a publisher to carry out this request.

@BizzClick
Copy link
Contributor Author

@bretg We're using it because we have different regions/datacenters of our servers. For instance US East and Europe regions, such parameter would be useful to reduce network latency.
I any case, we can receive traffic to our US servers from anywhere, but it would be preferable to get to the appropriate region

@bretg
Copy link
Contributor

bretg commented Dec 15, 2023

because we have different regions/datacenters of our servers

Many of us are in that situation that's not the problem. The problem is in how you're asking publishers to choose the region. That places an undue (often impossible) burden on them.

Copying what was said above:

For Prebid.js, you really ought to be paying for a global traffic management solution.

Many vendors offer such solutions. Search for "gslb solutions"

For Prebid Server, a solution is to have the host company deploy a different endpoint in different regions. See #3311

Please consider updating your bizzclick.yaml file similar to the rubicon.yaml there. https://github.com/prebid/prebid-server/pull/3311/files#diff-8fc6fb8199a8e0dcf5c4a887cf074043b99af8fdb349c2176ae5644e13a35ecc . You can your host param as a default if you'd like, though you can see here that Rubicon insists that PBS host companies make the choice based per-datacenter.

@BizzClick
Copy link
Contributor Author

@bretg I will add geoscope to .yaml file. But I think in our case it would be better to give us the right to chose. We're providing to publishers our fixed host parameter with the rest of params. So we can't let give a right to chose region for publisher, because due to internal settings of exchange each publisher is tied to its own region, though we can change it, but we would like to avoid that

Copy link

Code coverage summary

Note:

  • Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
  • Coverage summary encompasses all commits leading up to the latest one, 97cc6d7

bizzclick

Refer here for heat map coverage report

github.com/prebid/prebid-server/v2/adapters/bizzclick/bizzclick.go:21:	Builder				100.0%
github.com/prebid/prebid-server/v2/adapters/bizzclick/bizzclick.go:33:	getHeaders			100.0%
github.com/prebid/prebid-server/v2/adapters/bizzclick/bizzclick.go:56:	MakeRequests			83.3%
github.com/prebid/prebid-server/v2/adapters/bizzclick/bizzclick.go:89:	getImpressionExt		85.7%
github.com/prebid/prebid-server/v2/adapters/bizzclick/bizzclick.go:106:	buildEndpointURL		100.0%
github.com/prebid/prebid-server/v2/adapters/bizzclick/bizzclick.go:115:	checkResponseStatusCodes	100.0%
github.com/prebid/prebid-server/v2/adapters/bizzclick/bizzclick.go:137:	MakeBids			100.0%
github.com/prebid/prebid-server/v2/adapters/bizzclick/bizzclick.go:183:	getMediaTypeForImp		100.0%
total:									(statements)			96.0%

@bretg
Copy link
Contributor

bretg commented Dec 18, 2023

each publisher is tied to its own region

To be honest, I find it quite unfriendly to make publishers enter this static parameter on every adunit for architecture reasons. But as long as host is not a required param, we will tolerate it.

Copy link

Code coverage summary

Note:

  • Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
  • Coverage summary encompasses all commits leading up to the latest one, b07eb3e

bizzclick

Refer here for heat map coverage report

github.com/prebid/prebid-server/v2/adapters/bizzclick/bizzclick.go:21:	Builder				100.0%
github.com/prebid/prebid-server/v2/adapters/bizzclick/bizzclick.go:33:	getHeaders			100.0%
github.com/prebid/prebid-server/v2/adapters/bizzclick/bizzclick.go:56:	MakeRequests			83.3%
github.com/prebid/prebid-server/v2/adapters/bizzclick/bizzclick.go:89:	getImpressionExt		85.7%
github.com/prebid/prebid-server/v2/adapters/bizzclick/bizzclick.go:106:	buildEndpointURL		100.0%
github.com/prebid/prebid-server/v2/adapters/bizzclick/bizzclick.go:115:	checkResponseStatusCodes	100.0%
github.com/prebid/prebid-server/v2/adapters/bizzclick/bizzclick.go:137:	MakeBids			100.0%
github.com/prebid/prebid-server/v2/adapters/bizzclick/bizzclick.go:183:	getMediaTypeForImp		100.0%
total:									(statements)			96.0%

@BizzClick
Copy link
Contributor Author

@bretg Ok, I'll keep host parameter and I've updated bizzclick.yaml according to your previous messages. Let me know if it fits

Comment on lines 12 to 26
"placementId": {
"sourceId": {
"type": "string",
"description": "PlacementId id",
"description": "Source id",
"minLength": 1
},
"host": {
"type": "string",
"description": "Server region",
"minLength": 1
}
},
"required": ["accountId", "placementId"]
"required": [
"accountId",
"sourceId"
]
Copy link
Contributor

@onkarvhanumante onkarvhanumante Jan 9, 2024

Choose a reason for hiding this comment

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

@BizzClick , placementId was required param and PR is renaming placementId to sourceId.

This will be an breaking change for the publishers making call to adapter. Should consider a transition period before making such change. Transition period will give provide enough time for the publishers and PBS host compaines to accomadate for such change.

As of now,

  • will recommend to keep placementId and mark it as deprecated in bidder docs. Alsoto anyone as shown below for required params

    "oneOf": [
     { "required": ["accountId", "placementId"] },
     { "required": ["accountId", "sourceId"] }
    ]
    
  • Refer below example for deprecating placementId

    "publisher_id": {
    "type": "string",
    "description": "Deprecated, use org instead."
    }

  • Adapter code can be changed to use one of sourceId or placementId

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All right, that looks reasonable. Fixed in the last commit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Related commit in doc

Comment on lines +107 to +110
host := "us-e-node1"
if params.Host != "" {
host = params.Host
}
Copy link
Contributor

Choose a reason for hiding this comment

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

should add/ update test where params.Host is empty

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test was added here:
adapters/bizzclick/bizzclicktest/exemplary/default-host-param.json
With the placementId param also

Copy link

Code coverage summary

Note:

  • Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
  • Coverage summary encompasses all commits leading up to the latest one, 16bd598

bizzclick

Refer here for heat map coverage report

github.com/prebid/prebid-server/v2/adapters/bizzclick/bizzclick.go:21:	Builder				100.0%
github.com/prebid/prebid-server/v2/adapters/bizzclick/bizzclick.go:33:	getHeaders			100.0%
github.com/prebid/prebid-server/v2/adapters/bizzclick/bizzclick.go:56:	MakeRequests			83.3%
github.com/prebid/prebid-server/v2/adapters/bizzclick/bizzclick.go:89:	getImpressionExt		85.7%
github.com/prebid/prebid-server/v2/adapters/bizzclick/bizzclick.go:106:	buildEndpointURL		100.0%
github.com/prebid/prebid-server/v2/adapters/bizzclick/bizzclick.go:119:	checkResponseStatusCodes	100.0%
github.com/prebid/prebid-server/v2/adapters/bizzclick/bizzclick.go:141:	MakeBids			100.0%
github.com/prebid/prebid-server/v2/adapters/bizzclick/bizzclick.go:187:	getMediaTypeForImp		100.0%
total:									(statements)			96.2%

@gargcreation1992 gargcreation1992 merged commit 6195581 into prebid:master Jan 18, 2024
5 checks passed
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