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

slack-vitess-r14.0.5:Implement the Vitess Structure Logger VTSLogger #80

Merged
merged 4 commits into from
May 11, 2023
Merged

slack-vitess-r14.0.5:Implement the Vitess Structure Logger VTSLogger #80

merged 4 commits into from
May 11, 2023

Conversation

EmadMokhtar
Copy link

@EmadMokhtar EmadMokhtar commented May 10, 2023 •
edited by tanjinx
Loading

Description

This PR introduce a new flag --structured-logging to enable usage of Zap log instead of glog. Technically, I use noglog package to bind Zap log APIs with glog APIs. So the code can still use glog APIs but it's Zap logger.

Upstream WIP PR: vitessio#13061

Related Issue(s)

Checklist

  • "Backport to:" labels have been added if this change should be back-ported
  • Tests were added or are not required
  • Did the new or modified tests pass consistently locally and on the CI
  • Documentation was added or is not required

Deployment Notes

@EmadMokhtar
Implement the Vitess Structure Logger VTSLoger
b63979f 
Signed-off-by: Emad Habib <ehabib@slack-corp.com>
@EmadMokhtar EmadMokhtar requested a review from a team as a code owner May 10, 2023 13:02
@EmadMokhtar EmadMokhtar self-assigned this May 10, 2023
@tanjinx tanjinx changed the title Implement the Vitess Structure Logger VTSLoger slack-vitess-r14.0.5:Implement the Vitess Structure Logger VTSLoger May 10, 2023
@EmadMokhtar
Copy link
Author

EmadMokhtar commented May 10, 2023 •
edited
Loading

The tests are failing because I'm using Zap package that needs Go 1.9 but the module needs Go 1.18. I'll try to downgrade the Zap version.

@EmadMokhtar EmadMokhtar force-pushed the emad-structured-logging-zap branch from 7b8b85b to e9b19cf Compare May 10, 2023 15:03
@EmadMokhtar
Copy link
Author

Downgrade fixed the issue.

@EmadMokhtar EmadMokhtar changed the title slack-vitess-r14.0.5:Implement the Vitess Structure Logger VTSLoger slack-vitess-r14.0.5:Implement the Vitess Structure Logger VTSLogger May 10, 2023
@EmadMokhtar
Downgrade zap from 1.24.0 to 1.23.0
eab91bd 
Signed-off-by: Emad Habib <ehabib@slack-corp.com>
@EmadMokhtar EmadMokhtar force-pushed the emad-structured-logging-zap branch from e9b19cf to eab91bd Compare May 10, 2023 15:19
@EmadMokhtar
Ran go mod tidy
727b57e 
Signed-off-by: Emad Habib <ehabib@slack-corp.com>
@EmadMokhtar
Fix help text test
6437088 
Signed-off-by: Emad Habib <ehabib@slack-corp.com>
tanjinx
tanjinx approved these changes May 11, 2023
View reviewed changes
Copy link

@tanjinx tanjinx left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@tanjinx tanjinx merged commit 0e55277 into slack-vitess-r14.0.5 May 11, 2023
@timvaillancourt timvaillancourt mentioned this pull request Mar 18, 2024
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tanjinx tanjinx tanjinx approved these changes

@vmogilev vmogilev Awaiting requested review from vmogilev

@timvaillancourt timvaillancourt Awaiting requested review from timvaillancourt

Assignees

@EmadMokhtar EmadMokhtar

Labels
upstream-wip v14
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@EmadMokhtar @tanjinx