Add Elasticsearch notification#1452
Conversation
|
I optimized the module a bit, now the tests should succeed. I now also tested it on an authenticated multi-node cluster and tested the error handling, if elasticsearch returns errors itself. So from my site it's ready to review. The image 
|
|
Awesome, thanks for reviewing, I'll get to it the next few days! |
And feel free to add this screenshot in the docs page as well in a "Sample" section similar to https://crazymax.dev/diun/notif/mail/#sample or https://crazymax.dev/diun/notif/discord/#sample |
robin-moser
force-pushed
the
master
branch
2 times, most recently
from
24e0140 to
f0f141e
Compare
Elsasticsearch notification: add test cases Elasticsearch notification: make timeout configurable Elsasticsearch notification: add @timestamp field to JSON data Elsasticsearch notification: improve error handling use context.WithTimeoutCause Co-authored-by: CrazyMax <[email protected]> better comment the elaticsearch api endpoint add screenshot to documentation
|
@crazy-max |
crazy-max
left a comment
crazy-max
left a comment
There was a problem hiding this comment.
Thanks, left a review about the client, let me know what you think.
| // Build the Elasticsearch indexing URL | ||
| // This uses the Index API (POST /{index}/_doc) to create a document with an auto-generated _id: | ||
| // https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-create | ||
| url := fmt.Sprintf("%s://%s:%d/%s/_doc", c.cfg.Scheme, c.cfg.Host, c.cfg.Port, c.cfg.Index) |
There was a problem hiding this comment.
I looked a bit more at the docs and wonder if we could use their go client instead? https://github.com/elastic/go-elasticsearch
Also I think we should remove Scheme, Host and Port and replace them with an Address one that makes more sense looking at the API docs https://pkg.go.dev/github.com/elastic/go-elasticsearch#pkg-overview
So instead of:
elasticsearch:
scheme: https
host: localhost
port: 9200It would be:
elasticsearch:
address: https://localhost:9200Seems also they support multiple addresses, maybe we should too?
There was a problem hiding this comment.
These are good points. I initially oriented myself at the mqtt and webhook implementation and tried to stay as close to them as possible to have uniform code, but it makes sense to implement it properly with their client. I'll look into it, thanks for your input.
There was a problem hiding this comment.
I'd have to test, how forgiving their client is regarding elasticsearch version compatibility. The _doc endpoint is probably the most essential and basic endpoint, generally unchanged since version 5. So by using their go client, hopefully they don't restrict version compatibility, which would make it harder to maitain the integration.
I'll come with some tests, probably by the end of next week.
There was a problem hiding this comment.
Thanks, I pushed an extra commit to at least switch to Address field and also validate it using url package.
| s.Scheme = "http" | ||
| s.Host = "localhost" | ||
| s.Port = 9200 |
There was a problem hiding this comment.
http://localhost:9200 looks good as default looking at https://pkg.go.dev/github.com/elastic/go-elasticsearch#NewClient but I would prefer to just have an Address related to my previous comment.
crazy-max
left a comment
crazy-max
left a comment
There was a problem hiding this comment.
Getting this one in, we can look at using the official go client https://github.com/elastic/go-elasticsearch as follow-up.
2 participants
This PR adds the option to send the update information to an elasticsearch instance.
I used the message.RenderJSON to generate the same body which the webhook notifier uses, but to differentiate multiple
diuninstances who can send to the same Elasticsearch index, I added a configurableclientfield that get's also sent.I tested on a local one-node elasticsearch Cluster, I will test again the next days on a production Cluster.