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.

Sign up for GitHub

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

Jump to bottom

🔊 Add better logging when loading the Shoryuken environment #691

Merged
merged 4 commits into from
Jan 8, 2022
Merged

🔊 Add better logging when loading the Shoryuken environment #691

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
668e8bf
🔊 Add better logging
danielvdao Jan 6, 2022
349c4d5
Fix Rails 4.2 HEREDOC
danielvdao Jan 7, 2022
cb20244
🔊 add more logging + documentation
danielvdao Jan 7, 2022
47ccb6b
✅ Add tests
danielvdao Jan 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion lib/shoryuken/environment_loader.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,9 +159,17 @@ def validate_queues

return if non_existent_queues.none?

# NOTE: HEREDOC's ~ operator removes indents, but is only available Ruby 2.3+
# See github PR: https://github.com/ruby-shoryuken/shoryuken/pull/691#issuecomment-1007653595
error_msg = <<-MSG.gsub(/^\s+/, '')
The specified queue(s) #{non_existent_queues.join(', ')} do not exist.
Try 'shoryuken sqs create QUEUE-NAME' for creating a queue with default settings.
It's also possible that you don't have permission to access the specified queues.
MSG

fail(
ArgumentError,
"The specified queue(s) #{non_existent_queues.join(', ')} do not exist.\nTry 'shoryuken sqs create QUEUE-NAME' for creating a queue with default settings"
error_msg
)
end

Expand Down
45 changes: 39 additions & 6 deletions spec/shoryuken/environment_loader_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,36 @@
RSpec.describe Shoryuken::EnvironmentLoader do
subject { described_class.new({}) }

describe '#load' do
before do
Shoryuken.groups.clear
# See issue: https://stackoverflow.com/a/63699568 for stubbing AWS errors
allow(Shoryuken::Client)
.to receive(:queues)
.with('stubbed_queue')
.and_raise(Aws::SQS::Errors::NonExistentQueue.new(nil, nil))
Comment on lines +11 to +14
Copy link
Contributor Author

@danielvdao danielvdao Jan 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I guess looking at this, I can do something else similar to - https://stackoverflow.com/a/54314792. It could be better to not depend on an underlying API which is susceptible to change in the specs, otherwise if AWS SDK changes number of parameters, etc. to the Aws::ServiceError class, this test would break. WDYT? @cjlarose

Copy link
Collaborator

Choose a reason for hiding this comment

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

I didn't know about stub_const but it looks like it might be worth trying here!

allow(subject).to receive(:load_rails)
allow(subject).to receive(:prefix_active_job_queue_names)
allow(subject).to receive(:require_workers)
allow(subject).to receive(:validate_workers)
allow(subject).to receive(:patch_deprecated_workers)
Shoryuken.options[:groups] = [['custom', { queues: ['stubbed_queue'] }]]
end

context "when given queues don't exist" do
specify do
expect { subject.load }.to raise_error(
ArgumentError,
<<-MSG.gsub(/^\s+/, '')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ha, the ~ operator is actually used to deal with indentation. The suggested / cleanest solution I could find was to use #gsub. I know it can be expensive though (since it's re-creating string objects), but this doesn't seem like a potential blocker since this command is invoked on load versus runtime.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This solution looks good! Like you said, this isn't a hot path, so the performance won't matter much.

The specified queue(s) stubbed_queue do not exist.
Try 'shoryuken sqs create QUEUE-NAME' for creating a queue with default settings.
It's also possible that you don't have permission to access the specified queues.
MSG
)
end
end
end

describe '#parse_queues loads default queues' do
before do
allow(subject).to receive(:load_rails)
Expand Down Expand Up @@ -34,7 +64,7 @@

specify do
Shoryuken.options[:queues] = ['queue1', 'queue2'] # default queues
Shoryuken.options[:groups] = [[ 'custom', { queues: ['queue3'], delay: 25 }]]
Shoryuken.options[:groups] = [['custom', { queues: ['queue3'], delay: 25 }]]
subject.load

expect(Shoryuken.groups['default'][:queues]).to eq(%w[queue1 queue2])
Expand All @@ -44,7 +74,6 @@
end
end


describe '#prefix_active_job_queue_names' do
before do
allow(subject).to receive(:load_rails)
Expand Down Expand Up @@ -76,7 +105,7 @@

it 'does not prefix url-based queues' do
Shoryuken.options[:queues] = ['https://example.com/test_queue1']
Shoryuken.options[:groups] = {'group1' => {queues: ['https://example.com/test_group1_queue1']}}
Shoryuken.options[:groups] = { 'group1' => { queues: ['https://example.com/test_group1_queue1'] } }

subject.load

Expand All @@ -86,31 +115,35 @@

it 'does not prefix arn-based queues' do
Shoryuken.options[:queues] = ['arn:aws:sqs:fake-region-1:1234:test_queue1']
Shoryuken.options[:groups] = {'group1' => {queues: ['arn:aws:sqs:fake-region-1:1234:test_group1_queue1']}}
Shoryuken.options[:groups] = { 'group1' => { queues: ['arn:aws:sqs:fake-region-1:1234:test_group1_queue1'] } }

subject.load

expect(Shoryuken.groups['default'][:queues]).to(eq(['arn:aws:sqs:fake-region-1:1234:test_queue1']))
expect(Shoryuken.groups['group1'][:queues]).to(eq(['arn:aws:sqs:fake-region-1:1234:test_group1_queue1']))
end
end

describe "#setup_options" do
let (:cli_queues) { { "queue1"=> 10, "queue2" => 20 } }
let (:config_queues) { [["queue1", 8], ["queue2", 4]] }
let(:cli_queues) { { "queue1" => 10, "queue2" => 20 } }
let(:config_queues) { [["queue1", 8], ["queue2", 4]] }

context "when given queues through config and CLI" do
specify do
allow_any_instance_of(Shoryuken::EnvironmentLoader).to receive(:config_file_options).and_return({ queues: config_queues })
Shoryuken::EnvironmentLoader.setup_options(queues: cli_queues)
expect(Shoryuken.options[:queues]).to eq(cli_queues)
end
end

context "when given queues through config only" do
specify do
allow_any_instance_of(Shoryuken::EnvironmentLoader).to receive(:config_file_options).and_return({ queues: config_queues })
Shoryuken::EnvironmentLoader.setup_options({})
expect(Shoryuken.options[:queues]).to eq(config_queues)
end
end

context "when given queues through CLI only" do
specify do
Shoryuken::EnvironmentLoader.setup_options(queues: cli_queues)
Expand Down