google / slo-generator

SLO Generator computes SLIs, SLOs, Error Budgets and Burn Rates from supported backends, then exports an SLO report to supported targets.
Apache License 2.0
489 stars 78 forks source link

Running `make unit` in a fresh cloned repo fails on `test_compute_elasticsearch` #222

Closed lvaylet closed 2 years ago

lvaylet commented 2 years ago

Steps to reproduce (with Python 3.9.12):

$ git clone git@github.com:google/slo-generator.git
$ cd slo-generator
$ python -m venv venv/
$ source venv/bin/activate
$ make develop
$ make unit

[...]

======================================================================
ERROR: test_compute_elasticsearch (tests.unit.test_compute.TestCompute)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/local/google/home/lvaylet/workspace/github/google/slo-generator/venv/lib/python3.9/site-packages/mock/mock.py", line 1346, in patched
    return func(*newargs, **newkeywargs)
  File "/usr/local/google/home/lvaylet/workspace/github/google/slo-generator/tests/unit/test_compute.py", line 104, in test_compute_elasticsearch
    compute(config, CONFIG)
  File "/usr/local/google/home/lvaylet/workspace/github/google/slo-generator/slo_generator/compute.py", line 68, in compute
    report = SLOReport(config=slo_config,
  File "/usr/local/google/home/lvaylet/workspace/github/google/slo-generator/slo_generator/report.py", line 114, in __init__
    data = self.run_backend(config, backend, client=client, delete=delete)
  File "/usr/local/google/home/lvaylet/workspace/github/google/slo-generator/slo_generator/report.py", line 206, in run_backend
    instance = cls(client=client, **backend_cfg)
  File "/usr/local/google/home/lvaylet/workspace/github/google/slo-generator/slo_generator/backends/elasticsearch.py", line 41, in __init__
    self.client = Elasticsearch(**es_config)
TypeError: __init__() got an unexpected keyword argument 'url'

[...]

Ran 42 tests in 2.785s

FAILED (errors=1)
make: *** [Makefile:69: unit] Error 1
lvaylet commented 2 years ago

The test suite completes successfully after pinning elasticsearch to version 7.17.1. The latest 8.x.x release (8.1.1 at the time of writing) probably introduces a breaking change in how the ElasticSearch client is created and configured.

The release notes do not provide any specific migration instructions. However, the Overview pages of versions 7.17 and 8.1 show clear differences in how the client is created and configured:

Overview 7.17

>>> from elasticsearch import Elasticsearch

# By default we connect to localhost:9200
>>> es = Elasticsearch()

Overview 8.1

>>> from elasticsearch import Elasticsearch

# Connect to 'http://localhost:9200'
>>> es = Elasticsearch("http://localhost:9200")
lvaylet commented 2 years ago

@ocervell Passing the url entry of the es_config dictionary to the Elasticsearch client (instead of the whole dictionary) is a simple fix for this bug. However I am not sure about the impact on current deployments.

class ElasticsearchBackend:
    def __init__(self, client=None, **es_config):
        self.client = client
        if self.client is None:
+            self.client = Elasticsearch(es_config['url'])
-            self.client = Elasticsearch(**es_config)

The Elasticsearch client documentation states that:

Language clients are forward compatible; meaning that clients support communicating with greater or equal minor versions of Elasticsearch. Elasticsearch language clients are only backwards compatible with default distributions and without guarantees made.

WDYT?