Integration Type Implementation #2111

AlexBVolcy · 2021-12-14T17:42:40Z

This PR addresses the Integration Type portion of the Integration Type and Channel issue #1428

This PR does the following:

Reads integration type value from request from the path request.ext.prebid.integration
Allows account/publisher to have a DefaultIntegration value, that will be utilized in the case where an integration type isn't found in request
Once integration value is captured/set, it is then set as a query parameter for event URL and vtrack URL

(Note: Integration Type value is not required, nor does there need to be validation of the value per Bret)

bsardo · 2022-01-05T17:43:29Z

analytics/event.go

+	AccountID       string         `json:"account_id,omitempty"`
+	Bidder          string         `json:"bidder,omitempty"`
+	Timestamp       int64          `json:"timestamp,omitempty"`
+	IntegrationType string         `json:"integration_type,omitempty"`


Where did the field name integration_type come from? Is that something that was discussed with Bret or did we come up with that name ourselves?

Came up with the name myself, but I'll check the PBS-Java code to see what they do, but like you commented somewhere else, I might just update it to integration

👍 It looks like they do call it integration in PBS-Java.

bsardo · 2022-01-05T17:44:34Z

config/accounts.go

+	CCPA               AccountCCPA `mapstructure:"ccpa" json:"ccpa"`
+	GDPR               AccountGDPR `mapstructure:"gdpr" json:"gdpr"`
+	DebugAllow         bool        `mapstructure:"debug_allow" json:"debug_allow"`
+	DefaultIntegration string      `mapstructure:"default_integration"`


Same question as above: is default_integration the name PBS-Java is using too? From the corresponding issue, it seems like the name should just be integration.

👍 I see PBS-Java is using default-integration.

bsardo · 2022-01-05T17:45:09Z

config/accounts.go

+	CCPA               AccountCCPA `mapstructure:"ccpa" json:"ccpa"`
+	GDPR               AccountGDPR `mapstructure:"gdpr" json:"gdpr"`
+	DebugAllow         bool        `mapstructure:"debug_allow" json:"debug_allow"`
+	DefaultIntegration string      `mapstructure:"default_integration"`


It looks like you're missing json:"default_integration" in the annotations.

bsardo · 2022-01-05T17:52:49Z

config/config.go

