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: [#347] Switch logging channels on runtime #482

Merged
merged 11 commits into from
May 26, 2024
Merged

Conversation

kkumar-gcc
Copy link
Member

@kkumar-gcc kkumar-gcc commented May 23, 2024

Closes goravel/goravel#347

πŸ“‘ Description

βœ… Checks

  • My pull request adheres to the code style of this project
  • My code requires changes to the documentation
  • I have updated the documentation as required
  • All the tests have passed

β„Ή Additional Information

@kkumar-gcc kkumar-gcc closed this May 23, 2024
@kkumar-gcc kkumar-gcc reopened this May 23, 2024
@kkumar-gcc kkumar-gcc requested a review from a team May 23, 2024 07:02
log/application.go Outdated Show resolved Hide resolved
Copy link

codecov bot commented May 23, 2024

Codecov Report

Attention: Patch coverage is 31.57895% with 26 lines in your changes are missing coverage. Please review.

Project coverage is 69.53%. Comparing base (5cf1aa4) to head (ddc452c).

Files Patch % Lines
log/application.go 34.28% 22 Missing and 1 partial ⚠️
log/logrus_writer.go 0.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #482      +/-   ##
==========================================
- Coverage   69.70%   69.53%   -0.17%     
==========================================
  Files         174      174              
  Lines       10721    10744      +23     
==========================================
- Hits         7473     7471       -2     
- Misses       2681     2704      +23     
- Partials      567      569       +2     

β˜” View full report in Codecov by Sentry.
πŸ“’ Have feedback on the report? Share it here.

@codecov-commenter
Copy link

codecov-commenter commented May 24, 2024

Codecov Report

Attention: Patch coverage is 88.88889% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 69.99%. Comparing base (18fe188) to head (3aafec2).

Files Patch % Lines
log/logrus_writer.go 0.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #482      +/-   ##
==========================================
+ Coverage   69.89%   69.99%   +0.10%     
==========================================
  Files         175      175              
  Lines       10795    10815      +20     
==========================================
+ Hits         7545     7570      +25     
+ Misses       2683     2678       -5     
  Partials      567      567              

β˜” View full report in Codecov by Sentry.
πŸ“’ Have feedback on the report? Share it here.

@kkumar-gcc kkumar-gcc requested a review from hwbrzzl May 24, 2024 07:38
Copy link
Contributor

@hwbrzzl hwbrzzl left a comment

Choose a reason for hiding this comment

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

Good, please optimize test cases.

Comment on lines 72 to 78
channelPath := "logging.channels." + channel
driver := r.config.GetString(channelPath + ".driver")

if driver == log.StackDriver {
color.Red().Println("stack driver can't include self channel")
return nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We may don't need to add this limitation, registerHook will handle the logic.

Copy link
Member Author

@kkumar-gcc kkumar-gcc May 25, 2024

Choose a reason for hiding this comment

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

No, registerHook won't handle it. registerHook only checks if stack.channels refers to itself again. Since the stack function doesn't add channels into the config, it will not detect it.
image

Copy link
Contributor

Choose a reason for hiding this comment

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

The stack function doesn't add channels into the config? Do you mean this?

image

The method facades.Log().Stack([]string{"stack", "single"}) is allowed, you seem to be missing something?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, sorry, I was mistaking the channel for the driver.

devhaozi
devhaozi previously approved these changes May 25, 2024
Copy link
Member

@devhaozi devhaozi left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

Choose a reason for hiding this comment

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

Great

@kkumar-gcc kkumar-gcc merged commit a72c33a into master May 26, 2024
8 checks passed
@kkumar-gcc kkumar-gcc deleted the kkumar-gcc/#347 branch May 26, 2024 06:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants