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

MgidX: add optional region param #3396

Closed
wants to merge 20 commits into from
Closed

MgidX: add optional region param #3396

wants to merge 20 commits into from

Conversation

xmgiddev
Copy link
Contributor

doc - prebid/prebid.github.io#4932
continuation - #3220

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, bf4ae87

mgidX

Refer here for heat map coverage report

github.com/prebid/prebid-server/v2/adapters/mgidX/mgidX.go:31:	Builder			80.0%
github.com/prebid/prebid-server/v2/adapters/mgidX/mgidX.go:44:	MakeRequests		83.9%
github.com/prebid/prebid-server/v2/adapters/mgidX/mgidX.go:98:	getImpressionExt	71.4%
github.com/prebid/prebid-server/v2/adapters/mgidX/mgidX.go:112:	buildEndpointURL	80.0%
github.com/prebid/prebid-server/v2/adapters/mgidX/mgidX.go:124:	makeRequest		78.6%
github.com/prebid/prebid-server/v2/adapters/mgidX/mgidX.go:152:	MakeBids		100.0%
github.com/prebid/prebid-server/v2/adapters/mgidX/mgidX.go:191:	getBidMediaType		85.7%
total:								(statements)		85.4%

if params.Region == "eu" {
endpointParams = macros.EndpointTemplateParams{Host: "eu"}
} else {
endpointParams = macros.EndpointTemplateParams{Host: "us-east-x"}
Copy link
Contributor

Choose a reason for hiding this comment

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

This implementation will prevent a host company from sending all traffic from their EU data centers to yours, as the control is entirely placed on the publisher. Would you consider falling back to a host specified user endpoint instead of hardcoding US East?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, a host could still override the endpoint to include the eu subdomain and omit the macro. I think that's fine, if you could please make that clear through documentation.

endpoint: "https://us-east-x.mgid.com/pserver"
endpoint: "https://{{.Host}}.mgid.com/pserver"
Copy link
Contributor

Choose a reason for hiding this comment

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

@xmgiddev

should update json test to include endpoint from bidder-info

bidder, buildErr := Builder(openrtb_ext.BidderEmtv, config.Adapter{
Endpoint: "http://example.com/pserver"}, config.Server{ExternalUrl: "http://hosturl.com", GvlID: 1, DataCenter: "2"})
if buildErr != nil {
t.Fatalf("Builder returned unexpected error %v", buildErr)
}
adapterstest.RunJSONBidderTest(t, "mgidXtest", bidder)
}

Comment on lines +125 to +132
var mgidXExt *openrtb_ext.ImpExtMgidX
mgidXExt, err := a.getImpressionExt(&(request.Imp[0]))
if err != nil {
return nil, err
}

url, err := a.buildEndpointURL(mgidXExt)
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

only request.Imp[0] is considered to prepare mgidXExt

@xmgiddev could you clearify whether adapter supports multi-imps request. If not then should specify this in bidder docs. If multi-imps are supported then should mention that only request.Imp[0] will be considered to prepare mgidXExt

@Sonali-More-Xandr
Copy link
Contributor

@xmgiddev Could you please take a look at the comments above?

@onkarvhanumante
Copy link
Contributor

@xmgiddev requesting to take a look at PR comments

@onkarvhanumante
Copy link
Contributor

closing this PR due to inactivity.

@xmgiddev, could reopen this PR when comments are addressed or new changes are pushed

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