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

Add request_id option to client #133

Merged
merged 51 commits into from
Apr 23, 2018
Merged

Add request_id option to client #133

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
51 commits
Select commit Hold shift + click to select a range
a6a0946
Add request_id option to client
funkyboy Apr 17, 2018
fef93a1
Simplify options setting
funkyboy Apr 17, 2018
67d06b8
Add request_id to connection error and timeouts
funkyboy Apr 17, 2018
1fc2ef8
Remove path munging
funkyboy Apr 17, 2018
5d3666c
Add ConnectionTimeout test with request_id
funkyboy Apr 17, 2018
1670bb1
Use urlsafe_base64 and a local var for request_id
funkyboy Apr 17, 2018
9357a13
Add request_id to printed exception
funkyboy Apr 17, 2018
55f18aa
Fix spacing
funkyboy Apr 17, 2018
a1f71d4
Use attr_reader to access add_request_ids
funkyboy Apr 17, 2018
d39e11c
Add request_id to Faraday request context
funkyboy Apr 18, 2018
d9b6b8f
Read request_id from Faraday context
funkyboy Apr 18, 2018
3e55b9e
Reorganize request_id generation tests
funkyboy Apr 18, 2018
a061d3b
Add test for UnauthorizedRequest nonce
funkyboy Apr 18, 2018
c227120
Fix frozen hash issue
funkyboy Apr 18, 2018
890ced2
Add test without request_id
funkyboy Apr 18, 2018
97febef
Rename test context
funkyboy Apr 18, 2018
dc6f216
Pass request_id in Exception options
funkyboy Apr 18, 2018
844a938
Fix fetching of request_id from Faraday context
funkyboy Apr 18, 2018
a7f1e29
Merge request_id with Faraday context options
funkyboy Apr 18, 2018
66259dc
Fix spacing and indentation
funkyboy Apr 18, 2018
21da79c
Rename random_request_id as request_id
funkyboy Apr 18, 2018
aca07ad
Declare request_id as a local var
funkyboy Apr 18, 2018
f5cfc0f
Fix indentation
funkyboy Apr 19, 2018
f99e186
Add test to check if request_id is the same at request and response time
funkyboy Apr 20, 2018
4159c51
Add request_id option to client
funkyboy Apr 17, 2018
c2478be
Simplify options setting
funkyboy Apr 17, 2018
daacf46
Add request_id to connection error and timeouts
funkyboy Apr 17, 2018
44d08b7
Remove path munging
funkyboy Apr 17, 2018
d531f5a
Add ConnectionTimeout test with request_id
funkyboy Apr 17, 2018
24435b8
Use urlsafe_base64 and a local var for request_id
funkyboy Apr 17, 2018
4425fb8
Add request_id to printed exception
funkyboy Apr 17, 2018
756b266
Fix spacing
funkyboy Apr 17, 2018
72eda17
Use attr_reader to access add_request_ids
funkyboy Apr 17, 2018
3308586
Add request_id to Faraday request context
funkyboy Apr 18, 2018
39a9b75
Read request_id from Faraday context
funkyboy Apr 18, 2018
eb87ad6
Reorganize request_id generation tests
funkyboy Apr 18, 2018
f0e3730
Add test for UnauthorizedRequest nonce
funkyboy Apr 18, 2018
8efa111
Fix frozen hash issue
funkyboy Apr 18, 2018
a34000f
Add test without request_id
funkyboy Apr 18, 2018
df94980
Rename test context
funkyboy Apr 18, 2018
2731ca7
Pass request_id in Exception options
funkyboy Apr 18, 2018
528c92a
Fix fetching of request_id from Faraday context
funkyboy Apr 18, 2018
7399f18
Merge request_id with Faraday context options
funkyboy Apr 18, 2018
48c6cfc
Fix spacing and indentation
funkyboy Apr 18, 2018
c2ab83c
Rename random_request_id as request_id
funkyboy Apr 18, 2018
705f143
Declare request_id as a local var
funkyboy Apr 18, 2018
0b33cdd
Fix indentation
funkyboy Apr 19, 2018
8b87e5f
Add test to check if request_id is the same at request and response time
funkyboy Apr 20, 2018
749d49c
Add test for request_id when fallback is on and the request is retried
funkyboy Apr 23, 2018
e17125c
Dup params only if needed
funkyboy Apr 23, 2018
0774a2d
Add note about request_id in the middleware request context
funkyboy Apr 23, 2018
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
8 changes: 4 additions & 4 deletions lib/ably/exceptions.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ module Exceptions
class BaseAblyException < StandardError
attr_reader :status, :code, :request_id

def initialize(message, status = nil, code = nil, base_exception = nil, options = {}, request_id = nil)
def initialize(message, status = nil, code = nil, base_exception = nil, options = {})
super message

@base_exception = base_exception
Expand All @@ -25,7 +25,7 @@ def initialize(message, status = nil, code = nil, base_exception = nil, options
@code = code
@code ||= base_exception.code if base_exception && base_exception.respond_to?(:code)
@code ||= options[:fallback_code]
@request_id = request_id
@request_id ||= options[:request_id]
end

def to_s
Expand Down Expand Up @@ -73,8 +73,8 @@ class EncoderError < BaseAblyException; end

# Connection error from Realtime or REST service
class ConnectionError < BaseAblyException
def initialize(message, status = nil, code = nil, base_exception = nil, options = {}, request_id = nil)
super message, status, code, base_exception, options, request_id
def initialize(message, status = nil, code = nil, base_exception = nil, options = {})
super message, status, code, base_exception, options
end

def to_s
Expand Down
4 changes: 2 additions & 2 deletions lib/ably/rest/client.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -470,9 +470,9 @@ def send_request(method, path, params, options)
end
case error
when Faraday::TimeoutError
raise Ably::Exceptions::ConnectionTimeout.new(error.message, nil, 80014, error, {}, random_request_id)
raise Ably::Exceptions::ConnectionTimeout.new(error.message, nil, 80014, error, {request_id: random_request_id})
Copy link
Member

Choose a reason for hiding this comment

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

Spacing issues again, here and many times below

Copy link
Member

Choose a reason for hiding this comment

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

I am not a big fan of this. random_request_id is defined in line 445, yet is within a conditional. I realise Ruby allows this sort of thing, but it's messy and error prone. Why are you defining random_request_id as a varaiable when you have access to the params variable in this scope which is available anyway?

Copy link
Member

Choose a reason for hiding this comment

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

In fact, I am not sure I have seen a test that confirms that a) a request is sent to Ably with a request_id, b) it fails (intentionally), c) the test confirms that the exception raised has that SAME request_id, d) the Logger is showing the request_id in the output. This is the crux of what we're doing here. Given this code path, I fear our tests so far are just testing that a request_id is present, but not ensuring it is consistent throughout.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes sense, but I am struggling a bit to inspect the url of a request, say client.stats. Any suggestion?

when Faraday::ClientError
raise Ably::Exceptions::ConnectionError.new(error.message, nil, 80000, error, {}, random_request_id)
raise Ably::Exceptions::ConnectionError.new(error.message, nil, 80000, error, {request_id: random_request_id})
else
raise error
Copy link
Member

Choose a reason for hiding this comment

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

One of the errors they want to correlate was the nonce value one, which'll be in this branch, that also needs the requestId set.

Copy link
Member

Choose a reason for hiding this comment

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

I am also not keen on a var called random_request_id, that sounds more like a method. It's no longer random once it's assigned, and thus it's consistent for the request.

end
Expand Down
2 changes: 1 addition & 1 deletion lib/ably/rest/middleware/exceptions.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ def on_complete(env)

message = 'Unknown server error' if message.to_s.strip == ''
request_id = env.request.context[:request_id]
exception_args = [message, error_status_code, error_code, nil, nil, request_id]
exception_args = [message, error_status_code, error_code, nil, {request_id: request_id}]

if env.status >= 500
raise Ably::Exceptions::ServerError.new(*exception_args)
Expand Down