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.

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

feat: clickhouse logger #6215

Merged
merged 19 commits into from
Feb 16, 2022
Merged

Conversation

zhendongcmss
Copy link
Contributor

@zhendongcmss zhendongcmss commented Jan 27, 2022

What this PR does / why we need it:

feat: push access log to clickhouse DB

Pre-submission checklist:

  • Did you explain what problem does this PR solve? Or what new features have been added?
  • Have you added corresponding test cases?
  • Have you modified the corresponding document?
  • Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

@zhendongcmss zhendongcmss marked this pull request as ready for review January 27, 2022 07:48
apisix/plugins/clickhouse-logger.lua Outdated Show resolved Hide resolved
apisix/plugins/clickhouse-logger.lua Outdated Show resolved Hide resolved
conf/config-default.yaml Outdated Show resolved Hide resolved
apisix/plugins/clickhouse-logger.lua Outdated Show resolved Hide resolved
docs/en/latest/plugins/clickhouse-logger.md Outdated Show resolved Hide resolved
apisix/plugins/clickhouse-logger.lua Outdated Show resolved Hide resolved
t/plugin/clickhouse-logger.t Outdated Show resolved Hide resolved
apisix/plugins/clickhouse-logger.lua Outdated Show resolved Hide resolved
apisix/plugins/clickhouse-logger.lua Outdated Show resolved Hide resolved
apisix/plugins/clickhouse-logger.lua Show resolved Hide resolved
apisix/plugins/clickhouse-logger.lua Outdated Show resolved Hide resolved
@zhendongcmss
Copy link
Contributor Author

Hi, @spacewander
How fix this test case ?
https://github.com/apache/apisix/runs/4989817705?check_suite_focus=true#step:7:22

Installing collected packages: zhon
Successfully installed zhon-1.1.5
find broken newline in file: docs/zh/latest/plugins/clickhouse-logger.md
cat: /tmp/error.log: No such file or directory
Error: Process completed with exit code 1.

@spacewander
Copy link
Member

Hi, @spacewander How fix this test case ? apache/apisix/runs/4989817705?check_suite_focus=true#step:7:22

Installing collected packages: zhon
Successfully installed zhon-1.1.5
find broken newline in file: docs/zh/latest/plugins/clickhouse-logger.md
cat: /tmp/error.log: No such file or directory
Error: Process completed with exit code 1.

Sorry, it is the bug in our lint script. Fixed in #6239

apisix/plugins/clickhouse-logger.lua Outdated Show resolved Hide resolved
docs/en/latest/plugins/clickhouse-logger.md Outdated Show resolved Hide resolved
conf/config-default.yaml Outdated Show resolved Hide resolved
t/plugin/clickhouse-logger.t Outdated Show resolved Hide resolved
t/plugin/clickhouse-logger.t Show resolved Hide resolved
@tzssangglass
Copy link
Member

hi @zhendongcmss CI is broken

@zhendongcmss
Copy link
Contributor Author

zhendongcmss commented Feb 7, 2022

hi @zhendongcmss CI is broken

@spacewander @tzssangglass
sorry, I don't know how to fix. I need help, can you tell me how to fix it ?

#   Failed test 'TEST 4: add plugin on routes - status code ok'
#   at /apisix/test-nginx/lib/Test/Nginx/Socket.pm line 948.
#          got: '401'
#     expected: '200'

#   Failed test 'TEST 5: access local server - status code ok'
#   at /apisix/test-nginx/lib/Test/Nginx/Socket.pm line 948.
#          got: '404'
#     expected: '200'

#   Failed test 'TEST 5: access local server - response_body - response is expected (repeated req 0, req 0)'
#   at /apisix/test-nginx/lib/Test/Nginx/Socket.pm line 1589.
#          got: '{"error_msg":"404 Route Not Found"}
# '
#     expected: 'opentracing

t/plugin/clickhouse-logger.t Outdated Show resolved Hide resolved
t/plugin/clickhouse-logger.t Show resolved Hide resolved
@tzssangglass
Copy link
Member

hi @zhendongcmss CI is broken

@spacewander @tzssangglass sorry, I don't know how to fix. I need help, can you tell me how to fix it ?

#   Failed test 'TEST 4: add plugin on routes - status code ok'
#   at /apisix/test-nginx/lib/Test/Nginx/Socket.pm line 948.
#          got: '401'
#     expected: '200'

#   Failed test 'TEST 5: access local server - status code ok'
#   at /apisix/test-nginx/lib/Test/Nginx/Socket.pm line 948.
#          got: '404'
#     expected: '200'

#   Failed test 'TEST 5: access local server - response_body - response is expected (repeated req 0, req 0)'
#   at /apisix/test-nginx/lib/Test/Nginx/Socket.pm line 1589.
#          got: '{"error_msg":"404 Route Not Found"}
# '
#     expected: 'opentracing

My comments above solved the 401 problem with test 4, you can continue to see the problem with test 5, which is related to the code and configuration.

t/plugin/clickhouse-logger.t Outdated Show resolved Hide resolved
t/plugin/clickhouse-logger.t Outdated Show resolved Hide resolved
t/plugin/clickhouse-logger.t Outdated Show resolved Hide resolved
t/plugin/clickhouse-logger.t Show resolved Hide resolved
t/plugin/clickhouse-logger.t Outdated Show resolved Hide resolved
"clickhouse body: INSERT INTO t FORMAT JSONEachRow"
"clickhouse headers: X-ClickHouse-Key:a"
"clickhouse headers: X-ClickHouse-User:default"
"clickhouse headers: X-ClickHouse-Database:default"
Copy link
Member

@spacewander spacewander Feb 14, 2022

Choose a reason for hiding this comment

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

I have reconsidered the test.
What about using error_log like

apisix/t/plugin/loggly.t

Lines 572 to 574 in 537a8da

--- error_log
loggly body: {"route_id":"1"}
loggly tags: "apisix"
?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't work. I don't know why. I think --- grep_error_log_out is easier to use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

},
"plugins":{
"clickhouse-logger":{
"batch_max_size":1000,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"batch_max_size":1000,
"batch_max_size":1,

So the report can be triggered in the CI

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 thought there was a timer that called this callback function periodically and also if the batch_max_size value was reached. But I added a wait of --- wait: 5 below, it seem doesn'twork.

tzssangglass
tzssangglass previously approved these changes Feb 16, 2022
spacewander
spacewander previously approved these changes Feb 16, 2022
Copy link
Member

@spacewander spacewander left a comment

Choose a reason for hiding this comment

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

Could you apply this patch:

--- t/plugin/clickhouse-logger.t
+++ t/plugin/clickhouse-logger.t
@@ -149,7 +149,9 @@ property "endpoint_addr" is required
                                 "password": "a",
                                 "database": "default",
                                 "logtable": "t",
-                                "endpoint_addr": "http://127.0.0.1:10420/clickhouse-logger/test"
+                                "endpoint_addr": "http://127.0.0.1:10420/clickhouse-logger/test",
+                                "batch_max_size":1,
+                                "inactive_timeout":1
                             }
                         },
                         "upstream": {
@@ -173,7 +175,7 @@ property "endpoint_addr" is required
                             },
                             "plugins":{
                                 "clickhouse-logger":{
-                                    "batch_max_size":1000,
+                                    "batch_max_size":1,
                                     "max_retry_count":0,
                                     "retry_delay":1,
                                     "ssl_verify":true,
@@ -185,7 +187,7 @@ property "endpoint_addr" is required
                                     "name":"clickhouse-logger",
                                     "database":"default",
                                     "logtable":"t",
-                                    "inactive_timeout":5
+                                    "inactive_timeout":1
                                 }
                             },
                             "id":"1"
@@ -213,7 +215,7 @@ GET /opentracing
 opentracing
 --- error_log
 clickhouse body: INSERT INTO t FORMAT JSONEachRow
-clickhouse headers: X-ClickHouse-Key:a
-clickhouse headers: X-ClickHouse-User:default
-clickhouse headers: X-ClickHouse-Database:default
---- wait: 5
+clickhouse headers: x-clickhouse-key:a
+clickhouse headers: x-clickhouse-user:default
+clickhouse headers: x-clickhouse-database:default
+--- wait: 2

Now the test should pass.
There is another issue, the headers we get are all lowercase.

@spacewander spacewander merged commit a933c0b into apache:master Feb 16, 2022
@spacewander
Copy link
Member


## Attributes

| 名称 | 类型 | 必选项 | 默认值 | 有效值 | 描述 |
Copy link
Member

Choose a reason for hiding this comment

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

Hi @zhendongcmss, here have some Chinese 😄 Would you like to translate them?

Copy link
Member

@juzhiyuan juzhiyuan Feb 22, 2022

Choose a reason for hiding this comment

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

Hi, @yzeng25, do you have interest to do this? 🤗

Copy link
Member

Choose a reason for hiding this comment

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

I created an issue, let's fix it: #6414

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants