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

Improve CloudFront cache hit ratio #334

Closed
wants to merge 1 commit into from
Closed

Improve CloudFront cache hit ratio #334

wants to merge 1 commit into from

Conversation

guidev
Copy link

@guidev guidev commented Feb 7, 2022

Issue #, if available:

#304

Description of changes:

There's no need to include in the cache key the origin and accept http headers if AutoWebP is disabled.

This improves the cache hit ratio if AutoWebP is not used.

Checklist

  • 👋 I have run the unit tests, and all unit tests have passed.
  • ⚠️ This pull request might incur a breaking change.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

There's no need to include in the cache key the "origin" and "accept" http headers if AutoWebP is disabled. 

This improves the cache hit ratio if AutoWebP is not used.

#304
@fisenkodv
Copy link
Contributor

@guidev thanks for your contribution. Could you please elaborate the reason why

There's no need to include in the cache key the origin and accept http headers if AutoWebP is disabled.

but when AutoWebP is enabled the cache key needs the origin and accept header?

Also, could you please add unit tests to cover the changes?

@fisenkodv fisenkodv self-assigned this Feb 11, 2022
@fvsnippets
Copy link

fvsnippets commented Jun 18, 2022

Correct me if I am wrong.
ServerlessImageHandler solution code, aims to create, via CDK, a "final distributable template" that will be configured later by using Parameters (e.g. autoWebPParameter), that is during deployment time.

The code provided by this PR is making decisions at CDK synthesis time, based on a value that is unknow during cdk runtime.
In detail:
props.autoWebP is defined on serverless-image-stack.ts as autoWebPParameter.valueAsString, which in turn is finally translated on the generated template.yaml as Ref: AutoWebPParameter.
It won't contain the "Yes" or "No" value during CDK synthesis time.

--

On the other hand, the header "origin" has nothing to do with WebP's automatic conversion, but with cors enabled or not. If anything, we could remove the "origin" header from the cache key (and also stop forwarding it to the image handler) whenever cors is disabled (but that would be another PR).

--

I am sending a new PR for the "accept" header exclusion when WebP's automatic conversion is disabled, based on cloudformation's "Fn::If" intrinsic function, that will be evaluated by cloudformation (i.e. deployment time).

@guidev
Copy link
Author

guidev commented Sep 12, 2022

The code provided by this PR is making decisions at CDK synthesis time, based on a value that is unknow during cdk runtime.

@fvsnippets You're right, let's delete this pull request

@guidev guidev closed this Sep 12, 2022
@guidev guidev deleted the patch-1 branch September 12, 2022 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants