ApplySchema: log selected flags

[shlomi-noach]

Description

This simple PR adds more information in ApplySchema invocation, both in vtctl and in vtctldclient.

Related Issue(s)

None.

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

[vitess-bot]

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.

[shlomi-noach]

Made a small change: logging from inside VtctldServer.ApplySchema (server side) instead of in multiple client side endpoints.

[mattlord]

IMO it's better to add these things to the trace span and add stats for specific cases that you want to be able to track.

Our logs are already getting noisy and it's causing practical problems in some cases.

Is a new log message on each invocation really the best option? If so, then of course we can add it. I just wanted to ask.

[shlomi-noach]

ApplySchema invocations are sparse; those are normally human generated per schema change. So on most production systems this would amount to a single digit number per day, or per week. It's also possible to use ApplySchema to control migrations (e.g. ALTER VITESS_MIGRATION ... COMPLETE etc.). But again those are a handful of potential extra calls. So in terms of overhead I think the price is insignificant.

Let me look into trace span though, I don't have a good grasp of how that works and how that differs from logging.

[mattlord]

The trace span would be useful when tracing, a stat would be useful if we want to track X over the course of a process lifespan (e.g. ApplySchema statements with option X). I'm OK with adding the log message, just wondering if it's really the best option in this case. Let me know what you think.

[shlomi-noach]

To be honest, I don't dig how tracing or stats would be useful here. I understand that our logging is sometimes bloated, but in this particular case I don't find it to be an issue, as schema changes are relatively rare and human generated.

[mattlord]

Why do we think this log message will be helpful/useful, and to whom? What do we hope to get from it? I don't see that stated clearly, but the subject says: log selected flags

So I'm guessing we want to know how many times given flags are specified per keyspace? That's the kind of thing a multi-label stat can provide. But I also don't know if this is thought to be useful in debugging a known issue and may soon be removed (i.e. it's only useful to Vitess developers) or if we think this kind of information is generally usable (useful to Vitess users/administrators). If it's the former then the log makes sense to me, if it's the latter then I think stats are better.

I know we don't think that ApplySchema is called very often, but we've thought this about various things in the past and been wrong -- at least when you consider aggregated logging with up to 1,000s of tablets involved.

[shlomi-noach]

You're right, I added no context to this change. The purpose is not to do a weekly summary of how many this and that calls were made. The purpose is to be able to debug migrations that go wrong/fail, and to see what the exact invocation command was used, so we can try and reproduce them.

[mattlord]

Makes sense to have this as a means to debug unexpected behavior for something that shouldn't be called very often. Thanks!