Skip to content

Speed up avatar URLs with object storage

Sean McGivern requested to merge stop-signing-avatar-paths into master

What does this MR do?

From the commit message:

With object storage enabled, calling #filename on an upload does this:

  1. Call the #filename method on the CarrierWave object.
  2. Generate the URL for that object.
  3. If the uploader isn't public, do so by generating an authenticated URL, including signing that request.

That's all correct behaviour, but for the case where we use #filename, it's typically to generate a GitLab URL. That URL doesn't need to be signed because we do our own auth.

Signing the URLs can be very expensive, especially in batch (say, we need to get the avatar URLs for 150 users in one request). It's all unnecessary work. If we used the RecordsUploads concern, we have already recorded a path in the database. That path is actually generated from CarrierWave's #filename at upload time, so we don't need to recompute it - we can just use it and strip off the prefix if it's available.

On a sample users autocomplete URL, at least 10% of the time before this change went to signing URLs. After this change, we spend no time in URL signing, and still get the correct results.

More detail on the last part: look at http://profiler.gitlap.com/20190321/9b24d360-38e2-4981-a2cf-9435ace13dcc.html.gz

image

To test this in production, I used the console profiler. OpenSSL::PKey::PKey#sign did not appear in the output after monkey-patching this change in. For extra assurance, I did this in the console: class OpenSSL::PKey::PKey; def sign(*); raise 'should not be called!'; end; end. The request still succeeded.

What are the relevant issue numbers?

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/43065.

Does this MR meet the acceptance criteria?

Merge request reports