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

Add structure logging to Vitess #11960

Merged
merged 13 commits into from
Jan 25, 2023
Merged

Add structure logging to Vitess #11960

merged 13 commits into from
Jan 25, 2023

Conversation

EmadMokhtar
Copy link
Contributor

@EmadMokhtar EmadMokhtar commented Dec 14, 2022 •
edited
Loading

Description

The goal of this PR is to introduce the structure logging to Vitess. Vitess is using glog as logger. So I decided to use noglog and create drop-in replacement. I used PlanetScale log to replace glog as internally it's zap log.

The PR is also introducing a flag structure_logging where the user can choose to use structure logging or the default logging. The default value is False so the default is to use the current logging glog.

Related Issue(s)

#11613

Checklist

  • "Backport to:" labels have been added if this change should be back-ported
  • Tests were added or are not required
  • Documentation was added or is not required

Deployment Notes

@EmadMokhtar
Create a function to replace glog with PlanetScale log
9eb1e47 
Signed-off-by: Emad Habib <ehabib@slack-corp.com>
@vitess-bot
Copy link
Contributor

vitess-bot bot commented Dec 14, 2022 •
edited by GuptaManan100
Loading

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

  • Ensure that the Pull Request has a descriptive title.
  • If this is a change that users need to know about, please apply the release notes (needs details) label so that merging is blocked unless the summary release notes document is included.

If a new flag is being introduced:

  • Is it really necessary to add this flag?
  • Flag names should be clear and intuitive (as far as possible)
  • Help text should be descriptive.
  • Flag names should use dashes (-) as word separators rather than underscores (_).

If a workflow is added or modified:

  • Each item in Jobs should be named in order to mark it as required.
  • If the workflow should be required, the maintainer team should be notified.

Bug fixes

  • There should be at least one unit or end-to-end test.
  • The Pull Request description should include a link to an issue that describes the bug.

Non-trivial changes

  • There should be some code comments as to why things are implemented the way they are.

New/Existing features

  • Should be documented, either by modifying the existing documentation or creating new documentation.
  • New features should have a link to a feature request issue or an RFC that documents the use cases, corner cases and test cases.

Backward compatibility

  • Protobuf changes should be wire-compatible.
  • Changes to _vt tables and RPCs need to be backward compatible.
  • vtctl command output order should be stable and awk-able.
  • RPC changes should be compatible with vitess-operator
  • If a flag is removed, then it should also be removed from VTop, if used there.

EmadMokhtar and others added 5 commits December 14, 2022 18:24
@EmadMokhtar
Merge remote-tracking branch 'upstream/main'
3af20ab 
Signed-off-by: Emad Habib <ehabib@slack-corp.com>
@EmadMokhtar
Merge branch 'vitessio:main' into main
9efb9a3
@EmadMokhtar
Add test case for replacing glog
91a600d 
Signed-off-by: Emad Habib <ehabib@slack-corp.com>
@EmadMokhtar
Remove one test
e317113 
Signed-off-by: Emad Habib <ehabib@slack-corp.com>
@EmadMokhtar
Add flag for PS Logger usage
cf25de9 
Signed-off-by: Emad Habib <ehabib@slack-corp.com>
@EmadMokhtar EmadMokhtar marked this pull request as ready for review December 16, 2022 09:45
@EmadMokhtar EmadMokhtar changed the title Create a function to replace glog with PlanetScale log Add structure logging to Vitess Dec 16, 2022
GuptaManan100
GuptaManan100 approved these changes Dec 20, 2022
View reviewed changes
Copy link
Member

@GuptaManan100 GuptaManan100 left a comment

Choose a reason for hiding this comment

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

otherwise looks good to me

ajm188
ajm188 reviewed Dec 20, 2022
View reviewed changes
go/vt/logutil/planetscale_logger.go Outdated Show resolved Hide resolved
go/vt/logutil/planetscale_logger_test.go Outdated Show resolved Hide resolved
@EmadMokhtar
Update the usage test files and replace _ with - for the flag
9453797 
Signed-off-by: Emad Habib <ehabib@slack-corp.com>
@EmadMokhtar
Apply code review comments
eb47a09 
Signed-off-by: Emad Habib <ehabib@slack-corp.com>
@EmadMokhtar
Copy link
Contributor Author

I applied all your comments

@harshit-gangal harshit-gangal added Type: Enhancement Logical improvement (somewhere between a bug and feature) Component: General Changes throughout the code base release notes (needs details) This PR needs to be listed in the release notes in a dedicated section (deprecation notice, etc...) labels Dec 21, 2022
@EmadMokhtar EmadMokhtar requested review from systay and ajm188 and removed request for systay and ajm188 December 23, 2022 15:01
@EmadMokhtar
Update the year in the copyrights
4dcaf16 
Signed-off-by: Emad Habib <ehabib@slack-corp.com>
@EmadMokhtar EmadMokhtar requested review from systay and GuptaManan100 and removed request for systay January 5, 2023 17:45
@systay systay removed the release notes (needs details) This PR needs to be listed in the release notes in a dedicated section (deprecation notice, etc...) label Jan 9, 2023
@EmadMokhtar
Fix typo
6d451d4 
Signed-off-by: Emad Habib <ehabib@slack-corp.com>
@systay
Copy link
Collaborator

systay commented Jan 10, 2023

Looks like Run endtoend tests on Cluster (onlineddl_vrepl) mysql57 is not starting. Would you mind merging in upstream/main and see if that fixes this issue?

@EmadMokhtar
Merge remote-tracking branch 'upstream/main'
e388bf8 
Signed-off-by: Emad Habib <ehabib@slack-corp.com>
@EmadMokhtar
Empty Commit
f52977d 
Signed-off-by: Emad Habib <ehabib@slack-corp.com>
@frouioui frouioui merged commit c249696 into vitessio:main Jan 25, 2023
@EmadMokhtar EmadMokhtar mentioned this pull request Jan 26, 2023
Open
frouioui added a commit to planetscale/vitess that referenced this pull request Feb 2, 2023
@frouioui
Revert changes made in vitessio#11960
5cfcc86 
Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
@frouioui frouioui mentioned this pull request Feb 2, 2023
Merged
3 tasks
frouioui added a commit that referenced this pull request Feb 2, 2023
@frouioui
Revert changes made in #11960 (#12219)
f5698a3 
Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
@EmadMokhtar EmadMokhtar mentioned this pull request May 10, 2023
Closed
3 tasks
timvaillancourt pushed a commit to slackhq/vitess that referenced this pull request Mar 25, 2024
@EmadMokhtar @timvaillancourt
Add structure logging to Vitess (vitessio#11960)
1977d49 
* Create a function to replace glog with PlanetScale log

Signed-off-by: Emad Habib <ehabib@slack-corp.com>

* Add test case for replacing glog

Signed-off-by: Emad Habib <ehabib@slack-corp.com>

* Remove one test

Signed-off-by: Emad Habib <ehabib@slack-corp.com>

* Add flag for PS Logger usage

Signed-off-by: Emad Habib <ehabib@slack-corp.com>

* Update the usage test files and replace _ with - for the flag

Signed-off-by: Emad Habib <ehabib@slack-corp.com>

* Apply code review comments

Signed-off-by: Emad Habib <ehabib@slack-corp.com>

* Add copyrights and release notes

Signed-off-by: Emad Habib <ehabib@slack-corp.com>

* Update the year in the copyrights

Signed-off-by: Emad Habib <ehabib@slack-corp.com>

* Fix typo

Signed-off-by: Emad Habib <ehabib@slack-corp.com>

* Empty Commit

Signed-off-by: Emad Habib <ehabib@slack-corp.com>

Signed-off-by: Emad Habib <ehabib@slack-corp.com>
@timvaillancourt timvaillancourt mentioned this pull request Mar 25, 2024
Merged
4 tasks
timvaillancourt added a commit to slackhq/vitess that referenced this pull request Mar 26, 2024
@timvaillancourt @EmadMokhtar
slack-vitess-r15.0.5: backport structured logging from vitessio#11960…
453ee84 
… (#270)

* Add structure logging to Vitess (vitessio#11960)

* Create a function to replace glog with PlanetScale log

Signed-off-by: Emad Habib <ehabib@slack-corp.com>

* Add test case for replacing glog

Signed-off-by: Emad Habib <ehabib@slack-corp.com>

* Remove one test

Signed-off-by: Emad Habib <ehabib@slack-corp.com>

* Add flag for PS Logger usage

Signed-off-by: Emad Habib <ehabib@slack-corp.com>

* Update the usage test files and replace _ with - for the flag

Signed-off-by: Emad Habib <ehabib@slack-corp.com>

* Apply code review comments

Signed-off-by: Emad Habib <ehabib@slack-corp.com>

* Add copyrights and release notes

Signed-off-by: Emad Habib <ehabib@slack-corp.com>

* Update the year in the copyrights

Signed-off-by: Emad Habib <ehabib@slack-corp.com>

* Fix typo

Signed-off-by: Emad Habib <ehabib@slack-corp.com>

* Empty Commit

Signed-off-by: Emad Habib <ehabib@slack-corp.com>

Signed-off-by: Emad Habib <ehabib@slack-corp.com>

* github.com/planetscale/log v0.0.0-20221118170849-fb599bc35c50

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* go mod tidy after rebase

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

---------

Signed-off-by: Emad Habib <ehabib@slack-corp.com>
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Co-authored-by: Emad Mokhtar Elsayed Habib <emad.m.habib@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ajm188 ajm188 ajm188 left review comments

@systay systay systay approved these changes

@GuptaManan100 GuptaManan100 GuptaManan100 approved these changes

@frouioui frouioui frouioui approved these changes

Assignees
No one assigned
Labels
Component: General Changes throughout the code base Type: Enhancement Logical improvement (somewhere between a bug and feature)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@EmadMokhtar @systay @ajm188 @frouioui @GuptaManan100 @harshit-gangal