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

WIP - Replace esc with "embed" for packaging UI assets #2850

Closed
wants to merge 5 commits into from

Conversation

yurishkuro
Copy link
Member

@yurishkuro yurishkuro commented Feb 26, 2021

Part of #2749, closes #3229

  • infra changes to use go:embed in the build to pack UI assets
  • use embed.FS as a source for HTTP server for static assets
  • an adapter that implements http.FileSystem that can read gzip-compressed files from embed.FS (similar to https://github.com/vearutop/statigz, but we want to avoid 3rd party dependency)

@yurishkuro yurishkuro requested a review from a team as a code owner February 26, 2021 04:28
@mergify mergify bot requested a review from jpkrohling February 26, 2021 04:29
@yurishkuro
Copy link
Member Author

yurishkuro commented Feb 26, 2021

As I was afraid, the binary size goes up a lot (+13Mb) without doing some explicit compression:

# with esc
-rwxr-xr-x      43M Feb 25 23:15 ./cmd/all-in-one/all-in-one-darwin-amd64*
# with embed
-rwxr-xr-x      56M Feb 25 23:21 ./cmd/all-in-one/all-in-one-darwin-amd64*

@codecov
Copy link

codecov bot commented Feb 26, 2021

Codecov Report

Merging #2850 (1eecfee) into master (639333a) will decrease coverage by 0.48%.
The diff coverage is n/a.

❗ Current head 1eecfee differs from pull request most recent head 9af3744. Consider uploading reports for the commit 9af3744 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2850      +/-   ##
==========================================
- Coverage   96.47%   95.99%   -0.49%     
==========================================
  Files         260      242      -18     
  Lines       15215    14789     -426     
==========================================
- Hits        14679    14196     -483     
- Misses        453      513      +60     
+ Partials       83       80       -3     
Impacted Files Coverage Δ
cmd/anonymizer/app/anonymizer/anonymizer.go 64.10% <0.00%> (-19.66%) ⬇️
...ugin/sampling/strategystore/adaptive/aggregator.go 53.44% <0.00%> (-9.27%) ⬇️
cmd/flags/flags.go 46.29% <0.00%> (-1.92%) ⬇️
cmd/query/app/grpc_handler.go 98.53% <0.00%> (-0.16%) ⬇️
model/span.go 100.00% <0.00%> (ø)
cmd/env/command.go 100.00% <0.00%> (ø)
cmd/flags/admin.go 82.81% <0.00%> (ø)
cmd/query/app/server.go 95.58% <0.00%> (ø)
cmd/query/app/static_handler.go 95.80% <0.00%> (ø)
plugin/storage/badger/factory.go 95.80% <0.00%> (ø)
... and 56 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 639333a...9af3744. Read the comment docs.

@yurishkuro yurishkuro changed the title Replace esc with "embed" for packaging UI assets WIP - Replace esc with "embed" for packaging UI assets Feb 26, 2021
@yurishkuro
Copy link
Member Author

If all files are gzipped, then the size is comparable with esc (even a tiny bit smaller)

-rwxr-xr-x      42M Feb 25 23:40 ./cmd/all-in-one/all-in-one-darwin-amd64*

@yurishkuro
Copy link
Member Author

Found a brand new library by @vearutop that serves gzipped files https://github.com/vearutop/statigz. Not sure if we want to use it. It might conflict with our usage of gorilla.CompressHandler, although we could change it so that it doesn't apply to static assets.

@yurishkuro
Copy link
Member Author

@albertteoh fyi

@vearutop
Copy link

Combining statigz with gorilla.CompressHandler would work, but in a sub optimal way.

The problem is that gorilla.CompressHandler removes Accept-Encoding header before passing request to next handler.
https://github.com/gorilla/handlers/blob/v1.5.1/compress.go#L119

r.Header.Del(acceptEncoding)

In such case statigz would think user agent is not capable to receive compressed response and will decompress it on the fly, then this decompressed response will be compressed again by gorilla.CompressHandler.

albertteoh
albertteoh previously approved these changes Feb 26, 2021
jpkrohling
jpkrohling previously approved these changes Mar 1, 2021
@jpkrohling
Copy link
Contributor

What's the status of this PR?

@yurishkuro
Copy link
Member Author

It needs to be done, but I didn't have time. Currently it blows up the size of the binaries unless we introduce a new lib dependency.

@jpkrohling
Copy link
Contributor

It needs to be done, but I didn't have time. Currently it blows up the size of the binaries unless we introduce a new lib dependency.

Do you think an Outreachy candidate would be able to tackle this if a good description is provided in an issue?

@yurishkuro
Copy link
Member Author

yes, it's a fairly straightforward piece.

Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
@yurishkuro
Copy link
Member Author

incorporated into #3399

@yurishkuro yurishkuro closed this Nov 19, 2021
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.

Replace esc with "embed" for packaging UI assets
4 participants