@@ -88,6 +88,8 @@ type Configuration struct {
 	GenerateBidID bool `mapstructure:"generate_bid_id"`
 	// GenerateRequestID overrides the bidrequest.id in an AMP Request or an App Stored Request with a generated UUID if set to true. The default is false.
 	GenerateRequestID bool `mapstructure:"generate_request_id"`
+	// Integration Type value lives in config so that it can be updated from the request, and then passed for the event URL
+	IntegrationType string


I'm not sure I understand why this field is in the config. It seems like the integration type can only be defined in the account config as a default for that account or in the request. What am I missing?

It's because I was having trouble getting the integration information to update the vTrack URL integration parameter to the cachePutObjects function. The cachePutObjects function, which eventually calls EventRequestToUrl, has the config object as a member of its function, so by adding the IntegrationType value to the config object, it allowed me to then access it to update the vtrack URL.

This was confusing for me to type, so it might be confusing for you to read, so we can also talk about it tomorrow on a call if you'd like to see if there's an easier way for me to get the integration type value to the vtrack URL.

Let's chat about this as a group tomorrow. I need to brush up on how the vTrack endpoint works. From looking at the diagrams here #1015 (comment) it looks like prebid.js calls the vtrack endpoint. I was trying to piece this together so I took a look at PBS-Java and it looks like they are looking for the integration under the int query parameter in the vtrack handler, however, that is missing from their documentation on the prebid.org site. @mansinahar do you have any insight on this?

bsardo · 2022-01-05T17:54:50Z

config/config.go

@@ -970,6 +972,7 @@ func SetupViper(v *viper.Viper, filename string) {
 	v.SetDefault("auto_gen_source_tid", true)
 	v.SetDefault("generate_bid_id", false)
 	v.SetDefault("generate_request_id", false)
+	v.SetDefault("account_defaults.default_integration", "") // Default Integration For Accounts Can Be Set Here


I think you can remove this comment.

bsardo · 2022-01-06T16:22:34Z

openrtb_ext/request.go

@@ -32,6 +32,7 @@ type ExtRequestPrebid struct {
 	Channel              *ExtRequestPrebidChannel  `json:"channel,omitempty"`
 	Data                 *ExtRequestPrebidData     `json:"data,omitempty"`
 	Debug                bool                      `json:"debug,omitempty"`
+	Integration          string                    `json:"integration,omitempty"`


If a request contains .ext.prebid, integration will always be set given that this is a string and not a pointer to a string. In that case, if integration is not specified it will be an empty string. Is the plan to just check for an empty string when determining whether to apply the default integration value? Is there a use case where a publisher does not want to set an integration type, in which case they would explicitly set it to empty string? If so, then a pointer to a string would be needed as we would need to detect presence to differentiate between not set and set to empty string.

This is a good question to bring up in tomorrow's standup, but the current implementation I have is if integration is blank then we apply the default integration value.

However, the answer for how a publisher could omit an integration type in general is unclear. They could just set their default integration value to be blank, but I don't know if that approaches is what we want.

@mansinahar thoughts on this? Do we need to confirm this behavior with Bret? I did a little digging and it appears this implementation matches what's in PBS-Java and the edge case I described above is unaccounted for and perhaps not valid. In Ortb2RequestFactory.java, the StringUtils.isBlank(integration) call checks that the request integration is null or empty string before applying the default integration.

@bsardo Yeah I think this should be fine. I don't see a specific use case for a publisher to be able to not specify any integration at all being mentioned in the issue description #1428. And as you mentioned it matches with the Java implementation as well.

endpoints/openrtb2/auction.go

bsardo · 2022-01-06T16:30:34Z

endpoints/openrtb2/auction.go

+	if reqPrebid == nil {
+		reqPrebid = &openrtb_ext.ExtRequestPrebid{Integration: deps.cfg.AccountDefaults.DefaultIntegration}
+		reqExt.SetPrebid(reqPrebid)
+		req.RebuildRequest()


Does it make sense to rebuild the request here? This function is called from parseRequest which is called from Auction and at the end of Auction we rebuild the request.

I'll look into this, but I think you're right!

bsardo · 2022-01-06T16:34:01Z

endpoints/openrtb2/auction_test.go

+			expectedIntegrationType: "TestIntegrationType",
+		},
+		{
+			description: "Request doesn't have request.ext.prebid path, expect default integration value",


Can we add a test case for when prebid is defined but integration is not?

bsardo · 2022-01-06T16:39:37Z

endpoints/events/event.go

+	TimestampParameter       = "ts"
+	FormatParameter          = "f"
+	AnalyticsParameter       = "x"
+	IntegrationTypeParameter = "int"


Just checking, is this a standardized query parameter agreed upon with the PBS committee?

Yes I believe so, I'll double check with Mansi, but I believe this is correct

I see that this is the name of the parameter in PBS-Java.

bsardo · 2022-01-11T20:32:33Z

endpoints/openrtb2/auction.go

+	if reqPrebid == nil {
+		reqPrebid = &openrtb_ext.ExtRequestPrebid{Integration: deps.cfg.AccountDefaults.DefaultIntegration}
+		reqExt.SetPrebid(reqPrebid)
+	} else if reqPrebid.Integration == "" {
+		reqPrebid.Integration = deps.cfg.AccountDefaults.DefaultIntegration
+		reqExt.SetPrebid(reqPrebid)
+	}


Super nitpick - IMO this is a slight improvement that makes it a bit easier to reason:

if reqPrebid == nil { reqPrebid = &openrtb_ext.ExtRequestPrebid{} } if reqPrebid.Integration == "" { reqPrebid.Integration = deps.cfg.AccountDefaults.DefaultIntegration reqExt.SetPrebid(reqPrebid) }

bsardo · 2022-01-12T15:30:30Z

config/config.go

@@ -88,6 +88,8 @@ type Configuration struct {
 	GenerateBidID bool `mapstructure:"generate_bid_id"`
 	// GenerateRequestID overrides the bidrequest.id in an AMP Request or an App Stored Request with a generated UUID if set to true. The default is false.
 	GenerateRequestID bool `mapstructure:"generate_request_id"`
+	// Integration Type value lives in config so that it can be updated from the request, and then passed for the event URL
+	IntegrationType string


Let's chat about this as a group tomorrow. I need to brush up on how the vTrack endpoint works. From looking at the diagrams here #1015 (comment) it looks like prebid.js calls the vtrack endpoint. I was trying to piece this together so I took a look at PBS-Java and it looks like they are looking for the integration under the int query parameter in the vtrack handler, however, that is missing from their documentation on the prebid.org site. @mansinahar do you have any insight on this?

endpoints/openrtb2/auction.go

mansinahar · 2022-01-25T13:45:04Z

endpoints/openrtb2/auction.go

+	return nil
+}
+
+func getIntegrationFromRequest(req *openrtb_ext.RequestWrapper) (string, error) {


Looks like this function is only used in a test.. Do we still need this?

Yeah I wrote the function specifically for tests, I'll move the function to the test file!

mansinahar · 2022-01-25T14:36:38Z

endpoints/events/vtrack.go

@@ -268,16 +273,20 @@ func getAccountId(httpRequest *http.Request) string {
 	return httpRequest.URL.Query().Get(AccountParameter)
 }

+func getIntegrationType(httpRequest *http.Request) string {
+	return httpRequest.URL.Query().Get(IntegrationParameter)


We might wanna add some validation for integration param like Java does here: https://github.com/prebid/prebid-server-java/pull/869/files#diff-fb5e209319e349f196eaf16511350dae8972f0791238207c17353aa417e0e12bR98

mansinahar · 2022-01-25T14:54:12Z

Can we please add some JSON tests for these changes similar to the event tests in: https://github.com/prebid/prebid-server/tree/master/exchange/exchangetest.

Also, shouldn't the event URLs in these existing JSON tests should now have the integration query param in them too? Like here for example: https://github.com/prebid/prebid-server/blob/master/exchange/exchangetest/events-vast-account-off-request-on.json#L124 or here: https://github.com/prebid/prebid-server/blob/master/exchange/exchangetest/events-bid-account-on-request-off.json#L76

endpoints/openrtb2/auction.go

bsardo · 2022-02-03T15:07:05Z

endpoints/openrtb2/auction.go

@@ -1836,21 +1838,37 @@ func (deps *endpointDeps) setIntegrationType(req *openrtb_ext.RequestWrapper, ac
 		return err
 	}
 	reqPrebid := reqExt.GetPrebid()
-	if reqPrebid == nil {
+	if reqPrebid == nil && account != nil {
+		err := validateIntegrationType(account.DefaultIntegration)


Do we think this is where we want to validate the default integration? I'd be more inclined to validate it when we look up the account in account/account.go#GetAccount.

Discussed offline with the team and we will only validate on the event and vtrack endpoints to be consistent with Java so this can be disregarded.

bsardo · 2022-02-03T15:25:40Z

endpoints/openrtb2/auction.go

@@ -1836,21 +1838,37 @@ func (deps *endpointDeps) setIntegrationType(req *openrtb_ext.RequestWrapper, ac
 		return err
 	}
 	reqPrebid := reqExt.GetPrebid()
-	if reqPrebid == nil {
+	if reqPrebid == nil && account != nil {
+		err := validateIntegrationType(account.DefaultIntegration)


Also, it looks like we're considering an invalid default integration value an error that kills the auction since the calling code uses the error to determine whether to early return before calling HoldAuction. I just want to confirm that this is what we want. I imagine so since it will result in a 400 informing the user of the issue. The alternative would be to dynamically set it to empty string since the value is not required and does not appear to be critical to the auction but that would happen silently.

N/A since integration type validation will be removed from the auction endpoint.

bsardo · 2022-02-03T15:31:53Z

endpoints/openrtb2/auction.go

 		reqPrebid.Integration = account.DefaultIntegration
 		reqExt.SetPrebid(reqPrebid)
+	} else {
+		err := validateIntegrationType(reqPrebid.Integration)


I think we should move the validation of this request field to where we validate all other request fields in endpoints/openrtb2/auction.go#validateRequest.

Moving this validation and the default integration validation will also greatly simplify this logic so it's more like what was described here: #2111 (comment)

Discussed offline with the team and we will only validate on the event and vtrack endpoints to be consistent with Java so this can be disregarded.

bsardo · 2022-02-08T15:29:39Z

endpoints/events/event.go

+		return errors.New("integration type length is too long")
+	}
+	for _, char := range integrationType {
+		if !(unicode.IsDigit(char) || unicode.IsLetter(char)) && char != '-' && char != '_' {


Nitpick: this might be easier to read if you used all not/and operators: !unicode.IsDigit(char) && !unicode.IsLetter(char) && ...

bsardo · 2022-02-08T15:32:16Z

endpoints/openrtb2/auction.go

+		return err
+	}
+	reqPrebid := reqExt.GetPrebid()
+	if reqPrebid == nil && account != nil {


I think we need to check if the account is nil for both the if and elseif sections, in which case I suggest an early return before you execute the if-else if:

if account == nil { return nil }

You can then remove the account != nil check here which will effectively guarantee reqPrebid is defined when you try to access it as part of the else if statement (as currently written you could get an error here when reqPrebid and account are both nil).

bsardo · 2022-02-08T17:20:10Z

endpoints/events/event_test.go

+				Type:        analytics.Imp,
+				BidID:       "bidid",
+				AccountID:   "accountId",
+				Bidder:      "bidder",
+				Timestamp:   1234567,
+				Format:      analytics.Blank,
+				Analytics:   analytics.Enabled,


You can simplify all of the givenEventRequest objects defined in these test cases by just defining an empty object since none of these values are relevant to the tests and Integration can just take on the default empty string value.

bsardo · 2022-02-08T17:36:08Z

endpoints/events/vtrack.go

+	if err != nil {
+		return "", err
+	}


Can you add test coverage for when the validate function returns an error here?

bsardo · 2022-02-08T21:18:25Z

endpoints/events/event.go

@@ -162,6 +166,10 @@ func ParseEventRequest(r *http.Request) (*analytics.EventRequest, []error) {
 		errs = append(errs, err)
 	}

+	if err := readIntegrationType(event, r); err != nil {
+		errs = append(errs, err)


Can you add coverage for this line? You should be able to just add another test which is basically a copy of TestShouldReturnBadRequestWhenFormatValueIsInvalid.

bsardo

LGTM

mansinahar · 2022-02-09T13:59:12Z

endpoints/events/event_test.go

+	for _, test := range testCases {
+		testEventRequest := &analytics.EventRequest{}
+		err := readIntegrationType(testEventRequest, test.givenHttpRequest)
+		if err != nil {


The if condition here should be if test.expectedError != nil because you wanna check for an error if you expect one in the test case. Consider a test case where you expect an error but don't get back one, the execution here will fall to the else block which asserts that the error is empty which it will be because your else block only executes when error is empty/nil. This will result to a false positive.

mansinahar · 2022-02-09T14:00:28Z

endpoints/events/vtrack.go

+	// get integration value from request parameter
+	integrationType, err := getIntegrationType(r)
+	if err != nil {
+		w.Write([]byte(fmt.Sprintf("Invalid integration type: %s\n", err.Error())))


Please add the http.StatusBadRequest response code when returning an error here. Look at L85 for reference

mansinahar · 2022-02-09T14:02:25Z

endpoints/events/vtrack_test.go

+
+	for _, test := range testCases {
+		integrationType, err := getIntegrationType(test.givenHttpRequest)
+		if err != nil {


Same comment as above for the readIntegrationType function test

endpoints/openrtb2/auction.go

mansinahar · 2022-02-09T14:12:20Z

LGTM in general other than the minor comments above.

bsardo

LGTM

Integration Type Handling

59b03c8

AlexBVolcy changed the title ~~Integration Type Integration~~ Integration Type Implementation Dec 14, 2021

hhhjort requested review from bsardo and mansinahar December 14, 2021 18:24

hhhjort assigned bsardo and mansinahar Dec 14, 2021

bsardo reviewed Jan 6, 2022

View reviewed changes

AlexBVolcy added 2 commits January 10, 2022 17:29

Merge branch 'prebid:master' into New_Integration_Type

8c78d56

Addressing comments, fixed tests

71f7ff5

bsardo reviewed Jan 12, 2022

View reviewed changes

Updated vtrack URL integration, altered tests, refactoring

1e62728

mansinahar reviewed Jan 25, 2022

View reviewed changes

bsardo reviewed Jan 25, 2022

View reviewed changes

endpoints/openrtb2/auction.go Show resolved Hide resolved

AlexBVolcy added 2 commits January 25, 2022 14:25

Added integration type validation, minor refactoring

5bbabae

Updated JSON exchange tests, updated event URL to apply integration type

75c980e

bsardo reviewed Feb 3, 2022

View reviewed changes

AlexBVolcy added 3 commits February 7, 2022 09:51

Added integration validation to event/vtrack URLs

c12acb3

Merge branch 'master' into New_Integration_Type

25b03fc

Added more tests for integration validation

f12d6f4

bsardo reviewed Feb 8, 2022

View reviewed changes

Address minor feedback

afa43b1

bsardo reviewed Feb 8, 2022

View reviewed changes

Added test coverage for readIntegrationType error

a609124

bsardo previously approved these changes Feb 8, 2022

View reviewed changes

mansinahar reviewed Feb 9, 2022

View reviewed changes

Minor feedback updates

e7f1c64

AlexBVolcy dismissed bsardo’s stale review via e7f1c64 February 10, 2022 00:33

mansinahar approved these changes Feb 10, 2022

View reviewed changes

bsardo approved these changes Feb 10, 2022

View reviewed changes

mansinahar merged commit 674f096 into prebid:master Feb 14, 2022

pm-nilesh-chate added a commit to PubMatic-OpenWrap/prebid-server that referenced this pull request Mar 30, 2022

Integration Type Implementation (prebid#2111)

1661a45

AlexBVolcy mentioned this pull request Jul 12, 2022

Integration Type and Channel #1428

Closed

jorgeluisrocha pushed a commit to jwplayer/prebid-server that referenced this pull request Sep 28, 2022

Integration Type Implementation (prebid#2111)

5bab2c0

shunj-nb pushed a commit to ParticleMedia/prebid-server that referenced this pull request Nov 8, 2022

Integration Type Implementation (prebid#2111)

07ab459

Integration Type Implementation #2111

Integration Type Implementation #2111

Conversation

AlexBVolcy commented Dec 14, 2021

Choose a reason for hiding this comment

AlexBVolcy Jan 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsardo Jan 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mansinahar commented Jan 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsardo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mansinahar commented Feb 9, 2022

bsardo left a comment

Choose a reason for hiding this comment

AlexBVolcy Jan 10, 2022 •

edited

Loading

bsardo Jan 11, 2022 •

edited

Loading

mansinahar commented Jan 25, 2022 •

edited

Loading