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

Replace esc with "embed" for packaging UI assets #3229

Closed
jpkrohling opened this issue Aug 24, 2021 · 6 comments
Closed

Replace esc with "embed" for packaging UI assets #3229

jpkrohling opened this issue Aug 24, 2021 · 6 comments
Labels
enhancement good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement

Comments

@jpkrohling
Copy link
Contributor

To complete the issue #2749, there's one PR that needs some attention: #2850. At its current state, it adds the contents of the Jaeger UI build to a Go asset in an uncompressed format. This causes an increase of approximately 10MB to the binary size of the Query/all-in-one.

Ideally, we'd find a way to store the contents of the UI in a compressed format in the binary and serve the compressed asset directly to the client, without having to decompress/recompress.

@jpkrohling jpkrohling added enhancement help wanted Features that maintainers are willing to accept but do not have cycles to implement outreachy good first issue Good for beginners labels Aug 24, 2021
@Ashmita152
Copy link
Contributor

@jpkrohling @yurishkuro Are we okay going ahead with a third party library for compression. If yes, do you have one in mind. I noticed you mentioned: https://github.com/vearutop/statigz and mention gzip compression but looking at open-telemetry/opentelemetry-collector#2548 it seems like newer compression algorithms will be more beneficial here. I came across few more libraries, one which standout is https://github.com/thealetheia/broccoli but this comment thealetheia/broccoli#12 is concerning. It will be nice to use some library based on https://github.com/klauspost/compress library which sarama also uses. I would like to get your thoughts on this before starting any work. Also curious, what is the impact of having 10MB increase in binary size. I am guessing it will take 10MB more memory which isn't too bad in my opinion.

@yurishkuro
Copy link
Member

@Ashmita152 my preference would be if we could do it without a 3rd library. Note that this is not about compression - gzip comes with std library and is perfectly file performance-wise. The issue is implementing an FS interface on top of compressed static assets. We could probably just copy an implementation from one of the libs you mentioned (provided they have compatible license).

what is the impact of having 10MB increase in binary size

I think it is just bad hygiene/practice to bloat binary size (by what, 20% in this case) for a minor functionality, especially one that has no practical impact on the end users.

@Ashmita152
Copy link
Contributor

Thank you Yuri for explaining. I was misinterpreting the problem. That clears my understanding. I will explore it and try writing an FS interface over klauspost/compress.

@mohammadjalali
Copy link

Hi @jpkrohling @yurishkuro. I understand that you want to implement a compression algorithm but I cannot understand what the problem exactly is. If you don't mind please explain a little more to me. I find this project interesting and I like to participate in. Please notify me if I can be a participant.

@yurishkuro
Copy link
Member

@mohammadjalali we do not need to implement a compression algorithm, Go already supports gzip. I updated #2850 with the remaining step that needs implementation.

@pavolloffay
Copy link
Member

pavolloffay commented Oct 26, 2021

Dup of #2749

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants