Compress plain GPX uploads with gzip

[Rub21]

This PR gzips plain GPX files before they go to S3, which reduce a big part of their size. Files that are already compressed (gzip, bzip2, zip, tar) are the same.

The user still gets a plain .gpx, so the download link doesn't change for them. When the app stores a plain GPX gzipped, it sets server_gzipped = true in the blob metadata. On download it works in two ways:

• If the client accepts gzip, the server sends the gzipped file as is with Content-Encoding: gzip and the client unzips it, the server skips the decompression, so it saves CPU and most browsers support Content-Encoding: gzip today.

• If the client does not support gzip, the server unzips it first and sends plain GPX. This is the same work the server already does today when a client requests /data.gpx

For the server-side unzip I reused the existing xml_file function, so there's no new decompression code. The web and API downloads share it through a small concern.

Ref: #4188

cc. @1ec5

[pablobm]

If the majority of downloads are of the uncompressed file, am I right to think that this will translate in costs to the infrastructure? Compute of the server-side decompression and traffic.

If so, can this be significant enough to be a problem? Should the "download gpx" link be slightly more "hidden" to avoid this situation? Just an additional click or something like that.

[Rub21]

@pablobm you're right that this could add real bandwidth and CPU cost. I looked into it a bit more.

I think we can avoid it without hiding the link, using the Content-Encoding: gzip header. The file is already stored gzipped on S3, so we send it as-is and the browser unzips it by itself. The server does nothing and sends the small file.

Browsers always send Accept-Encoding: gzip, so this happens on its own and the user still gets a normal .gpx

I tested it with curl --compressed, which behaves like a browser:

$ curl -s --compressed -D - http://localhost:3000/traces/1/data.gpx -o out.gpx
content-encoding: gzip
content-length: 2025        # 2 KB over the network

$ file out.gpx
out.gpx: XML document text   # plain GPX on disk
$ wc -c out.gpx
15796                        # 15.8 KB unzipped by the client

If a client doesn't support gzip, the server unzips the file and sends it plain, same as today, so nothing breaks. Other formats work the same way too: we still serve whatever the user uploaded. And since the backend decides this from the request header, one download link is enough now, so I kept just one.

[pablobm]

Serving it directly sounds like a good idea! From a quick online lookup, it seems that even MS Edge should support it 👍

[Rub21]

I changed the logic a bit.

There is now a server_gzipped: true flag in the metadata, so the download knows if the server gzipped a plain GPX.

The user still gets a plain GPX either way. If their client accepts gzip, the server sends the gzip and the client unzips it. If not, the server unzips it first.

And the CPU usage already happens today when clients request data.gpx, because the server unzips every file requested with .gpx.

[tomhughes]

Please review the commit history and squash any fixup commits into the things they are fixing so that I can attempt to review this - as it is it's impossible because I find things to comment on and then when I go to leave a comment I find it's no longer there because it's been changed in a later commit.

[pablobm]

The PR description is now out of date, with the mention of Windows users, etc. Mind updating it? It'll make it easier to future readers.

General question: do we have any idea of how common is it to receive requests for the xml/gpx versions of the traces? The request.format == Mime[:xml], etc.

The website doesn't link to those, so I guess any website visitors will get the trace.file.attached? version. However the API docs do describe this option:

The response will always be a GPX format file if you use a .gpx URL suffix, a XML file in an undocumented format if you use a .xml URL suffix, otherwise the response will be the exact file that was uploaded.

Is this something that is very common and could generate this extra effort on our side, decompressing every time?

[pablobm]

Thinking a bit more about this. Would the server_gzipped path become the dominant one with this solution? If so, are we cornering ourselves into a situation where most future downloads will be served with send_data through the Ruby app, instead of delegated to S3?

I seem to remember there was a way to have S3 store gzipped files and serve them as Content-Encoding: gzip, which could be a solution here. Quick lookup yields https://stackoverflow.com/a/15117310

[Rub21]

I seem to remember there was a way to have S3 store gzipped files and serve them as Content-Encoding: gzip, which could be a solution here. Quick lookup yields https://stackoverflow.com/a/15117310

Good point, I looked at the Content-Encoding on S3 idea, but ActiveStorage's upload method can not set that header on the file. It can set content_type and some custom metadata and usea a prefix: x-amz-meta-* , but not Content-Encoding. To do it we would need to implement our own S3 code, we can implement that in other PR.

[Rub21]

I squashed into 2 commits, ready for another look, thanks.

[pablobm]

To do it we would need to implement our own S3 code, we can implement that in other PR.

My concern then is that this introduces new steps in this effort. If we do as you say, then we'll still need the if trace.file.metadata["server_gzipped"] paths in both controllers, serving the files that were uploaded in the interim. That's a maintainability issue (code to handle a situation that existed only for a short period), so we should migrate away from that too.

So the steps would be:

1. This PR.
2. PR with custom S3 uploader that sets Content-Encoding.
3. Ops team to run a script that converts (re-uploads?) the server_gzipped files to have Content-Encoding on S3.
   • That's probably for you to write and @tomhughes to run.
4. PR that removes the if trace.file.metadata["server_gzipped"] paths.

This sounds a bit more complex. What do Ops think? Would it be worth doing properly the first time?

[Rub21]

yeah, a custom uploader that stores the file on S3 with Content-Encoding: gzip makes more sense. I was trying to reuse the same upload path to keep the PR simple, but now that you point it out, the custom uploader is the better way.

The download side gets simpler too. If the client accepts gzip, we redirect to S3 and it will server the file, so the app stays out of the way. If a client doesn't support gzip, the server unzips it and sends it plain. The server already does that today when someone asks for /data.gpx, so it's not new behavior,

For the old files, once the new compression is running, we could add a background job that takes each plain GPX, gzips it, and re-uploads it with Content-Encoding: gzip. but that is for other PR.

[Rub21]

I implemented the custom uploader. GpxS3Service sets Content-Encoding: gzip on upload, so S3 serves the file gzipped and the client unzips it. On download, the clients that accept gzip get a redirect to S3.

I kept the server_gzipped flag for now, the server needs it to know when to unzip for clients without gzip. We can drop it once all the old plain GPX is migrated to gzip, but it is for other PR.

[pablobm]

Something to mention in the description and the documentation: in storage.yml, now we'll need service: GpxS3.

[tomhughes]

Why do we need to monkey patch rails? We seem to support gzip compressed traces currently without any of that nonsense so I don't see why we need it for ones which we compress?

I assume it's because of Windows not handling gzipped traces well and wanting to try and automatically decompress them again? But we've never worried about gzipped traces not being handled well on Windows before so why start now?

[tomhughes]

Can somebody explain how GpxS3Service even gets invoked? The only thing that I can see that uses it is a test?

[pablobm]

Why do we need to monkey patch rails?

It's not a monkey patch. The PR introduces a new storage adapter (ActiveStorage::Service::GpxS3Service) that inherits from an existing one (ActiveStorage::Service::S3Service).

Can somebody explain how GpxS3Service even gets invoked?

Set service: GpxS3 in storage.yml. I think that the new adapter exists in the ActiveStorage namespace so that it can be referenced here. No idea if different naming is possible. This PR is missing a change to the example file and a mention in the docs.

It's true that the code changes make the objective of this PR a bit unclear. The objective is making it so that we compress GPX files on upload (unless already compressed), in such a way that doesn't require us to decompress them back when serving them, because we delegate decompression to browsers via Content-Encoding.

For comparison, I have a GPX file here (taken from the website at random) that compresses from 341kB to 24kB. Having said that, I don't know what the storage impact would be overall as I don't know how frequently users upload plain GPX files as opposed to compressed ones.

[tomhughes]

Why do we need to monkey patch rails?

It's not a monkey patch. The PR introduces a new storage adapter (ActiveStorage::Service::GpxS3Service) that inherits from an existing one (ActiveStorage::Service::S3Service).

Do you know if there's any documentation on writing new adapters like this? Does it need to be in the rails owned ActiveStorage namespace, or could it be somewhere else?

Can somebody explain how GpxS3Service even gets invoked?

Set service: GpxS3 in storage.yml. I think that the new adapter exists in the ActiveStorage namespace so that it can be referenced here. No idea if different naming is possible. This PR is missing a change to the example file and a mention in the docs.

So this doesn't actually use this by default, we will need to enable it in our production environment? What happens if somebody uses S3 and doesn't enable it? The traces still get compressed but don't get automatically decompressed on download?

It's true that the code changes make the objective of this PR a bit unclear. The objective is making it so that we compress GPX files on upload (unless already compressed), in such a way that doesn't require us to decompress them back when serving them, because we delegate decompression to browsers via Content-Encoding.

For comparison, I have a GPX file here (taken from the website at random) that compresses from 341kB to 24kB. Having said that, I don't know what the storage impact would be overall as I don't know how frequently users upload plain GPX files as opposed to compressed ones.

I understand the goal is to compress otherwise uncompressed traces.

What I'm questioning is whether we need to automatically decompress them give we already have compressed traces and don't do that for them.

[pablobm]

What I'm questioning is whether we need to automatically decompress them give we already have compressed traces and don't do that for them.

Right. As I see it, currently this is what happens:

1. User uploads a GPX: we store the GPX exactly as given. If later downloaded, the user gets this GPX file.
2. User uploads a gzipped GPX: we store the gzip. If later downloaded, the user gets the gzip, it's their problem to decompress it. After all, they surely were able to compress it in the first place.
3. Same for all other types of file.

This PR changes the first case:

• User uploads a GPX: we compress it and store it as gzip. If later downloaded, the user will get a GPX and won't need to decompress it themselves (which particularly for Windows users would be problematic). All this while delegating the decompression to the browser by specifying the Content-Encoding.

End result: on download, users get exactly what they uploaded, avoiding confusion. We do something clever in the middle to compress/decompress with minimum impact to our infrastructure.

[Rub21]

Can somebody explain how GpxS3Service even gets invoked?

To actually use it, we need switch the traces storage in chef from service: "S3" to service: "GpxS3" rails.rb#L67. It's the same standard the other services already use: avatars, traces and images are all set by name as service: "S3" today. I'll add the example config and a docs note, which are missing.

Does it need to be in the rails owned ActiveStorage namespace, or could it be somewhere else?

From what I saw in the default configs, Active Storage turns the config name into the class name. For example, S3 maps to S3Service, and if we set GpxS3 it maps to GpxS3Service. That's the default, so the name and the namespace have to match.

What happens if somebody uses S3 and doesn't enable it? The traces still get compressed but don't get automatically decompressed on download?

This PR code gzips every plain GPX and stores it gzipped. The only difference between S3 and GpxS3 is the header: S3 stores the file without Content-Encoding, and GpxS3 adds it. So when a client asks for the file:

• With S3, the server decompresses it and sends back the plain GPX the user uploaded.
• With GpxS3, the app redirects to S3, which already has the Content-Encoding header. S3 sends the file and the browser decompresses it.

Either way the user gets their plain GPX back. Nothing breaks without GpxS3, we just decompress on the server instead of letting the browser do it.

whether we need to automatically decompress them give we already have compressed traces and don't do that for them.

The goal in #4188 is to compress the plain GPX files that people upload, to save storage and transfer-out. The part I wanted to keep is that users get back what they uploaded. If a user uploads a .gpx, it downloads as .gpx today. If we store it as gz and serve it raw, that user gets a .gz instead, which changes the behavior. I don't think this differs from leaving user .gz uploads alone. The rule is the same: give back what was uploaded.

If we're ok with plain GPX downloading as .gz, this gets much simpler: compress on upload, no custom adapter, no Content-Encoding, no flag. It's more of a product call, and I'm happy to go that way if the decision is to download as .gz.

[tomhughes]

It's not a monkey patch. The PR introduces a new storage adapter (ActiveStorage::Service::GpxS3Service) that inherits from an existing one (ActiveStorage::Service::S3Service).

I've looked at this in more detail and while it may not technically be a monkey patch in the traditional sense it's functionally equivalent to one in terms of how it works and the maintenance risk it creates.

As far as I can tell adding your own services is not an intended customisation point in the rails code base, and if it was they would probably have made it possible without having to inject code into their namespace.

That isn't the main problem though - the main problem is that this code is essentially copy and pasted from the code in the existing S3 service, except that for some reason it's been changed from using named parameters to using a generic options argument for most things, and the multipart upload support has been removed, though by default that is only used for files over 100Mb in size so that might be reasonable.

The result though is that any change to the internals of the current S3 service will need to be reflected here which creates a maintenance headache, especially for things that don't cause obvious immediate breakage.

That said I don't see any better way to set the Content-Encoding argument on upload 😢

[tomhughes]

The third commit should probably be squashed into the first one?

[Rub21]

@tomhughes, You're right, copying the whole upload was too much. It also had a real problem. It forced a single-part PUT for every upload, so large .zip or .tar traces skipped multipart. I reworked it, the GpxS3Service no longer overrides the public upload.

It now overrides only upload_with_single_part and upload_with_multipart, and just adds Content-Encoding: gzip in the PUT. The upload routing and checksum stay in the parent S3 service, so multipart keeps working for large files and there's much less copied code.

I also added the missing example in example.storage.yml and a note in settings.yml.

[pablobm]

I think it's worth creating a PR for Rails to support content_encoding here. I had a quick look and there doesn't seem to be any issue or PR.

That might not go anywhere, or perhaps it does. In any case it could give us some additional insight, if there's some detail we are overlooking. It doesn't need to block this PR: if in the future it's merged, we can drop the custom service.

[Rub21]

Right. So now I see it's not dead in the case where request_accepts_gzip? is false. However: is there any use to checking store_serves_gzip?(trace)?

The check is needed. Compression happens in the model on upload, so gzipped_by_server? is true for any storage. The Content-Encoding header only comes from GpxS3. So with plain S3 or Disk, the file is gzipped (gzipped_by_server? true) but there is no header (store_serves_gzip? false). That is also the current default, since GpxS3 is not enabled yet.

Without the store_serves_gzip? check we would redirect and the client gets the raw gzip named .gpx with no Content-Encoding, so a broken file. The check sends those cases to send_data so the server unzips first. It is the same case tomhughes asked about: using S3 without GpxS3.

[Rub21]

Here is an example, tied to what Tom asked about:

What happens if somebody uses S3 and doesn't enable it? The traces still get compressed but don't get automatically decompressed on download?

Let's say we merge this PR and run it on plain S3 for now, without GpxS3.

A user uploads route.gpx:

• The model gzips it before storing, so server_gzipped = true.
• S3 stores the gzipped file, but plain S3 does not add the Content-Encoding: gzip header.

Later the user downloads it:

• gzipped_by_server? = true (we did gzip it).
• store_serves_gzip? = false (plain S3, no header).
• So the code goes to send_data: the server unzips the file and sends a plain GPX. The user gets route.gpx back, same as before this PR.

Now let's say we enable GpxS3 later in the deploy

A user uploads route.gpx:

• The model gzips it before storing, so server_gzipped = true (same as before).
• GpxS3 stores the gzipped file and adds the Content-Encoding: gzip header.

Later the user downloads it:

• gzipped_by_server? = true (we did gzip it).
• store_serves_gzip? = true (GpxS3 adds the header).

So the code redirects to S3. S3 serves the gzipped file with the header, and the browser unzips it. The server does no work. The user still gets route.gpx back.

Either way the user gets a plain GPX. The only difference is who unzips: on plain S3 and Disk the server does it (send_data), on GpxS3 the browser does it (redirect). store_serves_gzip? is the check that picks between the two, so it is needed.

All three storages compress the file. Only GpxS3 serves the header, so only there the browser unzips:

Storage	compresses	Content-Encoding header	who unzips
GpxS3	yes	yes	browser (redirect)
S3	yes	no	server (send_data)
Disk	yes	no	server (send_data)

[Rub21]

I ran this on a deploy with GpxS3. I uploaded a plain GPX of about 200 KB. On S3 it is stored gzipped, only 22.7 KB, and with the Content-Encoding: gzip header.

https://osm-s3-website-dev.s3.us-east-1.amazonaws.com/krivwyyrck6usfbhjgmugyh461p2

On download, a browser gets the small file from S3 and unzips it on its own, so it still receives a normal GPX. A client that does not accept gzip gets the plain file, the server unzips it first. Either way the file is the same one I uploaded.

BASE=https://compress-gpx-uploads.167.233.138.142.nip.io

