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.

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

Make config work with es8 by migrating to @elastic/elasticsearch #141

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

michaelkirk
Copy link
Contributor

@michaelkirk michaelkirk commented Jun 15, 2023

Here's the reason for this change 🚀

My rationale is in pelias/pelias#953

As part of updating to elasticsearch 8, I'm updating pelias from the deprecated elastic package to it's successor @elastic/elasticsearch which supports both es7 and 8 (and some older versions too, but we only care about 7+).

Note that though this works with both the es7 and es8 server, this is a backwards incompatible client change - old pelias code using the elasticsearch package will not be able to use these new configs. It is a pre-requisite of adopting the new @elastic/elasticsearch client.

If any change required for the existing pacakges is required before the transition, I'd guess you'd want to make a minor/patch "hot fix" release, forked from a previous tag, rather than from master.

Here's what actually got changed 👏

updated config to be compatible with @elastic/elasticsearch


Here's how others can test the changes 👀

Other than the tests passing, I'm not sure what else should be done. If you want to try running pelias/schema#488, you can vicariously test this code.


Two things I wanted to highlight.

env:
What is env for? As in: "env": "production". Though it lived in the esclient config object, it doesn't seem to have been used by the elasticsearch client. Is it some pelias specific thing? I deleted it.

log
@elastic/elasticsearch replaced their built in logging for a series of event emitters. I think the idea is you observe those events and logging is left as an excercise for the consumer:
https://www.elastic.co/guide/en/elasticsearch/client/javascript-api/current/observability.html

My best guess was to integrate these new @elastic/elasticsearch events with with pelias-logger. There's some setup involved which I extracted into a new module pelias-elasticsearch, which I'm now using everywhere rather than the @elastic/elasticsearch directly.

I'm not familiar with wintson - I assumed there was a way to configure it at runtime with an environment variable, but I haven't yet found evidence of that. Should we expose log parameters in this config? I haven't seen evidence of this being done historically, but maybe its more significant now that we'd be using it for all the elasticsearch related logging.

alternatives considered

  1. The logging integration I propose could instead live in the existing dbclient package. The only reason I didn't was, that module seemed concerned entirely with importing logic. In fact, pelias-api doesn't depend on that module at all. But we could change that.

  2. We could keep the current config format exactly as it is and translate to the new format on the fly. This would have to be done everywhere we create a new client, but if we accept that we need some sort of client factory method regardless (to set up logging) maybe it's not so bad. The advantage would be to leave this module in a backwards compatible state for a while longer at least.

  3. the new and old formats could coexist. It would be backwards compatible, but potentially confusing for the user.

"esclient": {
 ...,
 "hosts": [{
       "env": "development",
       "protocol": "http",
       "host": "localhost",
       "port": 9200
     }],
"nodes": ["http://localhost:9200"]
}

I'm happy to do whatever.

BREAKING: Support new elasticsearch configuration format.

As part of updating to elasticsearch 8, I'm updating pelias from the
deprecated `elastic` package to it's successor `@elastic/elasticsearch`
which supports both es7 and 8 (and some older versions too, but I think we
only care about 7+).

Note that this is a backwards incompatible change - old pelias code
using the `elasticsearch` package will not be able to use these new
configs.

apiVersion is determined by the client version, so it's no longer
necessary.

`keepAlive` is true by default, you need to set `agent: false` to
disable it:

    const client = new Client({
      node: 'http://localhost:9200',
      // Disable agent and keep-alive
      agent: false
    })

`hosts` is now `nodes` (or `node`) and has a different format.

Deleted parameters that aren't used in the new client:

 - `env`: I'm not sure what it's for. I suspect it's a pelias thing, not
   an elasticsearch thing, which seems like a conflation of the
   responsibilities of 'esclient config' vs 'pelias env mgmt'. Maybe I'm
   confused though.

 - `log`: The new elasticsearch client emits events instead of logging.
@michaelkirk michaelkirk marked this pull request as ready for review March 12, 2024 18:12
@michaelkirk
Copy link
Contributor Author

I've updated the description and reformatted my commit message. This is ready for review (though I wouldn't be surprised if you want to consider an alternative).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant