Add request_id option to client #133

funkyboy · 2018-04-17T07:41:54Z

Adds a request_id=random_value query string to any path if the client is initialized with options
{ add_request_ids: true }. Handy for debugging.

SimonWoolf

This is missing the crucial part, which is including the request_id in the ConnectionTimeout, ConnectionError etc. so that when the customer gets the timeout they can correlate it with our logs. Otherwise there's no point, we get the request id but don't have anything to correlate it with

SimonWoolf · 2018-04-17T08:24:24Z

lib/ably/rest/client.rb

@@ -146,6 +150,7 @@ def initialize(options)
        @custom_host      = options.delete(:rest_host)
        @custom_port      = options.delete(:port)
        @custom_tls_port  = options.delete(:tls_port)
+        @add_request_ids  = options.delete(:add_request_ids) == false ? false : true


As an optional extra debug option, this should default to false (unless and until it's added to the spec). So just @add_request_ids = options.delete(:add_request_ids) is fine.

(Also, cond ? false : true is a pointless pattern, it's equivalent to !cond)

SimonWoolf · 2018-04-17T08:26:14Z

lib/ably/rest/client.rb

+          path_with_params.query_values = {request_id: SecureRandom.hex(20)}
+          query = path_with_params.query
+          path = "#{path}?#{query}"
+        end


Why are you manually munging the path? This method takes a params hash. You can add it to the params hash.

@SimonWoolf because this is Ruby and not Elixir. So the params object would be changed by just calling this method, which is bad.

So the params object would be changed by just calling this method, which is bad.

@mattheworiordan fair enough, but imo the solution to that is to clone the hash (as Cesare has done in the latest version of this pr), not to start munging a param into the path (meaning faraday has to parse it out again to combine with the params -- assuming it even does that rather than give you a path with two ?s). (for one thing the original approach assumes the path doesn't already have a querysting, so fails the 'what if two things did this' test)

SimonWoolf · 2018-04-17T08:30:21Z

spec/unit/rest/client_spec.rb

+    it 'includes request_id in URL' do
+      expect(subject.add_request_ids).to eql(true)
+    end
+  end


If you're going to add a test, may as well be one that checks the result (i.e. uses webmock to check that a request_id actually gets added to the querystring for rest requests, and checks timeout exceptions to make sure that the request ID gets printed with those), rather than just checks that an instance variable got set correctly, no?

funkyboy · 2018-04-17T12:40:21Z

@SimonWoolf re-ready for review.

SimonWoolf

Almost there

SimonWoolf · 2018-04-17T13:01:32Z

lib/ably/rest/client.rb

@@ -434,6 +439,10 @@ def send_request(method, path, params, options)
        max_retry_duration = http_defaults.fetch(:max_retry_duration)
        requested_at       = Time.now
        retry_count        = 0
+        if @add_request_ids
+          params = {} if params.nil?
+          params[:request_id] = SecureRandom.hex(20)


Hex takes 2 characters per byte, which means this adds 40 characters, which is just unnecessary. Go for SecureRandom.urlsafe_base64(10) instead, that'll be 14 characters

SimonWoolf · 2018-04-17T13:02:49Z

lib/ably/exceptions.rb

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


Getting there, but just having the request ID as a property of the exception isn't enough -- it'll be printed to a log file, it needs to be in the string representation, next method below this one.

If we leave request_id as an option of options, then no change here is needed.

SimonWoolf · 2018-04-17T13:04:42Z

lib/ably/rest/client.rb

@@ -456,12 +465,12 @@ def send_request(method, path, params, options)
            logger.warn { "Ably::Rest::Client - Retry #{retry_count} for #{method} #{path} #{params} as initial attempt failed: #{error}" }
            retry
          end
-
+          request_id = params[:request_id] if @add_request_ids


you generated and set the request ID only a few lines earlier; just make a local var there, it'll still be in scope, no need to fetch from the params hash

SimonWoolf · 2018-04-17T13:06:19Z

lib/ably/rest/client.rb

            when Faraday::ClientError
-              raise Ably::Exceptions::ConnectionError.new(error.message, nil, 80000, error)
+              raise Ably::Exceptions::ConnectionError.new(error.message, nil, 80000, error, {}, request_id)
            else
              raise error


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.

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.

mattheworiordan

In addition to @SimonWoolf's feedback...

mattheworiordan · 2018-04-17T13:21:37Z

lib/ably/exceptions.rb


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


I would have preferred for request_id to be an option given it's a flexible hash for this reason.

Didn’t know the supposed usage of that hash, that’s why I didn’t touch it. Good to know that I can use it.

mattheworiordan · 2018-04-17T13:23:05Z

lib/ably/exceptions.rb

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


If we leave request_id as an option of options, then no change here is needed.

mattheworiordan · 2018-04-17T13:24:02Z

lib/ably/rest/client.rb

@@ -434,6 +439,10 @@ def send_request(method, path, params, options)
        max_retry_duration = http_defaults.fetch(:max_retry_duration)
        requested_at       = Time.now
        retry_count        = 0
+        if @add_request_ids


It's bad practice to read an instance variable like this when it's available as a reader i.e. if @add_request_idds would always be false due to a type, whereas if add_request_idds would fail.

mattheworiordan · 2018-04-17T13:25:29Z

lib/ably/rest/client.rb

          case error
            when Faraday::TimeoutError
-              raise Ably::Exceptions::ConnectionTimeout.new(error.message, nil, 80014, error)
+              raise Ably::Exceptions::ConnectionTimeout.new(error.message, nil, 80014, error, {}, request_id)


Based on my proposed change, it would be raise Ably::Exceptions::ConnectionTimeout.new(error.message, nil, 80014, error, { request_id: request_id }).
I think raise Ably::Exceptions::ConnectionTimeout.new(error.message, nil, 80014, error, request_id: request_id) may work too, although I recall 1.9.2 having some constraints with pseudo named args.

mattheworiordan · 2018-04-17T13:26:21Z

spec/unit/rest/client_spec.rb

@@ -50,4 +50,11 @@
      end
    end
  end
+
+  context 'request_id generation' do
+    let(:client_options) { {key: 'appid.keyuid:keysecret', add_request_ids: true} }


Unusual spacing, in all other places { has a space after, and } a space before

funkyboy · 2018-04-18T07:06:57Z

@SimonWoolf @mattheworiordan

added request_id for nonce errors
now passing request_id in the options param
added more tests for both nonce and timeout errors with add_request_ids true and false

Ready for another look.

mattheworiordan

Quite a few comments I am afraid.

mattheworiordan · 2018-04-18T09:58:57Z

lib/ably/rest/client.rb

@@ -449,6 +449,7 @@ def send_request(method, path, params, options)
          use_fallback = can_fallback_to_alternate_ably_host? && retry_count > 0

          connection(use_fallback: use_fallback).send(method, path, params) do |request|
+            request.options.context = {request_id: random_request_id} if add_request_ids


Inconsistent spacing here again.

Also, shouldn't this append/merge into the request options context in case there are other attributes already? This will always overwrite them which feels wrong and is error prone.

mattheworiordan · 2018-04-18T10:00:27Z

spec/acceptance/rest/client_spec.rb

@@ -961,6 +961,7 @@ def encode64(text)
    end

    context 'request_id generation' do
+      context 'Timeout error' do


Indentation issues here. Why did you add context & end without indenting the code?

mattheworiordan · 2018-04-18T13:46:48Z

spec/acceptance/rest/client_spec.rb

+          begin
+            client2.auth.request_token(token_params)
+          rescue Ably::Exceptions::UnauthorizedRequest => err
+            expect(err.request_id).to_not eql(nil)


Do we have any test to ensure that when add_request_ids is false, that the request_id is nil?

@mattheworiordan yes https://github.com/ably/ably-ruby/blob/add-request-id-to-path/spec/acceptance/rest/client_spec.rb#L975

mattheworiordan · 2018-04-18T13:48:08Z

lib/ably/rest/client.rb

+          path_with_params.query_values = {request_id: SecureRandom.hex(20)}
+          query = path_with_params.query
+          path = "#{path}?#{query}"
+        end


@SimonWoolf because this is Ruby and not Elixir. So the params object would be changed by just calling this method, which is bad.

mattheworiordan · 2018-04-18T13:49:00Z

lib/ably/rest/client.rb

@@ -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})


Spacing issues again, here and many times below

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?

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.

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

mattheworiordan · 2018-04-18T13:54:24Z

lib/ably/rest/middleware/exceptions.rb

@@ -27,7 +27,7 @@ def on_complete(env)
            end

            message = 'Unknown server error' if message.to_s.strip == ''
-            request_id = env.request.context[:request_id]
+            request_id = env.request.context[:request_id] if env.request.context


Spacing issues below.

mattheworiordan · 2018-04-18T13:55:08Z

lib/ably/rest/client.rb

            when Faraday::ClientError
-              raise Ably::Exceptions::ConnectionError.new(error.message, nil, 80000, error)
+              raise Ably::Exceptions::ConnectionError.new(error.message, nil, 80000, error, {}, request_id)
            else
              raise error


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.

mattheworiordan · 2018-04-18T15:21:50Z

@funkyboy what is going on with your indentation?

mattheworiordan

LGTM 👍

mattheworiordan · 2018-04-23T10:06:24Z

lib/ably/rest/client.rb

@@ -434,11 +439,22 @@ def send_request(method, path, params, options)
        max_retry_duration = http_defaults.fetch(:max_retry_duration)
        requested_at       = Time.now
        retry_count        = 0
+        request_id         = nil
+        if add_request_ids
+          params = {} if params.nil?


This is a tiny comment, but it seems unnecesary to dup an empty hash, so perhaps instead just:

params = if params.nil? {} else params.dup end

mattheworiordan · 2018-04-23T10:09:08Z

lib/ably/rest/client.rb

            when Faraday::ClientError
-              raise Ably::Exceptions::ConnectionError.new(error.message, nil, 80000, error)
+              raise Ably::Exceptions::ConnectionError.new(error.message, nil, 80000, error, { request_id: request_id })


May be nice for future us to add a note about the raise error not explicitly passing the request_id, but in the Middleware it's available through the requext context. WDYT?

Add request_id option to client

a6a0946

funkyboy requested a review from SimonWoolf April 17, 2018 07:41

SimonWoolf requested changes Apr 17, 2018

View reviewed changes

funkyboy added 4 commits April 17, 2018 11:27

Simplify options setting

fef93a1

Add request_id to connection error and timeouts

67d06b8

Remove path munging

1fc2ef8

Add ConnectionTimeout test with request_id

5d3666c

funkyboy requested a review from mattheworiordan April 17, 2018 12:39

SimonWoolf requested changes Apr 17, 2018

View reviewed changes

Use urlsafe_base64 and a local var for request_id

1670bb1

mattheworiordan reviewed Apr 17, 2018

View reviewed changes

funkyboy added 3 commits April 17, 2018 15:31

Add request_id to printed exception

9357a13

Fix spacing

55f18aa

Use attr_reader to access add_request_ids

a1f71d4

mattheworiordan mentioned this pull request Apr 17, 2018

Add request ID to client lib spec ably/docs#417

Closed

funkyboy added 9 commits April 18, 2018 08:03

Add request_id to Faraday request context

d39e11c

Read request_id from Faraday context

d9b6b8f

Reorganize request_id generation tests

3e55b9e

Add test for UnauthorizedRequest nonce

a061d3b

Fix frozen hash issue

c227120

Add test without request_id

890ced2

Rename test context

97febef

Pass request_id in Exception options

dc6f216

Fix fetching of request_id from Faraday context

844a938

mattheworiordan requested changes Apr 18, 2018

View reviewed changes

funkyboy added 2 commits April 18, 2018 17:27

Merge request_id with Faraday context options

a7f1e29

Fix spacing and indentation

66259dc

funkyboy added 22 commits April 20, 2018 13:03

Add request_id to connection error and timeouts

daacf46

Remove path munging

44d08b7

Add ConnectionTimeout test with request_id

d531f5a

Use urlsafe_base64 and a local var for request_id

24435b8

Add request_id to printed exception

4425fb8

Fix spacing

756b266

Use attr_reader to access add_request_ids

72eda17

Add request_id to Faraday request context

3308586

Read request_id from Faraday context

39a9b75

Reorganize request_id generation tests

eb87ad6

Add test for UnauthorizedRequest nonce

f0e3730

Fix frozen hash issue

8efa111

Add test without request_id

a34000f

Rename test context

df94980

Pass request_id in Exception options

2731ca7

Fix fetching of request_id from Faraday context

528c92a

Merge request_id with Faraday context options

7399f18

Fix spacing and indentation

48c6cfc

Rename random_request_id as request_id

c2ab83c

Declare request_id as a local var

705f143

Fix indentation

0b33cdd

Add test to check if request_id is the same at request and response time

8b87e5f

funkyboy force-pushed the add-request-id-to-path branch from f99e186 to 8b87e5f Compare April 20, 2018 11:18

Add test for request_id when fallback is on and the request is retried

749d49c

mattheworiordan approved these changes Apr 23, 2018

View reviewed changes

funkyboy added 2 commits April 23, 2018 16:19

Dup params only if needed

e17125c

Add note about request_id in the middleware request context

0774a2d

funkyboy merged commit 6b9358b into master Apr 23, 2018

funkyboy deleted the add-request-id-to-path branch April 23, 2018 14:26

SimonWoolf mentioned this pull request Nov 6, 2019

Add addRequestIds client option ably/docs#763

Merged


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

Add request_id option to client #133

Add request_id option to client #133

Conversation

funkyboy commented Apr 17, 2018 • edited Loading

SimonWoolf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattheworiordan Apr 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

funkyboy commented Apr 17, 2018

SimonWoolf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattheworiordan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

funkyboy commented Apr 18, 2018

mattheworiordan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattheworiordan Apr 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattheworiordan commented Apr 18, 2018 • edited Loading

mattheworiordan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

funkyboy commented Apr 17, 2018 •

edited

Loading

mattheworiordan Apr 18, 2018 •

edited

Loading

mattheworiordan Apr 18, 2018 •

edited

Loading

mattheworiordan commented Apr 18, 2018 •

edited

Loading