# like a browser: it fetches the small gzipped file and unzips it
> curl -s --compressed -L "$BASE/traces/2/data" -o out.gpx -w "on the wire: %{size_download} bytes\n"
# on the wire: 23202 bytes

# see the header S3 sends
> curl -s -H "Accept-Encoding: gzip" -D - -L "$BASE/traces/2/data" -o /dev/null | grep -i content-encoding
# content-encoding: gzip

# a client without gzip: the server unzips it and sends the plain 200 KB
curl -s -H "Accept-Encoding: identity" -L "$BASE/traces/2/data" -o plain.gpx -w "on the wire: %{size_download} bytes\n"
# on the wire: 200176 bytes

[pablobm]

I think something is wrong. Scenario:

1. PR merged&deployed. Still on S3 adapter.
2. Trace uploaded with server_gzipped: true. File stored gzipped, but NO Content-Encoding: gzip
3. Download: gzipped_by_server? => true, store_serves_gzip? => false, therefore we decompress and send_data
4. Change to GpxS3 adapter.
5. Download: gzipped_by_server? => true, store_serves_gzip? => true, therefore redirected to path, but there's NO Content-Encoding: gzip! The user gets a gzip file marked as gpx, so gibberish to them 😞

[Rub21]

You're right, the check depended on the storage config, but the header is a property of each file, set once at upload time. to that approach won't work properly

I changed it to a per-file flag on upload, the model sets gzip_content_encoding in the blob metadata only when the store sets the Content-Encoding header (GpxS3). On download, store_serves_gzip? reads that flag instead of asking the current service.

I've also added tests for these cases 👇 :

• In the model: test_file_flags_server_gzipped_without_encoding_store and test_file_flags_gzip_content_encoding_with_encoding_store. They check that a normal store only sets server_gzipped, and a store like GpxS3 sets both flags.

• In the download controller: test_show_gzipped_by_server_without_store_header and test_show_gzipped_by_server_with_store_header. Without the flag the server unzips the file, with the flag it redirects. The first one is your scenario.

The scenario, step by step, what you highlight above:

1. PR merged & deployed. Still on S3 adapter.

2. Trace uploaded with server_gzipped: true . File stored gzipped, NO Content-Encoding: gzip header, and NO gzip_content_encoding flag (plain S3 doesn't set the header).

3. Download: gzipped_by_server? => true, store_serves_gzip? => false, so we decompress and send_data. Same as before.

4. Change to GpxS3 adapter.

5. Download: gzipped_by_server? => true, store_serves_gzip? => false, because now it reads the gzip_content_encoding flag from the file, not the current adapter. The old trace doesn't have it. So we still decompress and send_data, and the user gets a normal GPX 🙂

6. New trace uploaded under GpxS3: it gets both flags and the object has the Content-Encoding: gzip header on S3. Download: gzipped_by_server? => true, store_serves_gzip? => true, so it redirects and the browser unzips it with the header.

Files uploaded before enabling GpxS3 keep the server unzip path, later we can add a small backfill task to move them to the redirect path. The same backfill can also handle the old traces from before this PR, the ones without server_gzipped or gzip_content_encoding. I think that can be a other PR.

[tomhughes]

Please squash the relevant parts of the third commit into the first two... I'm not sure exactly what it's doing because I gave up reviewing when I realised that commit was completely changing how things work which is my point, that I don't want to waste my time reviewing changes only to find a later commit is going to remove them!

[tomhughes]

I think it's worth creating a PR for Rails to support content_encoding here. I had a quick look and there doesn't seem to be any issue or PR.

I'd certainly be much happier with this if it looked like we were on a path to not needing the custom service.

[Rub21]

Please squash the relevant parts of the third commit into the first two...

I squashed into two commits, the first adds the GpxS3 service, the second stores plain GPX gzipped and serves it with a redirect, it is ready for another look!

[Rub21]

Thanks @pablobm, I've applied both changes, the settings.yml comment and the multipart test coverage and I squashed them into the GpxS3 service commit.

[Rub21]

I'd certainly be much happier with this if it looked like we were on a path to not needing the custom service.

I opened a topic on the Rails core forum to propose per-file content_encoding support in Active Storage: https://discuss.rubyonrails.org/t/allow-setting-content-encoding-when-uploading-a-file-with-active-storage/91439