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

Allow out-of-band, async upload functionality #80

Merged
merged 1 commit into from
Sep 19, 2023
Merged

Conversation

bmansoob
Copy link
Contributor

@bmansoob bmansoob commented Jun 7, 2023

TL;DR; Adds a new query string param async. When specified, profile uploads happen in a background thread.

One use case for this is when we want to profile a certain % of traffic without end users explicitly requesting it. In such situations uploading profiles inline does not scale.

From the README:

If the async key in the query string is set to 1 or true, then profile will be uploaded later, in an async manner. One use case for this is when we want to profile a certain % of traffic without incurring costs of inline profile uploads.

If the async parameter is specified we add to a queue/buffer -- which we process every upload_queue_interval_secs. The queue is SizedQueue.

CC @jasonhl

Copy link
Member

@dalehamel dalehamel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I few questions and a couple nitpicks. Looking great overall. I'll try and think of other tests we might want.

lib/app_profiler/middleware/upload_action.rb Outdated Show resolved Hide resolved
lib/app_profiler/middleware/upload_action.rb Show resolved Hide resolved
lib/app_profiler/middleware/upload_action.rb Outdated Show resolved Hide resolved
@bmansoob bmansoob force-pushed the async-upload branch 2 times, most recently from 963399c to 9a8701d Compare June 14, 2023 14:43
@bmansoob bmansoob marked this pull request as ready for review June 14, 2023 14:49
@bmansoob bmansoob requested a review from gmcgibbon June 14, 2023 14:49
lib/app_profiler/middleware/upload_action.rb Outdated Show resolved Hide resolved
lib/app_profiler/middleware/upload_action.rb Outdated Show resolved Hide resolved
lib/app_profiler/railtie.rb Outdated Show resolved Hide resolved
lib/app_profiler/request_parameters.rb Outdated Show resolved Hide resolved
@robertlaurin
Copy link

Curious if you had considered the Rack Events APIto defer the upload work to after the request was complete?

There's some folks at Github exploring this approach as an alternative/successor to the OpenTelemetry rack middleware open-telemetry/opentelemetry-ruby-contrib#342 so that we can defer more work to outside of the request context.

@bmansoob
Copy link
Contributor Author

bmansoob commented Jul 6, 2023

@robertlaurin

to defer the upload work to after the request was complete?

I did not, but:

This has impact on capacity, at least for unicorn. The process cannot accept new requests until this work is done. We have dealt with this issue in the past where we ran into elevated request queuing due to performing somewhat expensive work after sending the response back to the client.

lib/app_profiler/middleware/upload_action.rb Outdated Show resolved Hide resolved
@bmansoob
Copy link
Contributor Author

Rebased and squashed the commit. I will 🎩 this against core and make sure nothing breaks.

lib/app_profiler/railtie.rb Outdated Show resolved Hide resolved
@bmansoob
Copy link
Contributor Author

🎩 'ed on an app

Header:

Screenshot 2023-07-27 at 1 47 49 PM

Logs for queue length:

cat logs | grep Queue                                                                                                               
Queue size is: 0
Queue size is: 0
Queue size is: 1
Queue size is: 0
Queue size is: 0
Queue size is: 1
Queue size is: 0

Copy link
Member

@dalehamel dalehamel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this 🙏

@bmansoob bmansoob force-pushed the async-upload branch 2 times, most recently from 60d40b3 to c52e343 Compare August 1, 2023 01:56
@bmansoob
Copy link
Contributor Author

bmansoob commented Aug 1, 2023

Squashed and rebased.

Also, I saw a test failure on my last push which I tracked down to a race condition between the assertion and the background thread. I added this method to stub the thread creation in tests:

      def with_stubbed_process_queue_thread
        # Stub out the thread creation, so that tests are not flaky.
        GoogleCloudStorage.stubs(:process_queue_thread)
        yield
      ensure
        GoogleCloudStorage.unstub(:process_queue_thread)
      end

@bmansoob
Copy link
Contributor Author

Rebased and resolved the merge conflicts.

@casperisfine -- Can you please give this a read, it is somewhat relevant to the work you have done in #87.

Copy link
Contributor

@casperisfine casperisfine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementation looks fine to me. I particularly appreciate the use of a SizedQueue to avoid piling work faster it's dequeued.

I'm not 100% sure I understand how it's meant to be used though. I see how it could be useful to reduce the impact of profiles triggered programaticaly, but I don't get the need for the X-Profile-Async header.

test/app_profiler/parameters_test.rb Show resolved Hide resolved
@bmansoob
Copy link
Contributor Author

Discussed with Jean in in a DM, about how we intend to use this -- it is for programatically triggered profiles.

I will keep X-Profile-Async header for now, it is mainly to aid us in verifying async functionality.

Will squash the commits and push.

@bmansoob bmansoob merged commit a930ec6 into main Sep 19, 2023
6 checks passed
@bmansoob bmansoob deleted the async-upload branch September 19, 2023 14:28
@shopify-shipit shopify-shipit bot temporarily deployed to production September 20, 2023 09:33 Inactive
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.

5 participants