Support connecting to multiple versions elasticsearch at runtime #2260
Conversation
ffe0e99 to
e693bfa
Compare
|
Hi, the changes has been tested (thanks @pradeepbbl). And it seems working fine. Also updated doc for how to update config files. |
|
Hey @kylebrandt, @captncraig! |
|
There was some refactoring that has been done that will require this to be updated (how miniprofiler is done). I'm not entirely clear on how the config is a broking change based on the documentation - is it only the Version field or is there more to it? Since the vendoring is so big, and 2.x is EOL (see https://www.elastic.co/support/eol) can we yank that? Supporting Multiple ES version will be nice and make upgrading easier on users. |
|
Hi @kylebrandt
No problem. I could rebase master and update it one more time.
Because before this change, bosun could only talk to one version of es server but now with multiple version support if we default it to any of the versions (2/5/6), it might break the other two when the es query is run because of wrong version of es client. Users would wonder what breaks it, also arguably harder to debug. Therefore I explicitly panic the program on start up so users know that they have to update the configuration after upgrading the program. |
|
A bit late but really happy maintainers finally come to this PR. |
|
Pushed the update and all seems working (no compile or test errors). Didn't drop v2 support yet because I am not sure if all maintainers are onboard with it. |
|
@stepashka Does Booking still care about Bosun support ES 2.x? |
|
Thank you for checking, @kylebrandt! :) Yes, we still have a couple of ES 2.* clusters :( |
cmd/bosun/conf/system_elastic_all.go
Outdated
| case expr.ESV6: | ||
| cfg = parseESConfig6(value) | ||
| default: | ||
| panic(fmt.Errorf("conf: invalid elastic version: %s (supported versions are: v2, v5, v6)", value.Version)) |
There was a problem hiding this comment.
slog.Fatal is more appropriate (exit with error code, no stack trace). panic is more for programming errors and not user configuration errors.
cmd/bosun/conf/system_elastic_all.go
Outdated
| case expr.ESV6: | ||
| cfg = parseESConfig6(ElasticConf(sc.AnnotateConf)) | ||
| default: | ||
| panic(fmt.Errorf("conf: invalid elastic version: %s (supported versions are: v2, v5, v6)", sc.AnnotateConf.Version)) |
There was a problem hiding this comment.
same here with panic vs fatal
|
Can bump the middle the number of Version in version.go since this will be a breaking change. |
cmd/bosun/conf/system_elastic_all.go
Outdated
| "fmt" | ||
|
|
||
| "bosun.org/cmd/bosun/expr" | ||
| "github.com/bosun-monitor/bosun/slog" |
There was a problem hiding this comment.
import here needs to be bosun.org/bosun/slog
There was a problem hiding this comment.
Typo? You mean to write "bosun.org/slog", right?
|
I noticed that the Fatal call doesn't happen until you call an elastic expression. If we are going to have that, I think it should be in cmd/bosun/conf/system.go loadSystemConfig so it happens at start. |
|
Nice catch. I decided to do the pre-check in main because the get APIs don't return errors and loadSystemConfig does. |
cmd/bosun/conf/system_elastic_all.go
Outdated
| cfg = parseESConfig6(value) | ||
| default: | ||
| panic(fmt.Errorf("conf: invalid elastic version: %s (supported versions are: v2, v5, v6)", value.Version)) | ||
| slog.Fatal(fmt.Errorf("conf: invalid elastic version: %s (supported versions are: v2, v5, v6)", value.Version)) |
There was a problem hiding this comment.
For this and the annotate error we should include:
- If it is for annotate or elastic, and also in the case of elastic which prefix it is (i.e. "default" or another named instance).
- I think since this is a breaking change, we should add a case for an empty version and return an error message saying that is a required parameter (including the info from the above bullet point)
cmd/bosun/conf/system_elastic_all.go
Outdated
| case "": | ||
| slog.Fatal(fmt.Errorf("conf: [AnnotateConf]: Version is required (supported versions are: v2, v5, v6)")) | ||
| default: | ||
| slog.Fatal(fmt.Errorf("conf: [AnnotateConf]: invalid elastic version: %s (supported versions are: v2, v5, v6)", value.Version)) |
There was a problem hiding this comment.
@kylebrandt how about this for the error message?
There was a problem hiding this comment.
Good, I would just change Version is required a field (supported values for Version are: "v2", "v5", and "v6")
Because: "Version" is a pretty generic word, so clarify that is a field, and since the values are strings, putting them in quotes maybe helps prevent a future Toml error.
|
The last thing on my list is optional (and kind of annoying request). That is basically to create a new PR with two commits:
The reason is that:
I'm saying it is optional because I really appreciate your hard work and patience. So if you don't have the time for copying around all the files or git magic to rewrite timestamps with a rebase, then I am fine accepting option 2 and just squashing it all into one. |
|
@kylebrandt No worries, I have pushed the changes and created another PR. Hope it's done as expected. :) |
|
Really thank you for the review too. It's lots of work and nice suggestions! |
|
done via #2316 |
With this patch bosun could connect to elasticsearch servers with different versions. Supported version are: 2.x, 5.x, 6.x. This is a breaking change, requiring users to update their configuration files to explicitly specify elasticsearch version with 2, 5, or 6, otherwise it would just panic (fail fast).
No compile and test errors on my machine, but I haven't test it against real elastic search servers. Publish it here to check out the community's opinions and the possibility of merging onto master. :)
Relevant tickets: #2240, #1982