Elasticsearch-rails: Per-search HTTP timeout configuration

[nate00]

Background

You can configure an Elasticsearch::Client with an HTTP timeout:

Elasticsearch::Client.new(request_timeout: 60).response

That means that every search will have the same timeout:

MyModel.__elasticsearch__.search('hello').response  # has a 60s HTTP timeout

You can set the Elasticsearch timeout parameter on a per-search basis:

MyModel.__elasticsearch__.search('hello', timeout: '30s').response  # has a 60s HTTP timeout and a 30s Elasticsearch timeout

but that parameter sets the Elasticsearch timeout, not the HTTP timeout.

Feature Request

It would be great if we could configure the HTTP timeout differently for different searches. Maybe the search could accept a request_timeout option as part of a third argument:

MyModel.__elasticsearch__.search('hello', {}, {request_timeout: 30}).response

or perhaps an instance of Elasticsearch::Client:

MyModel.__elasticsearch__.search('hello', {}, {client: Elasticsearch::Client.new(request_timeout: 30)}).response

Thanks for the consideration 😃

[karmi]

Hi @nate00, thanks for the idea! I assume the reasoning for the different timeouts is that you want a small one for some requests (eg. indexing) and a large one for eg. complicated searches with aggregations and such?

Allowing passing request_timeout couldn't be done easily, because that is set when the client is initialized.

My usual way of thinking is that people can initialize multiple clients in this scenarios, but I understand that there's no convenient way of doing that in the Elasticsearch::Model context.

I like the idea of injecting the client like you suggest, because that would a) solve the problem, b) be transparent, c) easy to implement (we may add some with_client sugar to handle it nicely in the library).

One request: can you please open a new issue in the https://github.com/elastic/elasticsearch-rails repository, because that's the context where this needs to be implemented? I'll copy over my response there as well and we can continue discussion there.

---

Copied from https://github.com/elastic/elasticsearch-ruby/issues/416#issuecomment-287289401

[nate00]

@karmi Yeah, you described my use case basically correctly. I want to render a webpage that has search results as its main content (with a longer timeout) and search result counts for related search queries as secondary content (with a shorter timeout). It's not terrible if that secondary content is sometimes missing from the page.

[karmi]

So, I was thinking about it a little, and it occured to me, that instead of passing the client object as an argument to the search method, which is _really_ weird, we could follow the with_foo {} convention, used frequently in the Ruby world. In pseudo-code, something like this:

MyModel.__elasticsearch__
  .with_client( Elasticsearch::Client.new(request_timeout: 30)}) )
  .search('hello')
  .response

MyModel.__elasticsearch__
  .with_client do
    client = Elasticsearch::Client.new(request_timeout: 30)})
    client.search('hello')
  end
  .response

client = Elasticsearch::Client.new(request_timeout: 30)})

MyModel.__elasticsearch__
  .with_client do |client|
    client.search('hello')
  end
  .response

What do you think about that? Would you be interested in having a shot at the implementation, maybe? I can provide assistance there.

[nate00]

Agreed, I like that interface better.

Were you thinking the implementation would be something basically equivalent to:

class Elasticsearch::Model::Proxy::ClassMethodsProxy
  def with_client(client)
    duplicate_proxy = dup
    duplicate_proxy.client = client
    duplicate_proxy
  end
end

I think that would get us the functionality in your first example. Because we're only shallow copying the proxy, you get potentially surprising behavior like:

MyModel.index_name = 'foo'

MyModel.with_client(Elasticsearch::Client.new).index_name = 'foobar'
MyModel.index_name  #=> still 'foo'

MyModel.with_client(Elasticsearch::Client.new).index_name << 'bar'
MyModel.index_name  #=> changed to 'foobar'

I think it's unlikely anyone will want to mutate the index name like that. But maybe ClassMethodsProxy has some other state that will behave surprisingly when we shallow copy it?

About the block syntax in your second and third examples: is the idea that the search method, when executed within the block, would take configuration (like the index name) from MyModel.__elasticsearch__? I don't really like the behaves-differently-inside-a-block type of magic, but I'm happy to implement it for others to use.

[nate00]

The block version could work like:

MyModel.__elasticsearch__.with_client(Elasticsearch::Client.new) do |model_proxy|
  # model_proxy is an instance of ClassMethodsProxy
  model_proxy.search('hello').response
end

That's useful and not magical, I think. What do you think?

[karmi]

I was more or less just throwing the ideas for the usage/syntax around, without much thinking about the implementation...

I think it's crucial to start with something immediately usable, which solves the actual problem you have, and for that the first, non-block syntax seems like a good candidate? So I would leave out the block syntax for now.

I think that we'll have to hook into the SearchRequest#execute! method, extracting the klass.client part so we can temporarily use a different client instance for executing the request.

But my mind usually doesn't work well until I'm in the code, so I'd have to try it and see :)

[karmi]

I was playing with it a little bit inside a Pry session with the activerecord_article.rb example, and I think I can provide less confusing feedback now :)

So, without the block, predictably, we have no way of setting that different client _temporarily_, since we don't have any way of setting the client back to the original. Therefore, with_client is the same as client=, and that's definitely not what we want.

With the block, we have a nice way of setting the client back to the original value, but exactly as you describe in your last example, the lazy loading makes it a bit convoluted, since you have to call the #response method to actually execute the request with the "new" client still set.

What I have right now is this:

diff --git i/elasticsearch-model/lib/elasticsearch/model.rb w/elasticsearch-model/lib/elasticsearch/model.rb
index 2c395bd..7f7b3b9 100644
--- i/elasticsearch-model/lib/elasticsearch/model.rb
+++ w/elasticsearch-model/lib/elasticsearch/model.rb
@@ -68,7 +68,7 @@ module Elasticsearch
   #     # ...
   #
   module Model
-    METHODS = [:search, :mapping, :mappings, :settings, :index_name, :document_type, :import]
+    METHODS = [:with_client, :search, :mapping, :mappings, :settings, :index_name, :document_type, :import]

     # Adds the `Elasticsearch::Model` functionality to the including class.
     #
diff --git i/elasticsearch-model/lib/elasticsearch/model/client.rb w/elasticsearch-model/lib/elasticsearch/model/client.rb
index c1a9b4e..db34b78 100644
--- i/elasticsearch-model/lib/elasticsearch/model/client.rb
+++ w/elasticsearch-model/lib/elasticsearch/model/client.rb
@@ -28,6 +28,17 @@ module Elasticsearch
         def client=(client)
           @client = client
         end
+
+        def with_client(client, &block)
+          original_client = self.client
+
+          self.client = client
+          begin
+            instance_eval(&block) if block_given?
+          ensure
+           self.client = original_client
+          end
+        end
       end

       module InstanceMethods
@@ -54,6 +65,17 @@ module Elasticsearch
         def client=(client)
           @client = client
         end
+
+        def with_client(client, &block)
+          original_client = self.client
+
+          self.client = client
+          begin
+            instance_eval(&block) if block_given?
+          ensure
+           self.client = original_client
+          end
+        end
       end

     end

Then, I'm able to use a syntax like this in the user code:

articles = Article.with_client Elasticsearch::Client.new url: 'http://localhost:9250' do
  search('foo').response
end

Of course, the nice thing is that we're not limited to searches, and can call all the other methods, e.g.. create an index:

Article.with_client(Elasticsearch::Client.new url: 'http://localhost:9250') { create_index! force: true }

I've added the with_client method also on the instance level, so everything fits together well:

@article = Article.first
@article.__elasticsearch__.with_client(Elasticsearch::Client.new url: 'http://localhost:9250') { index_document }

How do you feel about this interface?

Importantly, many thanks for bringing this to my attention, @nate00, and for the mental ping-pong. It does help very much with keeping focus, fleshing out the ideas, and striving towards the implementation.

[nate00]

Thanks for the thoughts @karmi!

1. I definitely agree that our implementation should support methods other than search, e.g. create_index!.

2. I also agree that the implementation should support with_client on both model classes and model instances.

3. I don't love using instance_eval. I always find it a bit surprising when the value of self changes inside a block. For example, using the activerecord_article.rb example:

class ArticleSearcher
  attr_reader :title

  def initialize(title)
    @title = title
  end

  def response
    Article.__elasticsearch__.with_client(Elasticsearch::Client.new(timeout: 30)) do
      search(self.title).response
    end
  end
end

ArticleSearcher.new('foo').response  #=> raises NoMethodError: undefined method `title' for #<Elasticsearch::Model::Proxy::ClassMethodsProxy:0x007f006b64e138>

I find it surprising that self.title tries to call title on an Article proxy, not on an ArticleSearcher. (I only find it surprising because Ruby libraries rarely change a block's self, in my experience. If this were JavaScript, where such things are much more common, I wouldn't find a changing self/this at all surprising.)

If we wanted to preserve the value of self, we could just execute the block without calling instance_eval. Then the response method would have to change to:

class ArticleSearcher
  attr_reader :title

  def initialize(title)
    @title = title
  end

  def response
    Article.__elasticsearch__.with_client(Elasticsearch::Client.new(timeout: 30)) do
      # Note that I added `Article.`:
      Article.search(self.title).response
    end
  end
end

ArticleSearcher.new('foo').response  #=> {"took"=>3, ...}

We could also yield self to the block, because it might make the code more readable in some cases:

Article.first.__elasticsearch__.with_client(Elasticsearch::Client.new(timeout: 30)) do |article|
  article.index_document
end

1. We'll need to be careful to make sure that changing Article.client doesn't leak across threads. For example:

child_thread_logger = Logger.new(STDERR)
child_thread_logger.formatter = proc { |_,_,_,msg| "CHILD: #{msg}\n" }
child_client = Elasticsearch::Client.new(logger: child_thread_logger)
parent_thread_logger = Logger.new(STDERR)
parent_thread_logger.formatter = proc { |_,_,_,msg| "PARENT: #{msg}\n" }
parent_client = Elasticsearch::Client.new(logger: parent_thread_logger)

Article.__elasticsearch__.client = parent_client

thr = Thread.new do
  Article.__elasticsearch__.with_client(child_client) do
    sleep 2
    search('second query').response.to_json
  end
end

sleep 1
Article.search('first query').response.to_json
sleep 2
Article.search('third query').response.to_json

thr.join

Prints the following logs:

CHILD: GET http://localhost:9200/articles/article/_search?q=first+query [status:200, request:0.012s, query:0.007s]
CHILD: < {"took":7,"timed_out":false,"_shards":{"total":5,"successful":5,"failed":0},"hits":{"total":0,"max_score":null,"hits":[]}}
CHILD: GET http://localhost:9200/articles/article/_search?q=second+query [status:200, request:0.009s, query:0.003s]
CHILD: < {"took":3,"timed_out":false,"_shards":{"total":5,"successful":5,"failed":0},"hits":{"total":0,"max_score":null,"hits":[]}}
PARENT: GET http://localhost:9200/articles/article/_search?q=third+query [status:200, request:0.014s, query:0.003s]
PARENT: < {"took":3,"timed_out":false,"_shards":{"total":5,"successful":5,"failed":0},"hits":{"total":0,"max_score":null,"hits":[]}}

I would've expected "first query" to use the parent thread's client.

But I think this problem would be easy to fix with thread locals?

1. One last opinion that I hold less strongly. I still kinda prefer having the option to duplicate the proxy rather than temporarily change the proxy's client. Basically, I think that creating a new local variable has less potential for confusion than modifying a global. (The global I'm referring to is Article.__elasticsearch__.client.)

We could provide both options. If with_client is given a block, it (temporarily) replaces the global proxy's client. Otherwise, it returns a new proxy.

Some examples:

default_logger = Logger.new(STDERR)
default_logger.formatter = proc { |_,_,_,msg| "DEFAULT #{msg}" }
default_client = Elasticsearch::Client.new(logger: default_logger);

new_logger = Logger.new(STDERR)
new_logger.formatter = proc { |_,_,_,msg| "NEW #{msg}" }
new_client = Elasticsearch::Client.new(logger: new_logger);

Article.__elasticsearch__.client = default_client;


Article.search('query one').response    # DEFAULT

Article.
  with_client(new_client).
  search('query two').
  response                              # NEW

Article.search('query three').response  # DEFAULT

Article.with_client(new_client) do
  Article.search('query four').response
end                                     # NEW

Article.search('query five').response   # DEFAULT

Here's a patch that implements that suggestion (but doesn't address 3 or 4):

diff --git a/elasticsearch-model/lib/elasticsearch/model.rb b/elasticsearch-model/lib/elasticsearch/model.rb
index 2c395bd..7f7b3b9 100644
--- a/elasticsearch-model/lib/elasticsearch/model.rb
+++ b/elasticsearch-model/lib/elasticsearch/model.rb
@@ -68,7 +68,7 @@ module Elasticsearch
   #     # ...
   #
   module Model
-    METHODS = [:search, :mapping, :mappings, :settings, :index_name, :document_type, :import]
+    METHODS = [:with_client, :search, :mapping, :mappings, :settings, :index_name, :document_type, :import]

     # Adds the `Elasticsearch::Model` functionality to the including class.
     #
diff --git a/elasticsearch-model/lib/elasticsearch/model/client.rb b/elasticsearch-model/lib/elasticsearch/model/client.rb
index c1a9b4e..32803cd 100644
--- a/elasticsearch-model/lib/elasticsearch/model/client.rb
+++ b/elasticsearch-model/lib/elasticsearch/model/client.rb
@@ -28,6 +28,23 @@ module Elasticsearch
         def client=(client)
           @client = client
         end
+
+        def with_client(client, &block)
+          if block_given?
+            original_client = self.client
+
+            self.client = client
+            begin
+              instance_eval(&block)
+            ensure
+             self.client = original_client
+            end
+          else
+            copy = dup
+            copy.client = client
+            copy
+          end
+        end
       end

       module InstanceMethods
@@ -54,6 +71,23 @@ module Elasticsearch
         def client=(client)
           @client = client
         end
+
+        def with_client(client, &block)
+          if block_given?
+            original_client = self.client
+
+            self.client = client
+            begin
+              instance_eval(&block)
+            ensure
+             self.client = original_client
+            end
+          else
+            copy = client.dup
+            copy.client = client
+            copy
+          end
+        end
       end

     end

Thanks for the consideration, and sorry if this comment is unnecessarily long! Let me know what you think, and thanks for being so diligent about getting this interface right.

[phlegx]

Hi! Do this work also if I use Elasticsearch::Persistence::Model like described here: elasticsearch-persistence#the-activerecord-pattern?

[OpakAlex]

How i can have one small global timeout for connection? (it's possible at 21 century? or i need to wait 100 years more?)

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.