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.

Sign up for GitHub

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

Jump to bottom

Feature: collect and report logs #147

Merged
merged 15 commits into from Aug 8, 2021
Merged

Feature: collect and report logs #147

merged 15 commits into from Aug 8, 2021

Conversation

Superskyyy
Copy link
Member

This is a OSPP Summer 2021 project supervised by @Humbertzhang | apache/skywalking#7118

The feature implements optional log reporter functionalities in alignment with the SkyWalking Java agent.

  • Intercepts logs from Python logging module.
  • Reports logs via a new temporary gRPC channel(to be removed in the future).
  • Supports unformatted/ formatted logs with custom layout.
  • Supports custom logging level threshold.
  • Bumps up submodule to support log collection protocols.
  • Bumps up skywalking-eyes and adds a config entry to ignore .gitignore during license checks.

@Humbertzhang Humbertzhang added core feature New feature labels Aug 7, 2021
@Humbertzhang Humbertzhang added this to the 0.7.0 milestone Aug 7, 2021
Humbertzhang
Humbertzhang reviewed Aug 7, 2021
View reviewed changes
skywalking/agent/protocol/grpc_log.py Outdated Show resolved Hide resolved
skywalking/log/sw_logging.py Outdated Show resolved Hide resolved
@Humbertzhang
Copy link
Member

Humbertzhang commented Aug 7, 2021 •
edited

Test it manually and it generally looks good to me. You should add test for it after this PR merged.
@kezhenxu94, I am wondering if this feature should be tested by using skywalking-infra-e2e ?

@kezhenxu94
Copy link
Member

@kezhenxu94, I am wondering if this feature should be tested by using skywalking-infra-e2e ?

I think it's not mandatory, though it would be a bonus if @Superskyyy can adopt skywalking-infra-e2e in the new test.

@wu-sheng
Copy link
Member

wu-sheng commented Aug 8, 2021

Test it manually and it generally looks good to me. You should add test for it after this PR merged.
@kezhenxu94, I am wondering if this feature should be tested by using skywalking-infra-e2e ?

This is recommended, that mechanism makes sure we wouldn't break it in the future.

wu-sheng
wu-sheng reviewed Aug 8, 2021
View reviewed changes
docs/EnvVars.md
@@ -32,3 +32,10 @@ Environment Variable | Description | Default
| `SW_CELERY_PARAMETERS_LENGTH`| The maximum length of `celery` functions parameters, longer than this will be truncated, 0 turns off | `512` |
| `SW_AGENT_PROFILE_ACTIVE` | If `True`, Python agent will enable profile when user create a new profile task. Otherwise disable profile. | `False` |
| `SW_PROFILE_TASK_QUERY_INTERVAL` | The number of seconds between two profile task query. | `20` |
| `SW_AGENT_LOG_REPORTER_ACTIVE` | If `True`, Python agent will report collected logs to the OAP or Satellite. Otherwise, it disables the feature. | `False` |
Copy link
Member

Choose a reason for hiding this comment

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

Besides this doc change, let's add a specific doc to show users how to use it.

Copy link
Member Author

Choose a reason for hiding this comment

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

@wu-sheng I just added a detailed guide, please see if that is sufficient.

@wu-sheng
Copy link
Member

wu-sheng commented Aug 8, 2021

And after this change, a new section should be added into main repo's doc, https://github.com/apache/skywalking/blob/master/docs/en/setup/backend/log-analyzer.md#java-agents-toolkits, which should link to this required doc #147 (comment)

@Superskyyy
Copy link
Member Author

Test it manually and it generally looks good to me. You should add test for it after this PR merged.
@kezhenxu94, I am wondering if this feature should be tested by using skywalking-infra-e2e ?

This is recommended, that mechanism makes sure we wouldn't break it in the future.

Will do that next :)

Superskyyy and others added 3 commits August 8, 2021 11:04
@Superskyyy
- Move Formatter initialization to outer scope.
a71c6b8 
Signed-off-by: YihaoChen <Superskyyy@outlook.com>
@Superskyyy
New document - A complete guide to log reporter.
407ceb9 
Signed-off-by: YihaoChen <Superskyyy@outlook.com>
@Superskyyy
Add a new section in README pointing to the log reporter guide.
eb83f1f 
Signed-off-by: YihaoChen <Superskyyy@outlook.com>
Humbertzhang
Humbertzhang approved these changes Aug 8, 2021
View reviewed changes
Copy link
Member

@Humbertzhang Humbertzhang left a comment

Choose a reason for hiding this comment

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

LGTM, Thank you for your contributing!

kezhenxu94
kezhenxu94 approved these changes Aug 8, 2021
View reviewed changes
Copy link
Member

@kezhenxu94 kezhenxu94 left a comment

Choose a reason for hiding this comment

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

@Superskyyy nice work!! Thanks 🙇🏻

wu-sheng
wu-sheng approved these changes Aug 8, 2021
View reviewed changes
Copy link
Member

@wu-sheng wu-sheng left a comment

Choose a reason for hiding this comment

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

Thanks. Please update the main repo doc linking to this doc.

@Superskyyy
Copy link
Member Author

Thanks. Please update the main repo doc linking to this doc.

Certainly.

@Humbertzhang Humbertzhang merged commit 3323770 into apache:master Aug 8, 2021
@Humbertzhang
Copy link
Member

Closes apache/skywalking#7118

@kezhenxu94 kezhenxu94 mentioned this pull request Aug 8, 2021
Closed
@tom-pytel
Copy link
Contributor

So this is grpc only? No http or kafka protocol?

@kezhenxu94
Copy link
Member

So this is grpc only? No http or kafka protocol?

For this PR, yes, we need iterations to support http and kafka protocol

@tom-pytel
Copy link
Contributor

For this PR, yes, we need iterations to support http and kafka protocol

This functionality should be pushed into the protocols themselves, which would also remove the separate channel for grpc as the author said.

@kezhenxu94
Copy link
Member

This functionality should be pushed into the protocols themselves, which would also remove the separate channel for grpc as the author said.

@Humbertzhang will be working to remove the separate channel for logs by reusing the same gRPC protocol / channel.

@Superskyyy Superskyyy deleted the grpc-logging branch August 12, 2021 03:50
@Superskyyy Superskyyy mentioned this pull request Sep 14, 2021
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wu-sheng wu-sheng wu-sheng approved these changes

@kezhenxu94 kezhenxu94 kezhenxu94 approved these changes

@Humbertzhang Humbertzhang Humbertzhang approved these changes

Assignees
No one assigned
Labels
core feature New feature
Projects
None yet
Milestone
0.7.0
5 participants
@Superskyyy @Humbertzhang @kezhenxu94 @wu-sheng @tom-pytel