feat: add elasticsearch-logger by ccxhwmy · Pull Request #7643 · apache/apisix

ccxhwmy · 2022-08-10T14:09:23Z

Description

Checklist

I have explained the need for this PR and the problem it solves
I have explained the changes or the new features added to this PR
I have added tests corresponding to this change
I have updated the documentation to reflect this change
I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

spacewander · 2022-08-14T12:25:42Z

@ccxhwmy
Could you fix the message so we can close the issue when this PR is merged?

spacewander · 2022-08-14T12:35:26Z

+
+local DEFAULT_ELASTICSEARCH_SOURCE = "apache-apisix-elasticsearch-logging"
+
+local plugin_name = "elasticsearch-logging"


We should call it elasticsearch-logger like the kafka-logger plugin?

We should call it elasticsearch-logger like the kafka-logger plugin?

OK, do I need to change all ealsticsearch-logging to elasticsearch-logger? Including the file name?

spacewander · 2022-08-14T12:36:26Z

+
+
+local function get_logger_entry(conf)
+    local entry = log_util.get_full_log(ngx, conf)


Please also support the custom log format.

Please also support the custom log format.

How about the reference kafka-logger plugin?

apisix/apisix/plugins/kafka-logger.lua

Line 178 in 689e4f1

local entry

local entry if conf.meta_format == "origin" then entry = log_util.get_req_original(ctx, conf) -- core.log.info("origin entry: ", entry) else local metadata = plugin.plugin_metadata(plugin_name) core.log.info("metadata: ", core.json.delay_encode(metadata)) if metadata and metadata.value.log_format and core.table.nkeys(metadata.value.log_format) > 0 then entry = log_util.get_custom_format_log(ctx, metadata.value.log_format) core.log.info("custom log format entry: ", core.json.delay_encode(entry)) else entry = log_util.get_full_log(ngx, conf) core.log.info("full log entry: ", core.json.delay_encode(entry)) end end

We do not reference the code of another plugin in one plugin. Unless we pull some generic code into a common module.

We do not reference the code of another plugin in one plugin. Unless we pull some generic code into a common module.

Please also support the custom log format.

Is the custom log format like:
https://github.com/apache/apisix/blob/master/docs/en/latest/plugins/kafka-logger.md#metadata

spacewander · 2022-08-14T12:37:53Z

+local schema = {
+    type = "object",
+    properties = {
+        endpoint = {


Why do we wrap all the fields in an extra endpoint field?

spacewander · 2022-08-14T12:39:25Z

+        }) .. "\n" ..
+        core.json.encode({
+            time = ngx_now(),
+            host = entry.server.hostname,


Why should we invent a format structure for a specific plugin?

Why should we invent a format structure for a specific plugin?

I refer to splunk-hec-logging:

apisix/apisix/plugins/splunk-hec-logging.lua

Line 77 in 689e4f1

local function get_logger_entry(conf)

How about core.json.encode(entry) directly?

we just need to use log_util.get_req_original(ctx, conf) to get entry, This is a json, and ES supports this format.

Remember that when we update the entry, we also need to update the png image.

we just need to use log_util.get_req_original(ctx, conf) to get entry, This is a json, and ES supports this format.

I found that log_util.get_req_original(ctx, conf) will return a string instead of json:

apisix/apisix/utils/log-util.lua

Lines 196 to 211 in c4d5f2f

function _M.get_req_original(ctx, conf)

local headers = {

ctx.var.request, "\r\n"

}

for k, v in pairs(ngx.req.get_headers()) do

core.table.insert_tail(headers, k, ": ", v, "\r\n")

end

-- core.log.error("headers: ", core.table.concat(headers, ""))

core.table.insert(headers, "\r\n")

if conf.include_req_body then

core.table.insert(headers, ctx.var.request_body)

end

return core.table.concat(headers, "")

end

my mistake. json string is ok. we can follow this.

spacewander · 2022-08-14T12:40:43Z

+    end
+
+    local uri = conf.endpoint.uri ..
+        (str_sub(conf.endpoint.uri, -1) == "/" and "_bulk" or "/_bulk")


Use string.byte would be better

spacewander · 2022-08-14T12:43:34Z

+-- limitations under the License.
+--
+
+local ngx             = ngx


We should move the localized variable after require ...

spacewander · 2022-08-14T12:47:04Z

+--- request
+GET /hello
+--- wait: 2
+--- response_body


We should check the data sent to the elasticsearch, via injecting like: #7593 (comment)

We should check the data sent to the elasticsearch, via injecting like: #7593 (comment)

Does you mean I should hook the function httpc:request_uri and mock it with my own mock_request_uri, and check the request body with the mock_request_uri?
If so, I am not quite understand the nessary to check request body.

We should check the data sent to the elasticsearch, via injecting like: #7593 (comment)

Does you mean I should hook the function httpc:request_uri and mock it with my own mock_request_uri, and check the request body with the mock_request_uri? If so, I am not quite understand the nessary to check request body.

@spacewander
Please pay attention to this question when you are free, Thank you.

Sorry for missing this. Yes, you are right.

spacewander · 2022-08-14T12:51:59Z

+                uri = core.schema.uri_def,
+                index = { type = "string"},
+                type = { type = "string"},
+                username = { type = "string"},


We can store username & password in an additional field like

apisix/apisix/plugins/kafka-proxy.lua

Line 25 in a8d03ac

properties = {

So that we can require them easily.

ccxhwmy · 2022-08-14T14:36:39Z

@ccxhwmy Could you fix the message so we can close the issue when this PR is merged?

I tryed to do it, but failed, it seems that I don't have permission.

tzssangglass · 2022-08-14T15:26:42Z

I tryed to do it, but failed, it seems that I don't have permission.

like:

Fixes #7636

ccxhwmy · 2022-08-15T00:01:17Z

I tryed to do it, but failed, it seems that I don't have permission.

like:

Fixes #7636

I get, Thank you

tzssangglass · 2022-08-15T06:51:04Z

+        headers["Authorization"] = authorization
+    end
+
+    core.log.info("uri: ", uri, ", body: ", body, ", headers: ", core.json.encode(headers))


This headers may contain username and passowrd in base64 format, which we should not output in the log for security reasons.

guitu168 · 2022-08-15T06:39:58Z

+---
+title: elasticsearch-logging
+---


can you add some keywords and description？

please refer: https://github.com/apache/apisix/blob/master/docs/zh/latest/plugins/http-logger.md

guitu168 · 2022-08-15T07:50:05Z

+| 名称                | 是否必需 | 默认值               | 描述                                                         |
+| ------------------- | -------- | -------------------- | ------------------------------------------------------------ |
+| endpoint            | 必选     |                      | Elasticsearch 端点配置信息                                   |
+| endpoint.uri        | 必选     |                      | Elasticsearch API                                            |
+| endpoint.index      | 必选     |                      | Elasticsearch [_index field](https://www.elastic.co/guide/en/elasticsearch/reference/current/mapping-index-field.html#mapping-index-field) |
+| endpoint.type       | 可选     | Elasticsearch 默认值 | Elasticsearch [_type field](https://www.elastic.co/guide/en/elasticsearch/reference/7.17/mapping-type-field.html#mapping-type-field) |
+| endpoint.username   | 可选     |                      | Elasticsearch [authentication](https://www.elastic.co/guide/en/elasticsearch/reference/current/setting-up-authentication.html) username |
+| endpoint.password   | 可选     |                      | Elasticsearch [authentication](https://www.elastic.co/guide/en/elasticsearch/reference/current/setting-up-authentication.html) password |
+| endpoint.ssl_verify | 可选     | true                 | 当设置为 `true` 则允许 SSL 验证，参考 [OpenResty docs](https://github.com/openresty/lua-nginx-module#tcpsocksslhandshake) |
+| endpoint.timeout    | 可选     | 10                   | 发送给 Elasticsearch 请求超时时间                            |


Please refer to http-logger modification.

guitu168 · 2022-08-15T07:54:07Z

great work! 👍

Co-authored-by: tzssangglass <[email protected]>

Co-authored-by: Fei Han <[email protected]>

tzssangglass · 2022-08-24T02:11:04Z

LGTM(left two comments about indent)

Co-authored-by: tzssangglass <[email protected]>

check endpoint_addr must not end with "/"

guitu168

other LGTM 👍

Co-authored-by: Fei Han <[email protected]>

ccxhwmy · 2022-08-25T00:07:00Z

Need we re-run the ci?

spacewander · 2022-08-29T07:25:11Z

+local schema = {
+    type = "object",
+    properties = {
+        meta_format = {


Where do we use this field?

Where do we use this field?

My mistake, I forgot to remove it.

spacewander · 2022-08-29T07:30:38Z

+
+    local uri = conf.endpoint_addr .. "/_bulk"
+    local body = core.table.concat(entries, "")
+    local headers = {["Content-Type"] = "application/json"}


Would be better to use application/x-ndjson?

Would be better to use application/x-ndjson?

I learned about ndjson, I think it is more suitable than json here. Thank you.

spacewander · 2022-08-29T07:31:31Z

+### 完整配置示例
+
+```shell
+curl http://127.0.0.1:9080/apisix/admin/routes/1 \


Would you update the port number to 9180 like the PR: #7806?

1. remove useless code 2. replace application/json with application/x-ndjson 3. modify cp port from 9080 to 9180

ccxhwmy · 2022-08-30T01:48:14Z

Need we re-run the ci?

tzssangglass · 2022-08-30T01:50:46Z

Need we re-run the ci?

done

ccxhwmy · 2022-08-30T05:36:47Z

Need we re-run the ci?

done

@tzssangglass Please re-run ci again, Thanks.

* upstream/master: (214 commits) feat: set constants.apisix_lua_home for used by plugins (apache#7893) fix: response-rewrite plugin might cause Apache AIPSIX hanging (apache#7836) test: sleep 1 second in t/cli/test_upstream_mtls.sh (apache#7889) fix: reload once when log rotate (apache#7869) change: move etcd conf under deployment (apache#7860) fix: plugin metadata missing v3 adapter call (apache#7877) docs: add ClickHouse and Elasticsearch loggers. (apache#7848) docs(plugin): refactor limit-conn.md (apache#7857) feat: call `destroy` method when Nginx exits (apache#7866) feat: Add ability to inject headers via prefix to otel traces (apache#7822) docs(plugin): refactor proxy-cache.md (apache#7858) fix: don't enable passive healthcheck by default (apache#7850) feat: add openfunction plugin (apache#7634) fix(zipkin): send trace IDs with a reject sampling decision (apache#7833) fix: Change opentelemetry's span kind to server (apache#7830) docs(hmac-auth): additional details for generating signing_string (apache#7816) docs: correct the test-nginx description (apache#7818) docs: add docs of workflow plugin (apache#7813) docs(FAQ): add how to detect APISIX alive status (apache#7812) feat: add elasticsearch-logger (apache#7643) ...

Co-authored-by: tzssangglass <[email protected]> Co-authored-by: Fei Han <[email protected]>

add elasticsearch-logging base function

6e1f2d4

tzssangglass changed the title ~~add elasticsearch-logging base function~~ feat: add elasticsearch-logging[WIP] Aug 10, 2022

fanchangjifen mentioned this pull request Aug 11, 2022

init elasticsearch-logger.lua #7099

Closed

wangchengcheng95 added 8 commits August 11, 2022 20:46

add the support of option of username and password

446b174

add the support of elasticsearch authentication

6dfc58c

add docs of elasticsearch plugin

54cc283

update docker compose file to add elasticsearch config

9916235

add test case

0e96d5c

repair ci error

00fe216

update test case

d7e95c4

set elasticsearch index and type with configure

f03e804

ccxhwmy marked this pull request as ready for review August 14, 2022 07:31

ccxhwmy changed the title ~~feat: add elasticsearch-logging[WIP]~~ feat: add elasticsearch-logging Aug 14, 2022

spacewander reviewed Aug 14, 2022

View reviewed changes

tzssangglass reviewed Aug 15, 2022

View reviewed changes

Comment thread apisix/plugins/elasticsearch-logging.lua Outdated

tzssangglass reviewed Aug 15, 2022

View reviewed changes

Comment thread apisix/plugins/elasticsearch-logging.lua Outdated

guitu168 reviewed Aug 15, 2022

View reviewed changes

ccxhwmy changed the title ~~feat: add elasticsearch-logging~~ feat: add elasticsearch-logger Aug 15, 2022

ccxhwmy and others added 6 commits August 15, 2022 21:10

Update apisix/plugins/elasticsearch-logging.lua

ecbd983

Co-authored-by: tzssangglass <[email protected]>

Update apisix/plugins/elasticsearch-logging.lua

5765063

Co-authored-by: tzssangglass <[email protected]>

Update docs/zh/latest/plugins/elasticsearch-logging.md

2b8801c

Co-authored-by: Fei Han <[email protected]>

Update docs/zh/latest/plugins/elasticsearch-logging.md

1f7c530

Co-authored-by: Fei Han <[email protected]>

Update docs/zh/latest/plugins/elasticsearch-logging.md

219c4fa

Co-authored-by: Fei Han <[email protected]>

Update docs/zh/latest/plugins/elasticsearch-logging.md

acf8cac

Co-authored-by: Fei Han <[email protected]>

ccxhwmy and others added 2 commits August 24, 2022 08:19

Merge branch 'master' into feat_elasticsearch_logging

cb6afd3

update endpoint_addr check pattern

4b37d1b

tzssangglass reviewed Aug 24, 2022

View reviewed changes

Comment thread apisix/plugins/elasticsearch-logger.lua Outdated

tzssangglass reviewed Aug 24, 2022

View reviewed changes

Comment thread t/plugin/elasticsearch-logger.t Outdated

ccxhwmy and others added 3 commits August 24, 2022 11:17

Update apisix/plugins/elasticsearch-logger.lua

cd96bc4

Co-authored-by: tzssangglass <[email protected]>

Update t/plugin/elasticsearch-logger.t

4c4859a

Co-authored-by: tzssangglass <[email protected]>

update test case

1a56fd9

check endpoint_addr must not end with "/"

ccxhwmy requested a review from guitu168 August 24, 2022 14:05

guitu168 reviewed Aug 24, 2022

View reviewed changes

ccxhwmy and others added 5 commits August 24, 2022 23:22

Update docs/zh/latest/plugins/elasticsearch-logger.md

902aa05

Co-authored-by: Fei Han <[email protected]>

Update docs/zh/latest/plugins/elasticsearch-logger.md

1693a2a

Co-authored-by: Fei Han <[email protected]>

Update docs/zh/latest/plugins/elasticsearch-logger.md

9914004

Co-authored-by: Fei Han <[email protected]>

Update docs/zh/latest/plugins/elasticsearch-logger.md

05e93e7

Co-authored-by: Fei Han <[email protected]>

Update docs/en/latest/plugins/elasticsearch-logger.md

5d9028a

Co-authored-by: Fei Han <[email protected]>

spacewander requested a review from tzssangglass August 25, 2022 05:59

remove endpoint_addr end with "/" judgement logic

5062c8d

tzssangglass previously approved these changes Aug 28, 2022

View reviewed changes

spacewander reviewed Aug 29, 2022

View reviewed changes

modify reviewer suggestion

a5972f2

1. remove useless code 2. replace application/json with application/x-ndjson 3. modify cp port from 9080 to 9180

ccxhwmy dismissed tzssangglass’s stale review via a5972f2 August 29, 2022 13:46

spacewander approved these changes Aug 30, 2022

View reviewed changes

tzssangglass approved these changes Aug 30, 2022

View reviewed changes

spacewander merged commit fd1411c into apache:master Aug 31, 2022

Liu-Junlin pushed a commit to Liu-Junlin/apisix that referenced this pull request Nov 4, 2022

feat: add elasticsearch-logger (apache#7643)

cb7efa2

Co-authored-by: tzssangglass <[email protected]> Co-authored-by: Fei Han <[email protected]>


		local DEFAULT_ELASTICSEARCH_SOURCE = "apache-apisix-elasticsearch-logging"

		local plugin_name = "elasticsearch-logging"



		local function get_logger_entry(conf)
		local entry = log_util.get_full_log(ngx, conf)

	function _M.get_req_original(ctx, conf)
	local headers = {
	ctx.var.request, "\r\n"
	}
	for k, v in pairs(ngx.req.get_headers()) do
	core.table.insert_tail(headers, k, ": ", v, "\r\n")
	end
	-- core.log.error("headers: ", core.table.concat(headers, ""))
	core.table.insert(headers, "\r\n")

	if conf.include_req_body then
	core.table.insert(headers, ctx.var.request_body)
	end

	return core.table.concat(headers, "")
	end

Uh oh!

Conversation

ccxhwmy commented Aug 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

spacewander commented Aug 14, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ccxhwmy commented Aug 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tzssangglass commented Aug 14, 2022

Uh oh!

ccxhwmy commented Aug 15, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

guitu168 commented Aug 15, 2022

Uh oh!

Uh oh!

Uh oh!

ccxhwmy commented Aug 10, 2022 •

edited

Loading

ccxhwmy commented Aug 14, 2022 •

edited

Loading