Skip to content

Remove glog logging support - #21309

Open
mhamza15 wants to merge 31 commits into
vitessio:mainfrom
mhamza15:remove-glog
Open

mhamza15 wants to merge 31 commits into
vitessio:mainfrom
mhamza15:remove-glog

Conversation

@mhamza15

@mhamza15 mhamza15 commented Sep 30, 2026 •

Copy link
Copy Markdown
Collaborator

Description

Removes glog logging support, which was deprecated in v24. slog is the only logging backend.

This PR removes the following.

  • the github.com/golang/glog dependency
  • the glog flags from binaries, examples, and flag snapshots
  • the TrickGlog startup shim and the -v shorthand workaround. In vtctldclient, -v is the shorthand for --version, the same as the other binaries.
  • glog log-file purging
  • log.V, log.Level, log.Verbose, and log.Flush. Former V-level messages use log.Debug.
  • logutil.Flush and logutil.OnFlush
  • the *pflag.FlagSet parameter of log.Init

v24 did not show a deprecation warning for some logging flags and for the /debug/flushlogs endpoint. This PR keeps them as no-ops that show a deprecation warning until v26.

  • --log-structured, --log-rotate-max-size, --keep-logs, --keep-logs-by-mtime, and --purge-logs-interval
  • --logtostderr and --alsologtostderr on vtctldclient and vtctlclient
  • /debug/flushlogs

A binary fails to start with --log-structured=false, because that value asks for glog log files.

The Vitess tester workflow installs the tester from vitessio/vt#100. The earlier tester version starts vtctld with --log_dir.

The v25 release notes list every removed and deprecated flag.

Related Issue(s)

Closes #21310

Checklist

  • "Backport to:" labels have been added if this change should be back-ported to release branches
  • If this change is to be back-ported to previous releases, a justification is included in the PR description
  • Tests were added or are not required
  • Did the new or modified tests pass consistently locally and on CI?
  • Documentation was added or is not required

Deployment Notes

Binaries fail to start when passed a removed logging flag or --log-structured=false. Remove these flags from startup arguments before upgrading. The v25 release notes list them.

AI Disclosure

This PR was written by Claude with user direction.

`glog` was deprecated in v24 and scheduled for removal in v25. Keeping both logging backends leaves legacy flags, startup shims such as `TrickGlog`, glog log-file purging, and the `log.V` and `log.Flush` compatibility APIs in the codebase.

This removes the `glog` dependency and its compatibility paths, leaving `slog` as the only logging backend. Former V-level messages use `log.Debug`, and gRPC and Azure verbosity checks use `log.Enabled`. The `--log-structured`, `--log-rotate-max-size`, `--logtostderr`, `--alsologtostderr`, `--stderrthreshold`, `--log_dir`, `--log_link`, `--log_backtrace_at`, `--v`, `--vmodule`, `--logbuflevel`, `--keep-logs`, `--keep-logs-by-mtime`, and `--purge-logs-interval` flags are removed from binaries, examples, and flag snapshots, and the v25 release notes document the breaking change.

Signed-off-by: Mohamed Hamza <mhamza@fastmail.com>
Copilot AI balanced review requested due to automatic review settings September 30, 2026 12:43
@github-actions github-actions Bot added this to the v25.0.0 milestone Sep 30, 2026
@vitess-bot vitess-bot Bot added NeedsWebsiteDocsUpdate What it says NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsIssue A linked issue is missing for this Pull Request NeedsBackportReason If backport labels have been applied to a PR, a justification is required labels Sep 30, 2026
@vitess-bot

vitess-bot Bot commented Sep 30, 2026

Copy link
Copy Markdown
Contributor

Review Checklist

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

General

  • Ensure that the Pull Request has a descriptive title.
  • Ensure there is a link to an issue (except for internal cleanup and flaky test fixes), new features should have an RFC that documents use cases and test cases.

Tests

  • Bug fixes should have at least one unit or end-to-end test, enhancement and new features should have a sufficient number of tests.

Documentation

  • Apply the release notes (needs details) label if users need to know about this change.
  • New features should be documented.
  • There should be some code comments as to why things are implemented the way they are.
  • There should be a comment at the top of each new or modified test to explain what the test does.

New flags

  • Is this flag really necessary?
  • Flag names must be clear and intuitive, use dashes (-), and have a clear help text.

If a workflow is added or modified:

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

Backward compatibility

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

@mhamza15

Copy link
Copy Markdown
Collaborator Author

Thanks for the review. Updated .agents/skills/e2e-tests/SKILL.md to point at the stderr files (5ecb9b3). I also removed a dead mysqlctl.INFO read in the vreplication cluster setup.

Signed-off-by: Mohamed Hamza <mhamza@fastmail.com>
Copilot AI balanced review requested due to automatic review settings September 30, 2026 16:49

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

The broad public API, binary flag, logging, CI, and operational changes warrant final human validation.

Review effort: Balanced
Findings: None

# Conflicts:
#	go/vt/vttablet/tabletmanager/vreplication/vplayer_flaky_test.go
Copilot AI balanced review requested due to automatic review settings September 30, 2026 17:03

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

The cross-cutting removal changes public APIs, binary flags, operational logging, CI, and numerous deployment paths.

Review effort: Balanced
Findings: None

@mattlord mattlord 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.

Blocking: I think we should fix the reference-MySQL setup before adopting the tester pin in .github/workflows/vitess_tester_vtgate.yml:108. At c19060c, setupExternalMySQL calls utils.NewMySQL on every keyspace iteration and returns the last server’s connection, so the databases no longer share one reference server. The current tester job confirms this: four queries in two_sharded_keyspaces/queries.test fail with Unknown database '\''customer'\'' on MySQL. The previous pin created both databases on one server. Fixing that setup in vitessio/vt, adding multi-keyspace coverage, and updating the pin should address the remaining failure.

Non-blocking: I also agree with Arthur’s misleading underscore-normalization hint concern; handling unknown flags consistently in a follow-up seems reasonable.

The pinned Vitess tester starts vtctld with `--log_dir`. vitessio/vt#100 upgrades its Vitess dependency and starts one reference MySQL server for all keyspaces. This installs the tester from the merge commit of that pull request.

Signed-off-by: Mohamed Hamza <mhamza@fastmail.com>
Copilot AI balanced review requested due to automatic review settings October 1, 2026 15:37
@mhamza15

mhamza15 commented Oct 1, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks, good catch. vitessio/vt#100 now starts one reference server for all keyspaces and adds a two-keyspace test. It has merged, and the tester pin here points at the merge commit (e13ac21).

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

The change removes public APIs and operational flags across many binaries, tests, and deployment paths, warranting final human validation.

Review effort: Balanced
Findings: None

v24 did not show a deprecation warning for `--log-structured`, `--log-rotate-max-size`, `--keep-logs`, `--keep-logs-by-mtime`, and `--purge-logs-interval`, or for `--logtostderr` and `--alsologtostderr` on `vtctldclient` and `vtctlclient`. Removing them in v25 stops a binary from starting with startup arguments that showed no warning.

This keeps these flags as hidden no-ops that show a deprecation warning until v26. `Init` fails on `--log-structured=false`, because that value asks for `glog` log files.

Signed-off-by: Mohamed Hamza <mhamza@fastmail.com>
v24 did not show a deprecation warning for the `/debug/flushlogs` HTTP endpoint. Removing it in v25 makes scripts that call it fail with a 404.

This keeps the endpoint until v26. It responds with `flushed` and logs a deprecation warning.

Signed-off-by: Mohamed Hamza <mhamza@fastmail.com>
No binary writes the glog `vtcombo.INFO` file. `TestCanGetKeyspaces` prints only the MySQL error log on failure.

Signed-off-by: Mohamed Hamza <mhamza@fastmail.com>
The release notes cover the command-line and HTTP interfaces. Vitess makes no compatibility promise for its Go packages.

Signed-off-by: Mohamed Hamza <mhamza@fastmail.com>
`vtctldclient` adds the standard library `flag.CommandLine` to its flags, and `_flag.Lookup` reads it. `glog` was the only package that registered flags there, and no caller of `_flag.Lookup` remains. This removes both.

Signed-off-by: Mohamed Hamza <mhamza@fastmail.com>
Steps 5 and 6 both read the `*-stderr.txt` files. One step covers startup and runtime errors.

Signed-off-by: Mohamed Hamza <mhamza@fastmail.com>
Signed-off-by: Mohamed Hamza <mhamza@fastmail.com>
Copilot AI balanced review requested due to automatic review settings October 1, 2026 15:54

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

The broad removal changes startup compatibility and logging behavior across many binaries, tests, examples, and an external workflow pin, warranting final human validation.

Review effort: Balanced
Findings: None

Signed-off-by: Mohamed Hamza <mhamza@fastmail.com>

# Conflicts:
#	go.sum
Copilot AI balanced review requested due to automatic review settings October 1, 2026 15:59

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

The cross-cutting logging, flag-compatibility, and operational changes affect nearly every binary and require final human validation.

Review effort: Balanced
Findings: None

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Breaking change Component: Observability Pull requests that touch tracing/metrics/monitoring Type: Internal Cleanup

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feature Request: Remove glog logging support

4 participants