Skip to content

Feature: collect and report logs#147

Merged
maxkezhang merged 15 commits into
apache:masterfrom
Superskyyy:grpc-logging
Aug 8, 2021
Merged

Feature: collect and report logs#147
maxkezhang merged 15 commits into
apache:masterfrom
Superskyyy:grpc-logging

Conversation

@Superskyyy

@Superskyyy Superskyyy commented Aug 7, 2021

Copy link
Copy Markdown
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.

@maxkezhang maxkezhang added core feature New feature labels Aug 7, 2021
@maxkezhang maxkezhang added this to the 0.7.0 milestone Aug 7, 2021
maxkezhang
maxkezhang reviewed Aug 7, 2021
View reviewed changes
Comment thread skywalking/agent/protocol/grpc_log.py Outdated
Comment thread skywalking/log/sw_logging.py Outdated
@maxkezhang maxkezhang requested a review from kezhenxu94 August 7, 2021 18:52
@maxkezhang

maxkezhang commented Aug 7, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

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

kezhenxu94 commented Aug 8, 2021

Copy link
Copy Markdown
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

wu-sheng commented Aug 8, 2021

Copy link
Copy Markdown
Member

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
Comment thread docs/EnvVars.md
| `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` |

@wu-sheng wu-sheng Aug 8, 2021

Copy link
Copy Markdown
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.

@Superskyyy Superskyyy Aug 8, 2021

Copy link
Copy Markdown
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

wu-sheng commented Aug 8, 2021

Copy link
Copy Markdown
Member

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

Superskyyy commented Aug 8, 2021

Copy link
Copy Markdown
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>
maxkezhang
maxkezhang approved these changes Aug 8, 2021
View reviewed changes

@maxkezhang maxkezhang left a comment

Copy link
Copy Markdown
Member

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

@kezhenxu94 kezhenxu94 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@Superskyyy nice work!! Thanks 🙇🏻

@kezhenxu94 kezhenxu94 requested a review from wu-sheng August 8, 2021 07:26
wu-sheng
wu-sheng approved these changes Aug 8, 2021
View reviewed changes

@wu-sheng wu-sheng left a comment

Copy link
Copy Markdown
Member

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

Superskyyy commented Aug 8, 2021

Copy link
Copy Markdown
Member Author

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

Certainly.

@maxkezhang maxkezhang merged commit 3323770 into apache:master Aug 8, 2021
@maxkezhang

maxkezhang commented Aug 8, 2021

Copy link
Copy Markdown
Member

Closes apache/skywalking#7118

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

tom-pytel commented Aug 8, 2021

Copy link
Copy Markdown
Contributor

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

@kezhenxu94

kezhenxu94 commented Aug 8, 2021

Copy link
Copy Markdown
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

tom-pytel commented Aug 8, 2021

Copy link
Copy Markdown
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

kezhenxu94 commented Aug 8, 2021

Copy link
Copy Markdown
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
@maxkezhang maxkezhang maxkezhang approved these changes

Assignees

No one assigned

Labels

core feature New feature

Projects

None yet

Milestone

0.7.0

Development

Successfully merging this pull request may close these issues.

5 participants

@Superskyyy @maxkezhang @kezhenxu94 @wu-sheng @tom-pytel