Certain document hash keys are "special" and get converted into URL parameters rather than being indexed with the document.

These fields are added to a document. They get pulled out and added to the query
string (rather than being passed in a a parameter to the index method). Any unknown underscore fields are ignored and indexed as usual.

These URL parameters are different between versions. _consistency got re-implemented as _wait_for_active_shards and has different values, so it's not a direct mapping.

Additionally, ES 5 no longer allows unknown URL parameters. So we need to not send it or force the caller to deal with the error. And vice-versa, there is a problem as well: if someone uses a new
parameter from ES 5, it will end up in a document as a field unless we pull it out and put it on the URL as a parameter (which ES 2 will ignore), or pull it out and ignore it.

We need to decide how to handle these differences in the library.

References:

• ES 5 wait_for_active_shards
• ES 2 consistency
• Issue about the change

The field stats API is very handy for picking out min / max values for fields along with other metrics. We should add this to the client.

/cc @grantr @chrismwendt

The Elasticsearch 1.0 api is removing the ok attributes that have historically been used to detect successful responses. In several of these apis it is unclear how to detect a successful response from the response body.

I've resisted this so far, but I wonder if now might be the time to start returning a Response object instead of the raw response hash. My initial vision for this is as simple as possible:

class Response < Hash
  attr_accessor :status, :headers
end

This would allow api consumers to do things like:

response = docs.index(document)
if response.status != 201
   # fail
end

I'd avoid adding abstractions like success? unless there was a clear benefit from having them in the low level client. IMO those kinds of things should be at a higher level (even though we don't have a higher level yet). The Response object is necessary in the client because the information would otherwise be unretrievable.

/cc @github/search

My good friend andrewvc is writing a book on elasticsearch and interested in hearing how we use it within GitHub. Would anyone be interested in sparing an hour to chat with him about it?

We need to pull in new features from ES 5.X that are not yet supported by the gem. When the gem is used with an ES 2.X cluster, a sensible empty result should be returned from these methods.

• Delete by query docs @look #185
• Task management API docs @elireisman #187
• Raise IllegalArgument exceptions when returned from Elasticsearch #184
• Upgrades tp + github/github deployment(s) of elastomer-client to pull in VersionSupport shims for ES2~ES5 transition period @elireisman @look (multiple patches)
• Pattern for parameter validation @TwP here

As per these docs a number of underscored parameters (such as _routing and _retry_on_conflict) have been removed from certain Elasticsearch (ES) APIs as of ES 7.

However, it appears that this client will rewrite non-underscored versions of some of these params to underscored versions before forwarding them to ES:

That prevents consumers of this library from specifying a routing parameter to the Bulk Index API of ES 7+, because an attempted use of the API as shown below will fail because the routing parameter gets converted to _routing:

index.bulk do |request|
  request.index({ foo: "bar" }, routing: "123")
end

The error I've observed looks like this:

Elastomer::Client::IllegalArgument: {"root_cause"=>[{"type"=>"illegal_argument_exception", "reason"=>"Action/metadata line [1] contains an unknown parameter [_routing]"}], "type"=>"illegal_argument_exception", "reason"=>"Action/metadata line [1] contains an unknown parameter [_routing]"}

This code in haystack reaches into elastomer-client's Faraday object to set the logger:

if Rails.env.development?
    Haystack.elastomer_client.connection.response :logger, Rails.logger
  end

https://github.com/github/haystack/blob/master/config/initializers/elasticsearch.rb#L18-L20

This shouldn't be necessary. Elastomer::Client should provide a method to set a logger object.

Lots of nondeterministic test failures are caused by forgetting to refresh the index after writing or before reading. It would be convenient if elastomer-client had a setting which handled refreshing automatically.

For performance, it could be "smart" and only refresh if a write has happened since the last refresh.

Elastomer::Client#inspect is now printing documentation, which makes console use painful.

When I run script/test, I get the following error message:

bundler: command not found: testrb
Install missing gem executables with `bundle install`

This is despite a successful run of script/bootstrap:

Gem environment up-to-date
Installing hooks into /home/administrator/elastomer-client
Successfully installed hooks into /home/administrator/elastomer-client

I installed Ruby 2.2.4 using rbenv on Ubuntu 14.04.

Has anyone encountered this error before? I have not been able to determine if it is a missing gem issue or something else. Thank you!

The delete_by_query API is going away in ES 2.0 because of multiple issues (See https://www.elastic.co/blog/core-delete-by-query-is-a-plugin). The alternative is a combination scan and bulk delete.

We should make the change in elastomer-client now to prep for the ES 2.0 upgrade.

/cc @TwP

We need to do a sweep of the codebase and remove deprecated ES 1.X code paths in preparation for the ES 5.X upgrade work.

This is ambitious, but I would love to have a docs folder that walks a brand new user through the basics of ElasticSearch. The goal of this is a pedagogical tool where the user could learn by doing.

The docs would walk a user through setting up ElasticSearch, creating an index, adding documents, searching, etc. Topics would be introduced slowly and we could link to the ElasticSearch guide to help people figure out how to use that resource, too.

Just a thought. It's a big effort that would require some high-level planning to get going.

cc @github/search

Steps to reproduce:

git pull origin master
script/bootstrap
bundle exec rake test

...
/Users/chrisbloom7/src/github/elastomer-client/test/test_helper.rb:42:in `<top (required)>': No server available at http://localhost:9200 (RuntimeError)
rake aborted!

Triggered by call to $client.available?. When trying to connect with same params from script/console the same check returns true.

Doing some byebug spelunking I tracked it back to an unhandled ArgumentError: unknown keyword: write_timeout exception in

      @socket = BufferedIO.new(s, read_timeout: @read_timeout,
                               write_timeout: @write_timeout,
                               continue_timeout: @continue_timeout,
                               debug_output: @debug_output)

A Google search reveals the same error message reported in an issue in Webmock around the time Ruby 2.6 was in RC. bblimke/webmock#786 Webmock 3.5 was released to address it.

Is this used anywhere? I saw it used in an elastomer test and was surprised that it exists.

If it's not needed for legacy code, I say remove it. Better to be consistent with the ES action names and be clear that there is no 'add' action.

We should tighten our url generation so that nil parameters can't accidentally delete higher level objects.

>> es = Elastomer::Client.new(:url => "http://localhost:19200")
=> #<Elastomer::Client:0x007f91e31e1500 @url="http://localhost:19200", @host="localhost", @port=19200, @read_timeout=4, @open_timeout=2, @adapter=:excon>
>> es.index('hello').create({})
=> {"ok"=>true, "acknowledged"=>true}
>> es.index('hello').exists?
=> true
>> es.index('hello').docs.delete(:id => nil)
=> {"ok"=>true, "acknowledged"=>true}
>> es.index('hello').exists?
=> false

We ran into an issue where the encoding of a body string passed to a GET request was being changed by the underlying Excon Faraday adapter. The encoding was changed from UTF-8 to ASCII, and it caused exceptions further up the stack when we attempted to render analytics information about individual Elasticsearch requests.

Excon wraps the body of a request with a StringIO instance. This allows Excon to read the body in chunks and send those chunks in the HTTP request. Excon calls StringIO#binmode to force binary encoding such that the chunks are not affected by multibyte UTF-8 characters. The problem is that the encoding of the underlying String is also changed by this binmode call.

We should modify the Elastomer::Client#extract_body method to return frozen Strings. This will prevent this type of error, and others like it, from occurring in the future.

Scroller currently only supports a handful of parameters. This prevents things like query string parameters from working on scroll requests:

        @index       = opts.fetch(:index, nil)
        @type        = opts.fetch(:type, nil)
        @scroll      = opts.fetch(:scroll, '5m')
        @size        = opts.fetch(:size, 50)
        @search_type = opts.fetch(:search_type, nil)

The original opts just need to be propagated through the methods.

When I ran a scan/scroll query against a 0.90.5 cluster today, it was failing in a very odd fashion. The initial scan search request worked just fine, but the subsequent scroll request was failing with our beloved OpaqueIdError.

I tracked this down to a very large scroll ID being returned by ElasticSearch. When we send this very large scroll ID via a URL param, it is causes some sort of problem with haproxy or another part of our infrastructure - the URL is just too long. The solution is to send the scroll ID as the body of the request. The ES documentation for the scan/scroll queries actually recommend this usage.

@github/search

The Bulk helper can issue multiple requests behind the scenes if requested. In this case we should instrument the entire operation as a single action in addition to the underlying scan/bulk request actions.

Allowing the user of this client to put underscore-prefixed params (e.g. _routing) into the query document, then extracting them out before sending the request to Elasticsearch is confusing because it violates the "one-to-one mapping" property of this client. Elasticsearch itself doesn't support such underscore-prefixed params, so I'm curious why this behavior was introduced. This behavior is also inconsistent between Docs methods (e.g. get doesn't have this behavior, while index does).

The culprit is from_document, which implicates methods like docs.index.

After fighting against the Tire gem for the past year or so, I'm really craving a stupid simple ElasticSearch client that has a one-to-one correlation with the ElasticSearch API. When the github/id was started, I recommended using the eson gem. It is a beautifully crafted gem that provides the one-to-one mapping, but it is completely frustrating to use - lot's of clever code in there that is poorly documented.

The only clever thing I'm doing in this client code is to break down the ElasticSearch API into logical chunks. All the API calls that deal with cluster management are handled in a Client::Cluster class. All the API calls that deal with index management are handled in a Client::Index class.

I'd love to hear people's thoughts on this approach in general. Also having some more eyes on this code would be much appreciated.

After the client code is fleshed out, all of our search niceties from .com will be pulled into the gem so the code can be used in gists2 and other places. A short list of things to pull in:

• index management code (mappings, settings, creation, etc)
• adapter code for getting information out of ActiveRecord models and into searchable documents
• query parsing
• filter building

cc @github/search @jbarnette @wfarr

We need to add some logging to the Client class. For each request the client makes, we should output a detailed debug message to a logger of some sort. I really like how Tire does it; logging a curl command that you can copy/paste and use directly. Also, outputting the status code of the response would be good, too ...

[200] curl -XGET 'http://localhost:9200/index/_search' -d '...'

Not sure how to handle large bodies and what not. And I don't think we need to log responses.

cc @github/search

Now that we have a CI environment configured for Elasticsearch 5.6, we need to get all the unit tests passing for both ES 2.4 and ES 5.6. The end goal is that the elastomer-client gem be compatible with both versions while breaking as little backwards compatibility as possible.

After we have green tests we can proceed to pull in new features from ES 5.X into the gem.

Things To Fix

• Warmers aren't a thing anymore in 5.x @look #161
• :payloads at index-time are not support @juruen #155
• Scan/scroll changes @juruen #163
• Index mappings: string type becomes text for analyzed string fields, keyword for non-analyzed (1st to come up is title but there will be lots) @look #166
• filtered query deprecation (replaced with bool queries) @look #169
• misc field naming convention changes in mappings (.percolator) @elireisman #167
• analyzer needs to be passed as a json object in the body (in Index#analyze from Elastomer::Client::Index#test_0009_analyzes text and returns tokens) @juruen #168
• Suggest output removed (output in Elastomer::Client::Index::when an index exists#test_0014_performs suggestion queries) @look #173
• Expected param change (consistency in Elastomer::Client::Docs#test_0004_extracts underscore attributes from the document) @look #178
• seeing some errors using client accessor in Warmer tests now, probably just need to use test-global $client here? @elireisman #172
• Exceptions aren't raised the same way ([Elastomer::Client::QueryParsingError] exception expected, not Class: <Elastomer::Client::RequestError>) @look #175

To start the upgrade work for the elastomer-client gem, it would be very helpful to get an Elasticsearch 5.6 Travis CI build script up and running. We can then progressively make all the tests go green from there.

I am working through our list of breaking changes in ES 5.X and identifying what we need to update here in the gem. But a failing CI build will help that effort along.

If we're going to accept 3rd-party PRs, we need to build on travis because our internal ci is not secure. @github/atom-ci, you have experience running both travis and janky, and we'd welcome your thoughts here.

@bhuga and I talked today about possibly extending chatterbox with an org hook that watches build statuses, thus allowing us to do /chatterbox subscribe elastomer-client-status to get build statuses from travis in chat. No idea when or if that will happen, but I'm preserving it here just to note that it would be ✨.

/cc @github/search

Depends on #211

Ensure any authorization credentials or tokens are filtered/excluded when an Elastomer::Client instance is inspected.

The delete_by_query call should exclude the document source from being retrieved when performing the scan operation. We only need the index, type, and id of the documents in order to delete them. Excluding source retrieval will reduce load on the search cluster, and it will reduce the size of our HTTP packets.

@query = query.dup
@query.delete :_source if @query.key? :_source
@query.delete "_source" if @query.key? "_source"
@query[:_source] = false 

Example implementation to put into the DeleteByQuery initializer.

/cc @chrismwendt @grantr

Today I tried to do a bulk transfer in haystack and ran into issues using the action_count bulk chunking feature. Turns out the version in haystack wasn't new enough to have it! But I had to look at the gem source to find out.

Even if we're not 1.0 quality yet, we should still have 0.x version numbers and tagged releases to be a little more obvious with features.

It would be great to wrapper Faraday::Error with Elastomer::Error so that users can catch all exceptions via the single error class.

refs https://github.com/github/github/pull/30758#discussion_r17155213

/cc @grantr @mattyoho @ymendel

I have just a few query types and no way to correlate them to the notifications that come back for measuring. For example, 'Get me the list of all users who have spoken in chat rooms you can search' is a very different query from 'Get me the messages that match this query'.

It would be great if the params on Docs#search would pass through to the payload in ActiveSupport::Notifications, so that I could do something like:

res = Chat::ES.messages_client.search(users_query, search_type: :count, category: :users)

And later:

ActiveSupport::Notifications.subscribe('request.client.elastomer') do |name, start_time, end_time, _, payload|

  puts ":tada:" if payload[:category] == :users
  # Or maybe payload[:params][:category]. yknow whatevs.
end

ES has a well defined API spec here: https://github.com/elasticsearch/elasticsearch/tree/master/rest-api-spec/api

We should test our API coverage against specific versions. On that note, we should probably explicitly note which versions we support (where support means we strive for 100% coverage) and which we don't (coverage might be incomplete). We could even add methods in the client for determining whether the connected version of ES is officially supported.

Chatting today with Drew Raines of ElasticSearch™ the company, and he recommended we use the X-Opaque-Id request header when communicating with the server. This header will be parroted back in the response headers. So we can validate that the request / response truly belong to each other. Here is a gist from Drew showing the usage.

https://github.com/github/github/issues/16381

When the read_timeout is reached in Elastomer::Client, an exception is raised and the Ruby code aborts the request handling process. The Elasticsearch cluster will continue processing the search request and will eventually send a response - the HTTP connection is still open. Subsequent calls using this same connection can return search results for a previous request that timed out. This scenario is the reason that the OpaqueId middleware exists.

A more proactive solution would be to not reuse the connection when a read timeout is reached. Instead, the connection should be discarded and a new connection established. This will prevent subsequent search requests from triggering the OpaqueId error condition.

/cc @grantr @tmm1

Percolate is not supported yet, but since it's changing completely in 1.0, maybe we should wait until that is released.

A friend of mine using percolate in production describes it as an ops nightmare currently, so I doubt we'll be needing it until the 1.0 revamp.

Elasticsearch 1.0 breaks a bunch of stuff. Current list of changes is at http://www.elasticsearch.org/guide/en/elasticsearch/reference/master/breaking-changes.html.

As far as I can tell, the following will require changes to Elastomer or github.com:

New stats url format

The stats urls now rely on url path instead of parameters for filtering. http://www.elasticsearch.org/guide/en/elasticsearch/reference/master/_stats_and_info_apis.html

New admin api url formats

Some of the admin url formats have changed. http://www.elasticsearch.org/guide/en/elasticsearch/reference/master/_indices_apis.html

Cannot index document with type wrapper

I don't think we ever did this, but if we did it's not allowed by default anymore. http://www.elasticsearch.org/guide/en/elasticsearch/reference/master/_index_request.html

All search apis take a :query element in the body

Yay! This was the source of much ambiguity (and a few Elastomer bugs). http://www.elasticsearch.org/guide/en/elasticsearch/reference/master/_search_requests.html

Top level :filter element renamed to :post_filter

We used to use this heavily in dotcom, less so now. http://www.elasticsearch.org/guide/en/elasticsearch/reference/master/_search_requests.html

New multifield syntax

I believe we use these in a few places in dotcom. Minor syntax change. http://www.elasticsearch.org/guide/en/elasticsearch/reference/master/_multi_fields.html

No english stopwords in standard and pattern analyzers

If we want to continue using stopwords, we'll have to add a stopword filter manually. http://www.elasticsearch.org/guide/en/elasticsearch/reference/master/_stopwords.html

New fuzzy query parameters

I think we use fuzzy queries for autocomplete queries. http://www.elasticsearch.org/guide/en/elasticsearch/reference/master/_parameters.html

No more ok elements in responses

I think haystack relies on this, and maybe some tests and other things? http://www.elasticsearch.org/guide/en/elasticsearch/reference/master/_return_values.html

That's what stood out to me. Everyone working with ES should take a look and see if they'll have to change anything.

/cc @github/search

An Issue to track updates to elastomer-client lib to shim various API calls for the 2.x to 5.x ES upgrade. I ran the test suite against a localk ES 5.6 docker image and I get 69 fails that we can bucket into these fixes so far (WIP, I'll be updating and refining this list as we go):

• Warmers aren't a thing anymore in 5.x
• Scan/scroll changes
• Index mappings: string type becomes text for analyzed string fields, keyword for non-analyzed (1st to come up is title but there will be lots)
• Strict boolean field values
• misc field naming convention changes in mappings (.percolator, _type)
• Index mappings: invalid params (suggest's payload param came up 1st but there will be more)

@TwP @look

@github/platform-data

We've monkey patched our local elastomer wrapper to inject basic auth credentials into the URL that is ultimately sent to the elastomer client. This seems like a useful feature that would be easy enough to add to the client directly and trigger if a basic_auth or token_auth param was passed as a parameter to the initializer

For better performance, consider using the Elasticsearch Transport which handles connecting to multiple nodes in the cluster, rotating across connections, logging and tracing requests and responses, maintaining failed connections, discovering nodes in the cluster, and provides an abstraction for data serialization and transport.

This should also be able to allow the use of Excon, Patron and other adapters in a similar manner to what you have which are supported with Elasticsearch's official client

The hot_threads test fails nondeterministically, which causes false negatives during builds.

Elastomer::Client::Nodes#test_0005_gets the hot threads for the node(s) [/var/lib/jenkins/workspace/elastomer-client/test/client/nodes_test.rb:44]:
Expected /cpu usage by thread/ to match "

Seems like a timing issue. Probably need to increase timeouts.

In the recent 3.2.0 release, passing in a param of basic_auth: { username: nil, password: nil } will trigger basic auth. We should make sure it is only triggered if there is a username and password present. This will be helpful for folks that are defining their params like so:

params = {
  # ...
  basic_auth: {
    username: ENV["basic_auth_username"],
    password: ENV["basic_auth_password"]
  }
}

where the ENV vars may be blank or unset in some environments.

Currently one has to do something like this:

params = {
  # ...
}

if %w[basic_auth_username basic_auth_password].any? { |v| ENV[v].present? }
  params[:basic_auth] = {
    username: ENV["basic_auth_username"],
     password: ENV[" basic_auth_password"]
  }
end

# or 

params = {
  # ...
  basic_auth: if %w[basic_auth_username basic_auth_password].any? { |v| ENV[v].present? }
                {
                  username: ENV["basic_auth_username"],
                  password: ENV[" basic_auth_password"]
                }
              end
}

Not having elastomer-client on rubygems is a bit of a hassle for gems like elastomer-indexing and elastomer-cli. These gems have to vendor all their dependencies because elastomer-client isn't published.

I don't see a reason not to publish elastomer-client, or any of the elastomer-* gems. I'd like to publish them all to rubygems going forward. We can keep the repositories private, but there's nothing secret in the code.

@TwP what do you think?

#68 introduced version-specific behavior to a few methods. Since this might happen more often, we should think of a clean way to do version-specific stuff that avoids version checks everywhere. /cc @TwP @jnunemaker

The Cluster#update_aliases method assumes that the first parameter is an array of alias actions. If I give it the request body as expected by ES (a hash with an :actions key), the request fails because update_aliases adds a wrapper {:actions => [<parameters>]}.

This is counterintuitive. I expect the update_aliases command to accept the request body documented in the ES docs.

I think this is a pretty easy fix: don't add the wrapper if the parameter is a hash with an :actions key.

Also, the get_aliases and update_aliases methods should have their own tests in test/client/cluster_test.rb. They are currently only tested in test/client/index_test.rb.
/cc @TwP

There are a few esoteric API calls that are not yet implemented. Just putting an issue in here that these need to be taken care of.

The list of missing API calls are found in the source code in the client/index.rb file and the client/docs.rb file. Those lists are taken from the right-hand menu on the ElasticSearch API reference.

cc @github/search

Ruby 2.1 requires Faraday 0.9.0.rc7 (as implied by this commit: lostisland/faraday@87950cd).

In faraday 0.9.0.rc7, loading elastomer in faraday mode fails:

/Users/ben/github/chat/vendor/bundle/ruby/2.1.0/gems/faraday-0.9.0.rc7/lib/faraday.rb:99:in `method_missing': undefined method `register_middleware' for #<Faraday::Connection:0x007fcccfc815b0> (NoMethodError)
    from /Users/ben/github/chat/vendor/bundle/ruby/2.1.0/gems/elastomer-0.2.0/lib/elastomer/middleware/opaque_id.rb:68:in `<top (required)>'
    from /Users/ben/github/chat/vendor/bundle/ruby/2.1.0/gems/elastomer-0.2.0/lib/elastomer/client.rb:255:in `require'
    from /Users/ben/github/chat/vendor/bundle/ruby/2.1.0/gems/elastomer-0.2.0/lib/elastomer/client.rb:255:in `block in <top (required)>'
    from /Users/ben/github/chat/vendor/bundle/ruby/2.1.0/gems/elastomer-0.2.0/lib/elastomer/client.rb:255:in `each'
    from /Users/ben/github/chat/vendor/bundle/ruby/2.1.0/gems/elastomer-0.2.0/lib/elastomer/client.rb:255:in `<top (required)>'
    from /Users/ben/github/chat/lib/chat.rb:8:in `require'
    from /Users/ben/github/chat/lib/chat.rb:8:in `<top (required)>'
    from /Users/ben/github/chat/config/application.rb:3:in `require'
    from /Users/ben/github/chat/config/application.rb:3:in `<top (required)>'

cc @mislav @github/chat

We are following GitHub's ruby style guide, but there are many places in the codebase where the style guide has been ignored:

• whitespace between parentheses and values
• single character variable names should be eschewed
• quotes "good" should be used for strings instead of apostrophes 'bad'
• hashrockets => should be used instead of the JSON style : in hashes
• trailing whitespace

Please add others to this list so I can do a sweep and fix my wayward coding style.

/cc @grantr @chrismwendt

Normally a bulk request returns the bulk response:

client.bulk do |bulk_request|
  bulk_request.index({"foo" => "bar"}, :id => '1', :index => 'test', :type => 'test')
end

=> {"took"=>61, "errors"=>false, "items"=>[{"index"=>{"_index"=>"test", "_type"=>"test", "_id"=>"1", "_version"=>1, "status"=>201}}]}

When using the :action_count or :request_size options to chunk bulk requests behind the scenes, the response is lost:

client.bulk(:action_count => 1) do |bulk_request|
  bulk_request.index({"foo" => "bar"}, :id => '1', :index => 'test', :type => 'test')
  bulk_request.index({"foo" => "bar"}, :id => '2', :index => 'test', :type => 'test')
end

=> nil

I think when :action_count or :request_size are used, the return value should be an array of response hashes.

I can't decide what the return value should be if no requests are made. The non-chunking bulk returns nil, but maybe the chunking version should return an empty array.

Is it worth making the multiple-request bulk helper a separate class? We use it heavily, so changing it would be painful, but it has opinions about how to handle multiple requests that maybe don't belong in the single-request bulk helper. Perhaps we could move it to a utility gem and supplement it with other tools like a standard scan/bulk looper.

We may need some compatibility code to maintain backward compatibility with 0.90 while we transition to 1.0. Ideally this compatibility layer would connect to both versions of ES, and work with code expecting either version.

Here's a list of compatibilities that might be useful:

• where ok was removed from responses, put it back
• where exists changed to found in responses, put it back alongside found
• allow :query top-level element or not in count, validate, and delete_by_query
• return both flat and nested settings from get_settings in the same response
• return mappings with the "mappings" element and without in the same response
• translate field query to query string

Would this be useful? We don't have to implement all of these. Some things could be upgraded before the transition, like removing ok and field queries.

/ref https://github.com/github/search/issues/12
/cc @github/search