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.

@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.

@tokers
Copy link
Contributor

tokers commented Jul 24, 2021

@dmsolr Please also replenish the documents.




=== 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

@@ -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.

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.

5 participants