New Adapter: Trustedstack #3618
Conversation
Code coverage summaryNote:
trustedstackRefer here for heat map coverage report |
|
|
||
| // Builder builds a new instance of the Trustedstack adapter for the given bidder with the given config. | ||
| func Builder(bidderName openrtb_ext.BidderName, config config.Adapter, server config.Server) (adapters.Bidder, error) { | ||
| url := buildEndpoint(config.Endpoint, config.ExtraAdapterInfo) |
There was a problem hiding this comment.
Why not use server.ExternalUrl instead of using the extra info field?
product-trustedstack
force-pushed
the
trustedstack-adapter
branch
from
b4a6ab3
to
a8844e6
Compare
Code coverage summaryNote:
trustedstackRefer here for heat map coverage report |
product-trustedstack
force-pushed
the
trustedstack-adapter
branch
from
a8844e6
to
b239786
Compare
Code coverage summaryNote:
trustedstackRefer here for heat map coverage report |
Code coverage summaryNote:
trustedstackRefer here for heat map coverage report |
product-trustedstack
force-pushed
the
trustedstack-adapter
branch
from
f34f503
to
5c3111b
Compare
Code coverage summaryNote:
trustedstackRefer here for heat map coverage report |
| mediaType, err := getBidMediaTypeFromMtype(bid) | ||
| if err == nil { | ||
| return mediaType, nil | ||
| } | ||
| mediaType = openrtb_ext.BidTypeBanner | ||
| for _, imp := range imps { | ||
| if imp.ID == bid.ImpID { | ||
| switch { |
There was a problem hiding this comment.
@product-trustedstack any particular reason to fall back to imp.ID comparission when getBidMediaTypeFromMtype
Additionally,
- err != nil case is being ignored for
getBidMediaTypeFromMtype - By default setting media type to
banneron line 90 seems incorrect. Better to add a switch case for banner type as well. This ensuresbannermedia type is used only whenimp.Banneris not nil
There was a problem hiding this comment.
The fall back was for mtype not correctly present in the response. That is the case for if err != nil, if mtype is wrong in the response, then impID can be used to fetch the correct mediaType.
Removed the fallbak to Impression Id. Updated the changes for default handling to banner and added it to the switch case.
| switch bid.MType { | ||
| case openrtb2.MarkupBanner: | ||
| return openrtb_ext.BidTypeBanner, nil | ||
| case openrtb2.MarkupVideo: | ||
| return openrtb_ext.BidTypeVideo, nil | ||
| case openrtb2.MarkupAudio: | ||
| return openrtb_ext.BidTypeAudio, nil | ||
| case openrtb2.MarkupNative: | ||
| return openrtb_ext.BidTypeNative, nil | ||
| default: | ||
| return "", fmt.Errorf("unable to fetch mediaType for imp: %s", bid.ImpID) |
There was a problem hiding this comment.
static/bidder-info/trustedstack.yaml doesnot define audio as supported media type.
Should make changes in adapter code to only support media types defined in bidder info yaml or update bidder info yaml to define audio as supported media type as well.
capabilities:
app:
mediaTypes:
- banner
- video
- native
site:
mediaTypes:
- banner
- video
- native
There was a problem hiding this comment.
According to the implementation of request to adapter, mediaType which adapters are not supported, request is not made for that impression and thus not sent to the bidder.
Shall bidder response parsing however can support all the media types and that can be utilised when the capabilities are updated?
There was a problem hiding this comment.
Shall bidder response parsing however can support all the media types and that can be utilised when the capabilities are updated?
@product-trustedstack for now adapter code should only check for media types supported. Code can be updated when capabilities includes new media type
| case imp.Banner == nil && imp.Video == nil && imp.Audio != nil && imp.Native == nil: | ||
| mediaType = openrtb_ext.BidTypeAudio |
There was a problem hiding this comment.
refer https://github.com/prebid/prebid-server/pull/3618/files#r1570529242
static/bidder-info/trustedstack.yaml doesnot define audio as supported media type.
Should make changes in adapter code to only support media types defined in bidder info yaml or update bidder info yaml to define audio as supported media type as well.
capabilities:
app:
mediaTypes:
- banner
- video
- native
site:
mediaTypes:
- banner
- video
- native
There was a problem hiding this comment.
According to the implementation of request to adapter, mediaType which adapters are not supported, request is not made for that impression and thus not sent to the bidder.
Shall bidder response parsing however can support all the media types and that can be utilised when the capabilities are updated?
There was a problem hiding this comment.
Shall bidder response parsing however can support all the media types and that can be utilised when the capabilities are updated?
@product-trustedstack for now adapter code should only check for media types supported. Code can be updated when capabilities includes new media type
There was a problem hiding this comment.
This was the fallback logic as discussed in the comment #3618 (comment).
This has been removed. The core logic to get the mtype from the field above has been updated to only support media types currently mentioned in the config.
|
@product-trustedstack requesting to merge prebid-server/adapters/pubmatic/pubmatic.go Lines 175 to 182 in 7635519 |
@onkarvhanumante this is done. |
…ent of banner mediaType.
Code coverage summaryNote:
trustedstackRefer here for heat map coverage report |
Code coverage summaryNote:
trustedstackRefer here for heat map coverage report |
Code coverage summaryNote:
trustedstackRefer here for heat map coverage report |
| }, nil | ||
| } | ||
|
|
||
| func getMediaTypeForImp(bid *openrtb2.Bid, imps []openrtb2.Imp) (openrtb_ext.BidType, error) { |
There was a problem hiding this comment.
imps are not used in the function. We can keep the definition clean by removing the unused parameters.
There was a problem hiding this comment.
Also, this function is not required. getBidMediaTypeFromMtype can be called directly from MakeBids function as follow:-
bidType, err := getBidMediaTypeFromMtype(&sb.Bid[i])
Code coverage summaryNote:
trustedstackRefer here for heat map coverage report |
Trustedstack server adapter added.
Docs - https://github.com/prebid/prebid.github.io/blob/master/dev-docs/bidders/trustedstack.md