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(error-log-logger): add clickhouse for error-log-logger #6256

Merged
merged 14 commits into from
Feb 28, 2022

Conversation

qizhendong1
Copy link
Contributor

What this PR does / why we need it:

Add error-log-logger supports clickhouse.

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

@qizhendong1 qizhendong1 force-pushed the error-clickhouse-logger1 branch 2 times, most recently from cbb5f53 to a7bff92 Compare February 14, 2022 07:32
@qizhendong1 qizhendong1 force-pushed the error-clickhouse-logger1 branch from a7bff92 to ce3a6d6 Compare February 14, 2022 07:35
@qizhendong1 qizhendong1 marked this pull request as ready for review February 17, 2022 05:44
@qizhendong1 qizhendong1 force-pushed the error-clickhouse-logger1 branch from 214de6f to f0f8e30 Compare February 18, 2022 01:54
@tzssangglass tzssangglass changed the title feat(error-log-logger): add clickouse for error-log-logger feat(error-log-logger): add clickhouse for error-log-logger Feb 18, 2022
@tzssangglass
Copy link
Member

Now that we have the clickhouse plugin, can we implement it in this plugin?

I am concerned that this will introduce duplicate code.

apisix/plugins/error-log-logger.lua Outdated Show resolved Hide resolved
## How to set the clickhouse

The plugin sends the error log as a string to the `data` field of the clickhouse table.
Step: update the attributes of the plugin
Copy link
Member

Choose a reason for hiding this comment

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

What does this sentence mean?

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's means that the plugins insert error logs to the clickhouse table with data column default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

┌─data──────────────────────────────────────────────────────────────────┬
│  2022/02/16 16:45:07 [error] 4279#4279: *7272 [lua] batch-processor.lua:73: Batch ...                                  │ 
└───────────────────────────────────────────────────────────────────────┴

GET /tg
--- response_body
--- error_log
clickhouse error log body: INSERT INTO t FORMAT JSONEachRow
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to verify the reported error log in the test?

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 is difficult because the error logs are different each other.
clickhouse error log body: INSERT INTO t FORMAT JSONEachRow {"data": " 2022/02/16 16:45:07 [error] 4279#4279: *7272 [lua] batch-processor.lua:73: Batch ... "}

@qizhendong1
Copy link
Contributor Author

Now that we have the clickhouse plugin, can we implement it in this plugin?

I am concerned that this will introduce duplicate code.

I think it is a good idea. One side we can reduce duplicate code, antoher avoid error-log-logger.lua oversize. It is friendly for new plugin.

@spacewander How about you ?

@spacewander
Copy link
Member

No.
@tzssangglass
Handling the error log and access log are two different things. The former needs to be configured at the global level, the latter can be done in a smaller scope.

@tzssangglass
Copy link
Member

The former needs to be configured at the global level

I got. Let's keep it that way for now and leave the optimization for later.

t/plugin/error-log-logger-clickhouse.t Outdated Show resolved Hide resolved
t/plugin/error-log-logger-clickhouse.t Show resolved Hide resolved
GET /tg
--- response_body
--- error_log
clickhouse error log body: INSERT INTO t FORMAT JSONEachRow
Copy link
Member

Choose a reason for hiding this comment

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

is there log body releated to "this is a warning message for test5."?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I will add it.

t/plugin/error-log-logger-clickhouse.t Show resolved Hide resolved
docs/zh/latest/plugins/error-log-logger.md Outdated Show resolved Hide resolved
t/plugin/error-log-logger-clickhouse.t Outdated Show resolved Hide resolved
t/plugin/error-log-logger-clickhouse.t Outdated Show resolved Hide resolved
t/plugin/error-log-logger-clickhouse.t Outdated Show resolved Hide resolved
t/plugin/error-log-logger-clickhouse.t Outdated Show resolved Hide resolved
t/plugin/error-log-logger-clickhouse.t Outdated Show resolved Hide resolved
@qizhendong1 qizhendong1 force-pushed the error-clickhouse-logger1 branch from 1e72ba6 to a3f86d1 Compare February 22, 2022 02:43
@qizhendong1
Copy link
Contributor Author

qizhendong1 commented Feb 22, 2022

There is a test case failed. How to fix it ? @spacewander @tzssangglass @leslie-tsang
https://github.com/apache/apisix/runs/5282480987?check_suite_focus=true#step:6:4

Run ./utils/check-category.py
Entry docs/zh/latest/plugins/datadog.md in the sidebar can't be found. Please remove it from docs/zh/latest/config.json.
Error: Process completed with exit code 2[5](https://github.com/apache/apisix/runs/5282480987?check_suite_focus=true#step:6:5)5.

@spacewander
Copy link
Member

Thanks for your report. I am fixing it via #6411

@qizhendong1 qizhendong1 force-pushed the error-clickhouse-logger1 branch from a3f86d1 to b99e156 Compare February 23, 2022 06:27
@leslie-tsang
Copy link
Member

Hello there, Plz don't force push during the review.

Commit history allows reviewers to see what has changed since the last review, which is useful for them.

A forced push could ruin the review experience.

@tzssangglass
Copy link
Member

LGTM, @zhendongcmss CI/build was failed, I will rerun it and if it still fails, we need to focus on it.

spacewander
spacewander previously approved these changes Feb 28, 2022
docs/zh/latest/plugins/error-log-logger.md Show resolved Hide resolved
@spacewander spacewander merged commit b41a2e0 into apache:master Feb 28, 2022
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.

4 participants