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

Add GZIP Compression #2347

Merged
merged 9 commits into from
Aug 24, 2022
Merged

Conversation

AlexBVolcy
Copy link
Contributor

@AlexBVolcy AlexBVolcy commented Aug 9, 2022

This PR addresses this issue here: #1812

The goal of the PR is to allow bidders to GZIP compress the request data this being utilized to create http requests that are being generated in by doRequestImpl()

To accomplish this, I've added a new string variable EndpointCompression, which can be set to tell PBS what type of compression a bidder may want (i.e. EndpointCompression = "GZIP").

This EndpointCompression object has been added as a member of the BidderInfo struct so that this compression information can be passed to AdaptBidder().

In AdaptBidder(), the EndpointCompression is then added to a bidderAdapterConfig object, so that we can access the compression info in requestBid().

Once at requestBid(), doRequest() and subsequently doRequestImpl() is called with the bidderAdapterConfig info present, so it's here we check if EndpointCompression == "GZIP", and if so, we compress the request data before calling http.NewRequest().

A single unit test was written for this update, and may require manual end-to-end testing given the nature of the feature requiring compressed data to be created and verified.

@AlexBVolcy AlexBVolcy changed the title Add GZIP Compression for Bidders Add GZIP Compression Aug 10, 2022
w := gzip.NewWriter(&b)
w.Write([]byte(req.Body))
w.Close()
requestBody = b.Bytes()
Copy link
Contributor

Choose a reason for hiding this comment

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

Lines 514-518 look like a good candidates to be extracted to a separate function.
requestBody = compressToGZIP(req.Body) (or better name for it :))

}

type CompressionType struct {
EndpointCompression string `yaml:"endpointCompression"`

Choose a reason for hiding this comment

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

Would it be better to be more specific here so that it is understood that this compression is for outgoing requests only?

There is already confusion around how Go handles GZIP in it's default httpclient.

My thoughts are that if we're more specific here the config will be better understood and easier to reason with.

Something like RequestCompressionType? Or something along those lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand your reasoning, but we are trying to be consistent with variable/object naming between PBS-Go and PBS-Java, so I think to accomplish this this object can stay being named this way. I'll see if other team members think differently though!


switch bidder.config.EndpointCompression {
case "GZIP":
var b bytes.Buffer
Copy link
Contributor

Choose a reason for hiding this comment

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

In config "GZIP" can be also specified as "gzip" or even "gZip". This case will never work.
Please replace switch statement to switch strings.ToUpper(bidder.config.EndpointCompression) {.
Also I recommend to extract "GZIP" to a constant.

@VeronikaSolovei9
Copy link
Contributor

VeronikaSolovei9 commented Aug 12, 2022

For some reason it shows weird difference in bidder_test.go file.
For line 275 where you compare debug http calls, can you add a verification if debug http calls are present, check the Content-Encoding header is present as well and it has gzip value in it? I understand it's hard to test, with this check you may ensure that Switch case worked.
Also please add a check that request body in debug http calls is not gziped. Basically just assert request body equals to {"key":"val"}

Line 189 Body: []byte("{\"key\":\"val\"}"), modify to

Body: []byte(`{"key":"val"}`),

@VeronikaSolovei9
Copy link
Contributor

VeronikaSolovei9 commented Aug 12, 2022

Code and local testing looks good to me, added some minor comments

Copy link
Contributor

@VeronikaSolovei9 VeronikaSolovei9 left a comment

Choose a reason for hiding this comment

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

Thank you for addressing the comments, LGTM

Still would be good if you add unit test checks I described in above comment :)

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.

Can we add an additional TestReadFullConfig() test case inside config/bidderinfo_test.go? Something similar to what we already have in config/config_test.go but with a hardcoded sample YAML file instead of a hardcoded JSON, and it comes with all the fields supported by the BidderInfo struct:

11  
12   const testInfoFilesPath = "./test/bidder-info"
13   const testSimpleYAML = `
14   maintainer:
15     email: "some-email@domain.com"
16   gvlVendorID: 42
17   `
   + const fullBidderYAMLConfig = `
   + maintainer:
   +   email: "some-email@domain.com"
   + capabilities:
   +   app:
   +     mediaTypes: 
   +       - banner
   +   site:
   +     mediaTypes:
   +       - video
   +       - native
   + modifyingVastXmlAllowed: true
   + debug:
   +   allow: true
   + gvlVendorID: 42
   + userSync:
   +   iframe:
   +     url: "someURL"
   +     userMacro: "aValue"
   +   redirect:
   +     url: anotherURL
   +     userMacro: anotherValue
   +   key: "akey"
   +   supports:
   +     - item
   +     - item2
   +   external_url: oneMoreUrl
   +   support_cors: true
   + experiment:
   +   adsCert:
   +     enabled: true
   + compressionType: GZIP
   + `
   +
   + func TestReadFullYamlBidderConfig(t *testing.T) {
   +     // Read the fullBidderYAMLConfig
   +     expected := BidderInfos{
   +     	bidder: {
   +     		Enabled: true,
   +     		Maintainer: &MaintainerInfo{
             . 
             . 
             . 
   +            CompressionType: "GZIP",
   +        },
   +     }
   +     // Read the fullBidderYAMLConfig
   +     // assert
   + }
18  
config/bidderinfo_test.go

}

type CompressionType struct {
EndpointCompression string `yaml:"endpointCompression"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a small description for CompressionType? Please feel free to reword as you see fit.

27 + // CompressionType if set, determines the type of compression the bid request will undergo before being sent to the corresponding bid server
28   type CompressionType struct {
29       EndpointCompression string `yaml:"endpointCompression"`
30   }
31  

guscarreon
guscarreon previously approved these changes Aug 17, 2022
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.

LGTM

guscarreon
guscarreon previously approved these changes Aug 17, 2022
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.

LGTM

@AlexBVolcy
Copy link
Contributor Author

Please don't merge this PR, until I'm done with my end to end manual testing!

@AlexBVolcy
Copy link
Contributor Author

Manual end to end testing is completed! This is ready for a final review/merge -- I just changed the naming of one yaml variable

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.

LGTM

@SyntaxNode SyntaxNode merged commit 76abaa9 into prebid:master Aug 24, 2022
pm-aadit-patil pushed a commit to PubMatic-OpenWrap/prebid-server that referenced this pull request Sep 22, 2022
pm-aadit-patil pushed a commit to PubMatic-OpenWrap/prebid-server that referenced this pull request Sep 22, 2022
pm-aadit-patil pushed a commit to PubMatic-OpenWrap/prebid-server that referenced this pull request Sep 22, 2022
jorgeluisrocha pushed a commit to jwplayer/prebid-server that referenced this pull request Sep 28, 2022
shunj-nb pushed a commit to ParticleMedia/prebid-server that referenced this pull request Nov 8, 2022
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.

None yet

5 participants