Skip to content

vtgate: ignore MySQL client probe query select $$ in querylog #20073

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

Closed
ChaitanyaD48 wants to merge 2 commits into from
Closed

vtgate: ignore MySQL client probe query select $$ in querylog #20073

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
23767bc
fix: suppress streamlog noise for MySQL 8.4+ connection probe query s…
ChaitanyaD48 May 9, 2026
1f4d8d4
test: add test cases for SQL query filtering with placeholders in str…
ChaitanyaD48 May 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions go/streamlog/streamlog.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,13 @@ var (
deliveredCount = stats.NewCountersWithMultiLabels(
"StreamlogDelivered",
"Stream log delivered",
[]string{"Log", "Subscriber"})
[]string{"Log", "Subscriber"},
)
deliveryDropCount = stats.NewCountersWithMultiLabels(
"StreamlogDeliveryDroppedMessages",
"Dropped messages by streamlog delivery",
[]string{"Log", "Subscriber"})
[]string{"Log", "Subscriber"},
)
)

const (
Expand Down Expand Up @@ -290,6 +292,13 @@ func (qlConfig QueryLogConfig) shouldSampleQuery() bool {
// It also returns an EmitReason which is a comma-separated-string to indicate all the conditions triggered for log emit.
// If both TimeThreshold and FilterTag condition are met, EmitReason will be time,filtertag
func (qlConfig QueryLogConfig) ShouldEmitLog(sql string, rowsAffected, rowsReturned uint64, totalTime time.Duration, hasError bool) (bool, string) {
// MySQL 8.4+ clients send "select $$" at connection time to probe dollar-quote
// syntax support. This is hardcoded client behavior that cannot be disabled;
// suppress it to avoid log noise.
if strings.EqualFold(strings.TrimSpace(sql), "select $$") {
return false, ""
}
Comment on lines +295 to +300
Copy link
Copy Markdown
Member

@mattlord mattlord May 9, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

IMO... clients can send whatever queries they want. It's not up to the Vitess developers to decide what should and should not be logged. It's a log of queries coming from clients.

And IMO every Vitess user should not pay the cost of an EqualFold on every query just to elide it. The use case in the issue was:

For integration load testing I have scripts that spin up a lot of parallel connections to the database, and every time a mysql client initiates a new connection to the server I get a log message in the vtgate log about the invalid query. As it's a hard-coded query that is meaningless (as far as I can tell) and completely out of my control, that is just unnecessary noise in the query log.

IMO we should not modify how Vitess behaves for all users simply so that it fits better into that very narrow use case / scenario, especially when there seems to be a pretty clear performance cost.

Am I missing or misunderstanding something?

Copy link
Copy Markdown
Member

@mattlord mattlord May 11, 2026

Choose a reason for hiding this comment

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

What might make sense though... is providing a Viper config option that allowed users to specify a set of SQL query shapes to ignore in the logs. What do you think about that? Then a user could specify this specific query and any others coming from their clients that they want to elide from the vtgate logs.

Copy link
Copy Markdown
Contributor Author

@ChaitanyaD48 ChaitanyaD48 May 16, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

@mattlord Thanks for the review! Its a Good Idea to have a user configurable viper config option to ignore a set of SQL Query shapes in Logs.

I have some questions around this -

Question 1: How should we "match" a query?

Should we match by normalized shape or exact string? In case of Reading "query shape" as the output of sqlparser.RedactSQLQuery, select 31239 and select 99999 would both collapse to a single entry like select :vtg1 but, select $$ itself doesn't parse, so RedactSQLQuery fails on it. We'd need a fallback for the un-parseable cases. The options I see:

  1. Shape-match when parseable, raw-string fallback otherwise.
  2. Just raw-string match (case/whitespace-insensitive), skipping normalization entirely.

Which approach do you think makes more sense?

And Addressing your performance Concern. I feel this new user-configurable viper config option must sit at the executor level in vtgate, not inside streamlog's ShouldEmitLog. Planning to go with a len() == 0 fast path something like -

func shouldIgnoreInQueryLog(sql string) bool {
    if len(ignoredQueries) == 0 {
        return false   // ← fast path: users who don't opt-in land here
    }
    // expensive part: match sql string,  do map lookup, etc.
    normalized := strings.ToLower(strings.TrimSpace(sql))
    _, found := ignoredQueriesMap[normalized]
    return found
}

for users who don't opt in pay essentially nothing (one int compare), and users who do pay a single map lookup per query. That should keep the cost off everyone except the operators who actually asked for it.

What do you think?

Copy link
Copy Markdown
Member

@mattlord mattlord May 22, 2026

Choose a reason for hiding this comment

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

I think we should do it by shape or the normalized form where the variable parts are replaced by a placeholder.

For the performance part, that sounds good. Thanks! ❤️

Copy link
Copy Markdown
Contributor Author

@ChaitanyaD48 ChaitanyaD48 May 23, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Got it, thanks @mattlord!
I've created a new issue (#20173) and opened a PR (#20174) for the proposed change.

Closing this PR.


var aMatch, allMatches bool
var aReason string
reasons := []string{}
Expand Down
20 changes: 20 additions & 0 deletions go/streamlog/streamlog_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,6 +372,26 @@ func TestShouldEmitLog(t *testing.T) {
ok: true,
emitReason: "error",
},
{
sql: "select $$",
ok: false,
emitReason: "",
},
{
sql: "SELECT $$",
ok: false,
emitReason: "",
},
{
sql: " select $$ ",
ok: false,
emitReason: "",
},
{
sql: "select $$ from dual",
ok: true,
emitReason: "",
},
}

for _, tt := range tests {
Expand Down
Loading