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: add error log skywalking reporter #4633

Merged
merged 25 commits into from
Aug 4, 2021
Merged

Conversation

dmsolr
Copy link
Member

@dmsolr dmsolr commented Jul 20, 2021

What this PR does / why we need it:

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

his plugin is error-log-logger-like, but it sends log data to Apache SkyWalking over http.

apisix/plugins/error-log-logger.lua Show resolved Hide resolved
apisix/plugins/error-log-logger.lua Outdated Show resolved Hide resolved
apisix/plugins/error-log-logger.lua Outdated Show resolved Hide resolved
apisix/plugins/error-log-logger.lua Outdated Show resolved Hide resolved
@dmsolr
Copy link
Member Author

dmsolr commented Jul 22, 2021

If these changes are ok in basically, I will update the related docs and tests.

apisix/plugins/error-log-logger.lua Show resolved Hide resolved
apisix/plugins/error-log-logger.lua Outdated Show resolved Hide resolved
@tokers
Copy link
Contributor

tokers commented Jul 24, 2021

@dmsolr Please also replenish the documents.

apisix/plugins/error-log-logger.lua Outdated Show resolved Hide resolved
t/plugin/error-log-skywalking-logger.t Outdated Show resolved Hide resolved
t/plugin/error-log-skywalking-logger.t Outdated Show resolved Hide resolved
t/plugin/error-log-skywalking-logger.t Outdated Show resolved Hide resolved
t/plugin/error-log-skywalking-logger.t Outdated Show resolved Hide resolved
apisix/plugins/error-log-logger.lua Outdated Show resolved Hide resolved
docs/en/latest/plugins/error-log-logger.md Outdated Show resolved Hide resolved
t/plugin/error-log-logger-skywalking.t Outdated Show resolved Hide resolved
t/plugin/error-log-logger.t Show resolved Hide resolved



=== TEST 5: log an warn level message
Copy link
Contributor

Choose a reason for hiding this comment

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

What information did I miss? What's the point of these tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

This case is testing whether a warning message could be wrapper as the skywalking log format

docs/en/latest/plugins/error-log-logger.md Outdated Show resolved Hide resolved
apisix/plugins/error-log-logger.lua Outdated Show resolved Hide resolved
t/plugin/error-log-logger-skywalking.t Show resolved Hide resolved
apisix/plugins/error-log-logger.lua Outdated Show resolved Hide resolved
apisix/plugins/error-log-logger.lua Outdated Show resolved Hide resolved
@@ -281,7 +287,115 @@ passed



=== TEST 9: want to reload the plugin by route
=== TEST 9: log a warn level message (schema compatibility testing)
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 add test at the end of file

Copy link
Member Author

Choose a reason for hiding this comment

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

we have resetting environment tasks at the end.

| tcp.port | integer | required | | [0,...] | Target upstream port. |
| tcp.tls | boolean | optional | false | | Control whether to perform SSL verification. |
| tcp.tls_server_name | string | optional | | | The server name for the new TLS extension SNI. |
| skywalking.endpoint_addr | string | required | http://127.0.0.1:12900/v3/logs | | the http endpoint of Skywalking. |
Copy link
Member

Choose a reason for hiding this comment

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

Once you provide a default value it is no longer required.

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it.

docs/en/latest/plugins/error-log-logger.md Outdated Show resolved Hide resolved
docs/en/latest/plugins/error-log-logger.md Outdated Show resolved Hide resolved
docs/en/latest/plugins/error-log-logger.md Outdated Show resolved Hide resolved
docs/en/latest/plugins/error-log-logger.md Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants