Support for express server compression middleware. #2980

revanth0212 · 2021-01-29T16:56:47Z

Description

The express server that serves the app provides a middleware that applies GZIP compression if not applied already on the responses requested by the storefront. Compression is a very important but sensitive feature to manage because it can be applied at different layers of a system. One can apply it at the proxy layer or application layer and overdoing it can cause performance degradation. Hence the express server compression will be controlled by an environment variable. If someone wants to apply compression at the express layer, they have to set the value of ENABLE_EXPRESS_SERVER_COMPRESSION to true. By default, it will be set to false.

ENABLE_EXPRESS_SERVER_COMPRESSION in a sample project's .env file created using the create-pwa scafolding tool:

Related Issue

Closes PWA-1108.

Acceptance

Should apply GZIP compression on responses if ENABLE_EXPRESS_SERVER_COMPRESSION to true.

Verification Stakeholders

@jimbo
@dpatil-magento

Verification Steps

Change/create env variable in the .env file ENABLE_EXPRESS_SERVER_COMPRESSION=true
Run the app. Check both yarn watch:venia and yarn build && yarn stage:venia
Open the app and check the network tab for responses with a header Content-Encoding: gzip.
Change the value of ENABLE_EXPRESS_SERVER_COMPRESSION to false.
Restart the app.
Open the app and check the network tab for responses without a header Content-Encoding: gzip.

Screenshots / Screen Captures (if appropriate)

When ENABLE_EXPRESS_SERVER_COMPRESSION=true

When ENABLE_EXPRESS_SERVER_COMPRESSION=false

Checklist

I have added tests to cover my changes, if necessary.
I have added translations for new strings, if necessary.
I have updated the documentation accordingly, if necessary.

…SION env variable.

PWAStudioBot · 2021-01-29T17:07:06Z

	Messages
📖	Associated JIRA tickets: PWA-1108.
📖	DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).
📖	Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.

Generated by 🚫 dangerJS against 9d252a3

fooman · 2021-01-29T22:44:28Z

This project used shrink-ray-current in the past
https://github.com/magento/pwa-studio/pull/2618/files#diff-2d63256fca103d1f1f8c9cc0a8c4adc56738f8a0397261b1240f824c87ba599e
to provide compression which I guess was favoured as it included brotli compression expressjs/compression#71. Was dropping brotli support on purpose?

tjwiebell · 2021-02-01T16:33:16Z

This project used shrink-ray-current in the past
...
Was dropping brotli support on purpose?

@fooman
Digging through old PRs I can't find the justification from @zetlen, but I recall two big reasons

Ideally a different layer of the application would handle compression, and an Express middleware should be the very last option. We wanted it in place so local development would still give close to production performance scores, but was not intended as a go live solution.
The iltorb Python dependency of shrink-ray was troublesome. Between security alerts and weekly bug reports, the maintenance began to pile up.

This PR ideally solves both of those problems; use a very simple gzip compression middleware and make it opt-in. Paired with this we will also be working on documentation around compression best practices at the web server layer, focusing on AWS/nginx and Magento Cloud (Fastly on Pro/Stage branches, nginx config on Integration branches).

tjwiebell

Logic is sound, I think it just needs moved. Potential to remove unnecessary middleware too. Nice work!

tjwiebell · 2021-02-01T18:12:38Z

packages/upward-js/lib/createUpwardServer.js

+    if (env.ENABLE_EXPRESS_SERVER_COMPRESSION === 'true') {
+        app.use(
+            compression({
+                threshold: 0
+            })
+        );
+        app.use(gzipValidationMiddleware(env));
+    }


We've already implemented inversion of control here with the before prop, so let's just use that. Move this logic to PWADevServer, and follow the same pattern as addImgOptMiddleware. With the environment variable being defined in buildpack, it would also make more sense for buildpack to be the one to use it.

Having it in the PWADevServer will only run this as part of the Dev mode. If someone wants to have compression in other modes/environments as well, that wouldn't be possible with it being in PWADevServer. That's the reason why I have left this in the serve module.

Let me rephrase; we cannot add API to upward-js without also updating the upward-spec and upward-php implementations. If I had confidence UPWARD was going to remain in the stack, this would be an option, but it would likely be better to use upward.yml config as opposed to an environment variable.

Given this, please move the middleware to PWADevServer and utilities/serve in buildpack. Sorry I didn't specify to also put it in the serve utility as well, was just pointing out where to find an example of the IOC pattern.

tjwiebell · 2021-02-01T18:49:59Z

packages/upward-js/lib/gzipValidationMiddleware.js

+
+                if (contentEncoding !== 'gzip') {
+                    log(
+                        '\nGzip compression is supported by the Client. For better performance please enable gzip compression.\n'


I'm not a fan of this getting logged on every request. Oddly enough though, when I switched to this branch compression was enabled, even though I have nothing set in my .env. If this was working as intended, it also doesn't make sense this middleware is only injected alongside compression with that flag set.

I propose removing this middleware entirely, and we focus on adding a blurb to the getting started docs that highlights the real purpose of this middleware.

If you want to run performance metrics on a local development instance, it is suggested that you replicate your production environment as closely as possible to get more accurate measurements. To simulate web server layer compression, you can pass this flag to enable an Express middleware that performs gzip compression of assets.

It should not if you haven't set the ENV variable. Can you check?

Also, I have added a check to not render this warning in a production environment but there is a known bug in the serve module that does not let it work if NODE_ENV is production. I have created a ticket in our backlog to handle this.

Checked again, always compresses with yarn watch:venia, while yarn stage:venia seems to work as intended. Regardless, I would still get rid of the warning. If someone is purposely not using compression or using a different algorithm, there's no reason to annoy them.

packages/venia-concept/package.json

packages/pwa-buildpack/envVarDefinitions.json

revanth0212 · 2021-02-04T15:07:23Z

packages/pwa-buildpack/lib/Utilities/serve.js

            }
        }
    );

    let envPort;
-    if (process.env.PORT) {


This has been a false-positive all along. Since we dint have tests, we never found it. I have added covering tests for this change.

tjwiebell

Suggestion for test improvement but not a blocker to send this to QA. Awesome work covering this with tests too 👌

tjwiebell · 2021-02-04T18:21:47Z

packages/pwa-buildpack/lib/Utilities/__tests__/serve.spec.js

+test('should create upward server', async () => {
+    const server = await serve('pwa-buildpack');
+
+    expect(createUpwardServer.mock.calls).toMatchSnapshot();


This assertion looks fragile; has a lot of cruft in it that I'm not sure we need to check (eg. some devDependency stuff). Can we slim this down?

tjwiebell · 2021-02-04T19:17:07Z

packages/pwa-buildpack/lib/Utilities/serve.js

+     * null and undefined are represented as strings in the env
+     * so we have to match using strings instead.
+     */
+    if (process.env.PORT !== 'null' && process.env.PORT !== 'undefined') {


I was going to propose parseInt here, but I guess 0 is a valid port number. I'm fine with this, but a little type checking wouldn't hurt.

Yeah you are right, 0 is a valid port number.

dpatil-magento · 2021-02-05T15:26:19Z

@revanth0212 Condition update to port validation is failing. When you run yarn stage:venia with STAGING_SERVER_PORT set in .env. Either its ignored or entering if condition before this and ends up running server on random port. Same issue on AWS so PR deploy instance is not accessible.

yarn run v1.22.4
$ yarn venia start
$ yarn workspace @magento/venia-concept start
$ buildpack serve .
  ℹ  PORT is set in environment: undefined
  ℹ  Launching UPWARD server
https://magento-venia-concept-ub-0n.local.pwadev:9536/

…mpression

dpatil-magento · 2021-02-09T15:56:01Z

QA Approved.

tjwiebell · 2021-02-09T16:11:34Z

docker/.env.docker.prod

@@ -16,4 +16,4 @@ UPWARD_JS_LOG_URL=1
 CHECKOUT_BRAINTREE_TOKEN=redacted
 GOOGLE_MAPS_API_KEY=redacted
 MAGENTO_BACKEND_EDITION=EE
-ENABLE_EXPRESS_SERVER_COMPRESSION=false
+ENABLE_EXPRESS_SERVER_COMPRESSION=true


Our official guidance should be to do compression at the web server or proxy layer, not the application layer. Any reason you opted for this config instead of adding compression to nginx?

Dev wanted to test it out and have it enabled on our AWS instances which are not served by Nginx (I presume).

You are right @tjwiebell. Our app is served using nginx but the compression is not enabled.

For instance, this is a PR instance without

And here is the same network req from this PR instance with compression enabled,

@dpatil-magento do you know if we can enable compression at nginx level, if so, how long does it take. Till then this can be a stop-gap solution.

Given this change I'm assuming we just deploy these Docker images directly to EBS, so nginx is involved. It sounds like this isn't as straight forward as it should be with the jwilder/nginx-proxy container, but is possible. Not going to block this PR, but we should consider doing this at the correct layer if our goal is performance.

Where to add config: https://github.com/nginx-proxy/nginx-proxy#proxy-wide
Sample of config needed: https://github.com/h5bp/server-configs-nginx/blob/master/h5bp/web_performance/compression.conf

revanth0212 added 2 commits January 28, 2021 16:33

Added compression middleware controlled by ENABLE_UPWARD_TEXT_COMPRES…

5acf4f5

…SION env variable.

ENABLE_UPWARD_TEXT_COMPRESSION -> ENABLE_EXPRESS_SERVER_COMPRESSION

4253ae7

revanth0212 added the version: Minor This changeset includes functionality added in a backwards compatible manner. label Jan 29, 2021

m2-community-project bot added the Progress: ready for review label Jan 29, 2021

revanth0212 changed the title ~~Revanth/express compression~~ Support for express server compression middleware. Jan 29, 2021

PWAStudioBot added pkg:pwa-buildpack pkg:upward-js Pertains to upward-js reference implementation of UPWARD. pkg:venia-concept labels Jan 29, 2021

Added express middleware to validate gzip compression.

da53fd1

tjwiebell suggested changes Feb 1, 2021

View reviewed changes

m2-community-project bot assigned tjwiebell Feb 1, 2021

m2-community-project bot added Progress: changes requested and removed Progress: ready for review labels Feb 1, 2021

m2-community-project bot added Progress: review and removed Progress: changes requested labels Feb 1, 2021

tjwiebell reviewed Feb 1, 2021

View reviewed changes

packages/pwa-buildpack/envVarDefinitions.json Outdated Show resolved Hide resolved

revanth0212 added 5 commits February 2, 2021 12:33

Removing Gzip validation middleware.

dbb0917

Minor.

0a9e08b

Moving the compression logic to buildpack.

58f3608

Updated PWADevServer tests.

efbad80

Added serve module tests.

18ccb81

revanth0212 commented Feb 4, 2021

View reviewed changes

tjwiebell previously approved these changes Feb 4, 2021

View reviewed changes

m2-community-project bot added Progress: review and removed Progress: approved labels Feb 4, 2021

tjwiebell previously approved these changes Feb 4, 2021

View reviewed changes

m2-community-project bot added Progress: approved and removed Progress: review labels Feb 4, 2021

Minor.

c9c7408

revanth0212 dismissed tjwiebell’s stale review via c9c7408 February 5, 2021 14:07

revanth0212 and others added 5 commits February 5, 2021 11:08

Checking for undefined PORT.

e0bc4e3

Minor

429195f

Merge remote-tracking branch 'origin/develop' into revanth/express_co…

389e81a

…mpression

Enabling compression in prod mode.

36fd369

Merge branch 'develop' into revanth/express_compression

ba4da6b

tjwiebell suggested changes Feb 9, 2021

View reviewed changes

m2-community-project bot added Progress: review and removed Progress: approved labels Feb 9, 2021

tjwiebell approved these changes Feb 9, 2021

View reviewed changes

m2-community-project bot added Progress: approved and removed Progress: review labels Feb 9, 2021

Merge branch 'develop' into revanth/express_compression

9d252a3

dpatil-magento merged commit 5e38b33 into develop Feb 11, 2021

m2-community-project bot added Progress: done and removed Progress: approved labels Feb 11, 2021

dpatil-magento deleted the revanth/express_compression branch February 11, 2021 16:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for express server compression middleware. #2980

Support for express server compression middleware. #2980

revanth0212 commented Jan 29, 2021

PWAStudioBot commented Jan 29, 2021 •

edited

Loading

fooman commented Jan 29, 2021

tjwiebell commented Feb 1, 2021

tjwiebell left a comment

tjwiebell Feb 1, 2021

revanth0212 Feb 2, 2021

tjwiebell Feb 2, 2021

tjwiebell Feb 1, 2021

revanth0212 Feb 1, 2021

tjwiebell Feb 1, 2021

revanth0212 Feb 4, 2021 •

edited

Loading

tjwiebell left a comment

tjwiebell Feb 4, 2021

tjwiebell Feb 4, 2021

revanth0212 Feb 4, 2021

dpatil-magento commented Feb 5, 2021

dpatil-magento commented Feb 9, 2021

tjwiebell Feb 9, 2021

revanth0212 Feb 9, 2021

revanth0212 Feb 9, 2021

tjwiebell Feb 9, 2021

Support for express server compression middleware. #2980

Support for express server compression middleware. #2980

Conversation

revanth0212 commented Jan 29, 2021

Description

Related Issue

Acceptance

Verification Stakeholders

Verification Steps

Screenshots / Screen Captures (if appropriate)

Checklist

PWAStudioBot commented Jan 29, 2021 • edited Loading

fooman commented Jan 29, 2021

tjwiebell commented Feb 1, 2021

tjwiebell 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

revanth0212 Feb 4, 2021 • edited Loading

Choose a reason for hiding this comment

tjwiebell 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

dpatil-magento commented Feb 5, 2021

dpatil-magento commented Feb 9, 2021

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

PWAStudioBot commented Jan 29, 2021 •

edited

Loading

revanth0212 Feb 4, 2021 •

edited

Loading