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.

Sign up for GitHub

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

Jump to bottom

feat(elasticsearch-logger): support multi elasticsearch endpoints #8604

Merged
merged 4 commits into from
Feb 1, 2023
Merged

feat(elasticsearch-logger): support multi elasticsearch endpoints #8604

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7878c23
support multi elasticsearch endpoints
xiaoxuanzi Jan 3, 2023
fb5cb48
add a new field endpoint_addrs
xiaoxuanzi Jan 29, 2023
c009c92
add a test to show that different endpoints will be chosen randomly
xiaoxuanzi Jan 30, 2023
3ab6051
adjusted TEST 14
xiaoxuanzi Jan 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 29 additions & 6 deletions apisix/plugins/elasticsearch-logger.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,18 +23,35 @@ local plugin = require("apisix.plugin")

local ngx = ngx
local str_format = core.string.format
local math_random = math.random
local type = type

local plugin_name = "elasticsearch-logger"
local batch_processor_manager = bp_manager_mod.new(plugin_name)


local schema = {
type = "object",
properties = {
endpoint_addr = {
local endpoint_schema = {
anyOf = {
{
-- deprecated, use "array" instead
type = "string",
pattern = "[^/]$",
},
{
type = "array",
minItems = 1,
items = {
type = "string",
pattern = "[^/]$",
},
}
}
}

local schema = {
type = "object",
properties = {
endpoint_addr = endpoint_schema,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to use endpoint_addrs with the new type.
Using different types in the same field brings us lots of trouble when we need to work with static-type language, like writing a Go client.

Copy link
Contributor Author

@xiaoxuanzi xiaoxuanzi Jan 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @spacewander , thank you for your reply.
In my opinion, to use endpoint_addrs will cause trouble, this change is not backward compatible.

The code submitted for the first time is modified as follows:

 local schema = {
     type = "object",
     properties = {
+        -- deprecated, use "endpoint_addrs" instead
         endpoint_addr = {
             type = "string",
             pattern = "[^/]$",
         },
+        endpoint_addrs = {
+            type = "array",
+            minItems = 1,
+            items = {
+                type = "string",
+                pattern = "[^/]$",
+            },
+        },

I fixed the code according to @soulbird's suggestion.
#8604 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep endpoint_addr, add a new field endpoint_addrs to support both string and array better

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change is not backward compatible

I am confused. People can still use the old endpoint_addr configuration.

Keep endpoint_addr, add a new field endpoint_addrs to support both string and array better

It is not a good idea to have a field of multiple types. The upstream's nodes field is an example. People keep complaining that this field doesn't have a consistent type.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood. So we need to keep both endpoint_addr and endpoint_addrs, where endpoint_addrs is only specified as array type, is that right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

field = {
type = "object",
properties = {
Expand Down Expand Up @@ -68,7 +85,7 @@ local schema = {
}
},
encrypt_fields = {"auth.password"},
required = { "endpoint_addr", "field" },
required = {"endpoint_addr", "field"},
}


Expand Down Expand Up @@ -127,7 +144,13 @@ local function send_to_elasticsearch(conf, entries)
return false, str_format("create http error: %s", err)
end

local uri = conf.endpoint_addr .. "/_bulk"
local selected_endpoint_addr
if type(conf.endpoint_addr) == "string" then
selected_endpoint_addr = conf.endpoint_addr
else
selected_endpoint_addr = conf.endpoint_addr[math_random(#conf.endpoint_addr)]
end
local uri = selected_endpoint_addr .. "/_bulk"
local body = core.table.concat(entries, "")
local headers = {["Content-Type"] = "application/x-ndjson"}
if conf.auth then
Expand Down
4 changes: 3 additions & 1 deletion docs/en/latest/plugins/elasticsearch-logger.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ When the Plugin is enabled, APISIX will serialize the request context informatio

| Name | Type | Required | Default | Description |
| ------------- | ------- | -------- | --------------------------- | ------------------------------------------------------------ |
| endpoint_addr | string | True | | Elasticsearch API. |
| endpoint_addr | string/array | True | | Elasticsearch API. |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a deprecated mark like https://github.com/apache/apisix/blob/release/2.15/docs/en/latest/plugins/mqtt-proxy.md#attributes

| field | array | True | | Elasticsearch `field` configuration. |
| field.index | string | True | | Elasticsearch [_index field](https://www.elastic.co/guide/en/elasticsearch/reference/current/mapping-index-field.html#mapping-index-field). |
| field.type | string | False | Elasticsearch default value | Elasticsearch [_type field](https://www.elastic.co/guide/en/elasticsearch/reference/7.17/mapping-type-field.html#mapping-type-field). |
Expand All @@ -53,6 +53,8 @@ This Plugin supports using batch processors to aggregate and process entries (lo

## Enabling the Plugin

If multiple endpoints are configured, they will be written randomly.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be put into the attribute table


### Full configuration

The example below shows a complete configuration of the Plugin on a specific Route:
Expand Down
8 changes: 7 additions & 1 deletion docs/zh/latest/plugins/elasticsearch-logger.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ description: 本文介绍了 API 网关 Apache APISIX 的 elasticsearch-logger

| 名称 | 类型 | 必选项 | 默认值 | 描述 |
| ------------- | ------- | -------- | -------------------- | ------------------------------------------------------------ |
| endpoint_addr | string | 是 | | Elasticsearch API。 |
| endpoint_addr | string/array | 是 | | Elasticsearch API。 |
| field | array | 是 | | Elasticsearch `field`配置信息。 |
| field.index | string | 是 | | Elasticsearch `[_index field](https://www.elastic.co/guide/en/elasticsearch/reference/current/mapping-index-field.html#mapping-index-field)`。 |
| field.type | string | 否 | Elasticsearch 默认值 | Elasticsearch `[_type field](https://www.elastic.co/guide/en/elasticsearch/reference/7.17/mapping-type-field.html#mapping-type-field)` |
Expand Down Expand Up @@ -93,6 +93,12 @@ curl http://127.0.0.1:9180/apisix/admin/routes/1 \
}'
```

:::note 注意

如果配置多个 `endpoints`，日志将会随机写入到各个 `endpoints`。

:::

### 最小化配置示例

```shell
Expand Down
38 changes: 37 additions & 1 deletion t/plugin/elasticsearch-logger.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ passed
property "endpoint_addr" is required
property "field" is required
property "field" validation failed: property "index" is required
property "endpoint_addr" validation failed: failed to match pattern "\[\^/\]\$" with "http://127.0.0.1:9200/"
property "endpoint_addr" validation failed: object matches none of the required



Expand Down Expand Up @@ -515,3 +515,39 @@ apisix:
--- response_body
123456
PTQvJEaPcNOXcOHeErC0XQ==



=== TEST 13: add plugin on routes using multi elasticsearch-logger
--- config
location /t {
content_by_lua_block {
local t = require("lib.test_admin").test
local code, body = t('/apisix/admin/routes/1', ngx.HTTP_PUT, {
uri = "/hello",
upstream = {
type = "roundrobin",
nodes = {
["127.0.0.1:1980"] = 1
}
},
plugins = {
["elasticsearch-logger"] = {
endpoint_addr = {"http://127.0.0.1:9200", "http://127.0.0.1:9201"},
field = {
index = "services"
},
batch_max_size = 1,
inactive_timeout = 1
}
}
})

if code >= 300 then
ngx.status = code
end
ngx.say(body)
}
}
--- response_body
passed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to add a test to show that different endpoints will be chosen randomly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with you, but I don't know how.
Can you give an example?
I take PR #7517 as reference.
=== TEST 5: add plugin on routes using multi clickhouse-logger

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about hitting the path multiple times and triggering multiple sending?
Like https://github.com/apache/apisix/blob/master/docs/en/latest/internal/testing-framework.md#send-request
Then we can check if all the endpoints are used via error log.

Copy link
Contributor Author

@xiaoxuanzi xiaoxuanzi Jan 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @spacewander.
I fixed the code according to your suggestion. Please review it.