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

33Across: Add video support in adapter #1557

Merged
merged 6 commits into from
Nov 11, 2020
Merged

Conversation

curlyblueeagle
Copy link
Contributor

No description provided.

@SyntaxNode
Copy link
Contributor

@curlyblueeagle Are you looking to also add support for Long Form Video / Instream Video with this PR?

guscarreon
guscarreon previously approved these changes Nov 4, 2020
Copy link
Contributor

@guscarreon guscarreon left a comment

Choose a reason for hiding this comment

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

Looks pretty good, just a question in regards of the JSON files. Also, just for consistency in respect of other adapters, can we rename adapters/33across/33across/ folder to adapters/33across/33acrosstest/ please?

"ext": {
"ttx": {
"mediaType": "video"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we only expect the 33Across server to respond with an "ext" field to video requests? Because in the banner example found in adapters/33across/33across/exemplary/simple-banner.json the "mockResponse" doesn't have that. If they do, should we expect a "banner" value?

45       "mockResponse": {
46         "status": 200,
47         "body": {
48           "id": "test-request-id",
49           "seatbid": [
50             {
51               "seat": "ttx",
52               "bid": [{
53                 "id": "8ee514f1-b2b8-4abb-89fd-084437d1e800",
54                 "impid": "test-imp-id",
55                 "price": 0.500000,
56                 "adm": "some-test-ad",
57                 "crid": "crid_10",
58                 "h": 90,
59                 "w": 728
   +               "ext": {
   +                 "ttx": {
   +                   "mediaType": "video"
   +                 }
   +               }
60               }]
61             }
62           ],
63           "cur": "USD"
64         }
65       }
66     }
67   ],
adapters/33across/33across/exemplary/simple-banner.json

Copy link
Contributor Author

@curlyblueeagle curlyblueeagle Nov 4, 2020

Choose a reason for hiding this comment

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

Correct, the ext field if present will specify the mediaType. If not present assume banner, which is what the adapter will do in such a case.

@guscarreon guscarreon dismissed their stale review November 4, 2020 03:00

didn't mean to approve, I meant to request changes

Copy link
Contributor

@guscarreon guscarreon left a comment

Choose a reason for hiding this comment

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

Looks pretty good, just a question in regards of the JSON files. Also, just for consistency in respect of other adapters, can we rename adapters/33across/33across/ folder to adapters/33across/33acrosstest/ please?

@curlyblueeagle
Copy link
Contributor Author

@SyntaxNode Yes we are looking to support instream. The adapter will treat it the same as outstream except for validating placement and startdelay values.

@SyntaxNode
Copy link
Contributor

@SyntaxNode Yes we are looking to support instream. The adapter will treat it the same as outstream except for validating placement and startdelay values.

Gotcha. Sounds good. The /openrtb2/video endpoint requires certain fields on the bid be populated to work properly. We have those requirements documented here: https://docs.prebid.org/prebid-server/developers/add-new-bidder-go.html#long-form-video-support

@curlyblueeagle
Copy link
Contributor Author

@SyntaxNode Yes we are looking to support instream. The adapter will treat it the same as outstream except for validating placement and startdelay values.

Gotcha. Sounds good. The /openrtb2/video endpoint requires certain fields on the bid be populated to work properly. We have those requirements documented here: https://docs.prebid.org/prebid-server/developers/add-new-bidder-go.html#long-form-video-support

Oh interesting. We were assuming that Long Form was Ad Pods which is distinct from instream. Is that not the case? We are not looking to support Ad Pods. Just instream video.

@SyntaxNode
Copy link
Contributor

Oh interesting. We were assuming that Long Form was Ad Pods which is distinct from instream. Is that not the case? We are not looking to support Ad Pods. Just instream video.

Ah, ok. You're fine. I was wrongfully assuming all instream video was ad pod based. I'll update our docs to make that clearer.

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.

Code looks good. I commented on a few areas where we'd like to see increased test coverage.

adapters/33across/33across.go Show resolved Hide resolved
videoCopy.Placement = 1

if videoCopy.StartDelay == nil {
videoCopy.StartDelay = openrtb.StartDelay.Ptr(0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add test coverage for this default to 0 branch.

return openrtb_ext.BidTypeVideo
}

return openrtb_ext.BidTypeBanner
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add test coverage for this assume banner if not a video mediatype branch.

@curlyblueeagle
Copy link
Contributor Author

Oh interesting. We were assuming that Long Form was Ad Pods which is distinct from instream. Is that not the case? We are not looking to support Ad Pods. Just instream video.

Ah, ok. You're fine. I was wrongfully assuming all instream video was ad pod based. I'll update our docs to make that clearer.

I asked this question earlier to the Prebid team :). This was the answer I received #1487 (comment)

So yes, we will be doing on only "Short form" instream and not Ad Pod (aka Long form)

@curlyblueeagle
Copy link
Contributor Author

Code looks good. I commented on a few areas where we'd like to see increased test coverage.

@SyntaxNode I have added more tests to pass the threshold of 90% (sorry only just came to know about this requirement). Please let me know if you need anything more.

Copy link
Contributor

@guscarreon guscarreon left a comment

Choose a reason for hiding this comment

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

I believe you forgot to get rid of the adapters/33across/33across and the adapters/33across/33across/supplemental directories that now seem to be empty

╰─➤  tree adapters/33across
adapters/33across
├── 33across
│   └── supplemental
├── 33across.go
├── 33across_test.go
├── 33acrosstest
│   ├── exemplary
│   │   ├── bidresponse-defaults.json
│   │   ├── instream-video-defaults.json
│   │   ├── multi-format.json
│   │   ├── optional-params.json
│   │   ├── outstream-video-defaults.json
│   │   ├── simple-banner.json
│   │   └── simple-video.json
│   ├── params
│   │   └── race
│   │       ├── banner.json
│   │       └── video.json
│   └── supplemental
│       ├── status-not-ok.json
│       └── video-validation-fail.json
├── params_test.go
├── usersync.go
└── usersync_test.go

@curlyblueeagle
Copy link
Contributor Author

I believe you forgot to get rid of the adapters/33across/33across and the adapters/33across/33across/supplemental directories that now seem to be empty

╰─➤  tree adapters/33across
adapters/33across
├── 33across
│   └── supplemental
├── 33across.go
├── 33across_test.go
├── 33acrosstest
│   ├── exemplary
│   │   ├── bidresponse-defaults.json
│   │   ├── instream-video-defaults.json
│   │   ├── multi-format.json
│   │   ├── optional-params.json
│   │   ├── outstream-video-defaults.json
│   │   ├── simple-banner.json
│   │   └── simple-video.json
│   ├── params
│   │   └── race
│   │       ├── banner.json
│   │       └── video.json
│   └── supplemental
│       ├── status-not-ok.json
│       └── video-validation-fail.json
├── params_test.go
├── usersync.go
└── usersync_test.go

Hmm not sure where you are seeing this folder structure. Here's what I see in the source branch
Screen Shot 2020-11-09 at 8 59 05 AM

@SyntaxNode
Copy link
Contributor

@guscarreon I just pulled down a fresh copy and cannot see the empty directories. That might be a sync status issue on your machine.

@guscarreon
Copy link
Contributor

guscarreon commented Nov 9, 2020

Hmm not sure where you are seeing this folder structure. Here's what I see in the source branch

You are right, those must have been left-overs in my local folder

adapters/33across/33across.go Show resolved Hide resolved
@@ -49,6 +56,14 @@ func (a *TtxAdapter) makeRequest(request *openrtb.BidRequest) (*adapters.Request
errs = append(errs, err)
}

if reqCopy.Imp[0].Banner == nil && reqCopy.Imp[0].Video == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

reqCopy.Imp[0]

It's unexpected for an adapter to only forward the first impression and throw out the rest. I recognize this is existing behavior which predates me by quite a bit. I don't wish for this to hold up this PR.

At the very least, please document this behavior here: https://github.com/prebid/prebid.github.io/blob/master/dev-docs/bidders/33across.md

As a future improvement, you may wish to consider sending multiple requests to your bidding server for each impression provided by the publisher.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion, will incorporate this in follow up PR. I will also file a Docs PR shortly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SyntaxNode a follow up question about your suggestion to send multiple requests-- how would Prebid Server handle responses? Wait for all responses for each request to return or send them upstream as they come one-by-one?

adapters/33across/33across.go Show resolved Hide resolved
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.

3 participants