Skip to content

Add --num-threads option to query-cloudwatch #9444

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

Merged
zachmargolis merged 1 commit into from
Oct 25, 2023
Merged

Add --num-threads option to query-cloudwatch #9444

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 10 additions & 0 deletions bin/query-cloudwatch
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ class QueryCloudwatch
:format,
:complete,
:progress,
:num_threads,
:wait_duration,
:count_distinct,
keyword_init: true,
Expand Down Expand Up @@ -83,6 +84,7 @@ class QueryCloudwatch
log_group_name: config.group,
progress: config.progress,
slice_interval: config.slice,
num_threads: config.num_threads,
**config.to_h.slice(:wait_duration).compact,
)
end
Expand All @@ -98,6 +100,7 @@ class QueryCloudwatch
time_slices: [],
complete: false,
progress: true,
num_threads: Reporting::CloudwatchClient::DEFAULT_NUM_THREADS,
count_distinct: nil,
)

Expand Down Expand Up @@ -199,6 +202,13 @@ class QueryCloudwatch
end
end

opts.on(
'--num-threads NUM',
"number of threads, defaults to #{Reporting::CloudwatchClient::DEFAULT_NUM_THREADS}",
) do |str|
config.num_threads = Integer(str, 10)
Copy link
Copy Markdown
Contributor

@n1zyy n1zyy Oct 25, 2023

Choose a reason for hiding this comment

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

Asking out of curiosity: is there a reason we explicitly specify that this is base 10 (the second argument)? I see mixed use in the project. AFAICT the default if not specified is base 10; any other behavior would feel very surprising.

I have no objection at all to including it; I'm just curious if there's a fun story here or if it's just a pattern we use on the project.

Copy link
Copy Markdown
Contributor

@n1zyy n1zyy Oct 25, 2023

Choose a reason for hiding this comment

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

Oh, fun, disregard! I answered my own question with an example of my childhood ZIP code:

[12] pry(main)> Integer("03054")
=> 1580
[13] pry(main)> Integer("03054", 10)
=> 3054

This interpretation was one of my first surprises years ago when I first started working with Ruby. (Also also a good lesson in ZIP codes being strings, not integers.)

Copy link
Copy Markdown
Contributor Author

@zachmargolis zachmargolis Oct 25, 2023

Choose a reason for hiding this comment

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

The Integer() method returns wrong results with leading zeroes, so this is about strictness. And I chose Integer over .to_i because .to_i but that silently returns 0 with non-numeric things, but Integer will raise

Integer('077') # unexpected value, bad
=> 63
Integer('077', 10) # right value, good
=> 77
> Integer('a', 10) # should error, good
(irb):6:in `Integer': invalid value for Integer(): "a" (ArgumentError)
> "a".to_i # won't error, bad
=> 0

end

opts.on(
'--date DATE,DATE',
'(optional) dates to query, can be disjoint, to provide time slices to query'
Expand Down
19 changes: 19 additions & 0 deletions spec/bin/query-cloudwatch_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,24 @@
end
end

context 'number of threads' do
let(:argv) { required_parameters }

it 'defaults to Reporting::CloudwatchClient::DEFAULT_NUM_THREADS' do
config = parse!
expect(config.num_threads).to eq(Reporting::CloudwatchClient::DEFAULT_NUM_THREADS)
end

context 'with --num-threads' do
let(:argv) { required_parameters + %w[--num-threads 15] }

it 'overrides the number of threads' do
config = parse!
expect(config.num_threads).to eq(15)
end
end
end

def build_stdin_without_query
StringIO.new.tap do |io|
allow(io).to receive(:tty?).and_return(true)
Expand All @@ -222,6 +240,7 @@ def build_stdin_with_query(query)
query: 'fields @timestamp, @message',
format: format,
count_distinct: count_distinct,
num_threads: Reporting::CloudwatchClient::DEFAULT_NUM_THREADS,
)
end
let(:query_cloudwatch) { QueryCloudwatch.new(config) }
Expand Down