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

Use a throttled logger for exceeded memory warnings #15424

Merged
merged 3 commits into from
Mar 26, 2024
Merged

Use a throttled logger for exceeded memory warnings #15424

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
eecffd2
feat: create a throttled logger and use that for exeeced memory warnings
GuptaManan100 Mar 8, 2024
1b6c139
return a mysql warning
frouioui Mar 12, 2024
d27dec5
fix typo
frouioui Mar 13, 2024
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
7 changes: 5 additions & 2 deletions go/vt/vtgate/executor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ import (
"vitess.io/vitess/go/vt/callerid"
"vitess.io/vitess/go/vt/key"
"vitess.io/vitess/go/vt/log"
"vitess.io/vitess/go/vt/logutil"
binlogdatapb "vitess.io/vitess/go/vt/proto/binlogdata"
querypb "vitess.io/vitess/go/vt/proto/query"
topodatapb "vitess.io/vitess/go/vt/proto/topodata"
Expand Down Expand Up @@ -73,6 +74,8 @@ var (

queriesProcessedByTable = stats.NewCountersWithMultiLabels("QueriesProcessedByTable", "Queries processed at vtgate by plan type, keyspace and table", []string{"Plan", "Keyspace", "Table"})
queriesRoutedByTable = stats.NewCountersWithMultiLabels("QueriesRoutedByTable", "Queries routed from vtgate to vttablet by plan type, keyspace and table", []string{"Plan", "Keyspace", "Table"})

exceedMemoryRowsLogger = logutil.NewThrottledLogger("ExceedMemoryRows", 1*time.Minute)
)

const (
Expand Down Expand Up @@ -231,7 +234,7 @@ func (e *Executor) Execute(ctx context.Context, mysqlCtx vtgateservice.MySQLConn
if err != nil {
piiSafeSQL = logStats.StmtType
}
log.Warningf("%q exceeds warning threshold of max memory rows: %v. Actual memory rows: %v", piiSafeSQL, warnMemoryRows, len(result.Rows))
exceedMemoryRowsLogger.Warningf("%q exceeds warning threshold of max memory rows: %v. Actual memory rows: %v", piiSafeSQL, warnMemoryRows, len(result.Rows))
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this throttle only on the exact matching len(result.Rows) or also on different lengths? And should it only throttle on exactly the same or should we also throttle if it's a slightly different value?

Copy link
Member

Choose a reason for hiding this comment

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

It feels like the throttler will throttle all max memory warning regardless of the query. I think it would be more useful if we could throttle based on the query we are receiving rather than just the error.

Copy link
Member

@deepthi deepthi Mar 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Right now it will throttle all log messages that are generated by this line. That is, it might be a different query, or it might be a different number of rows. It will only print one message a minute.
I agree that it will be nice to print one message per distinct query. That will need writing a new kind of ThrottledLogger. I don't know if that is worth it. If we believe it is, then let's do it.

Copy link
Member

Choose a reason for hiding this comment

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

On second thought, maybe we should just throttle like before and focus on making sure that this is returned as a warning to the client. With mysql clients, that should be pretty easy. What do we do for grpc clients?

}

logStats.SaveEndTime()
Expand Down Expand Up @@ -365,7 +368,7 @@ func (e *Executor) StreamExecute(
if err != nil {
piiSafeSQL = logStats.StmtType
}
log.Warningf("%q exceeds warning threshold of max memory rows: %v. Actual memory rows: %v", piiSafeSQL, warnMemoryRows, srr.rowsReturned)
exceedMemoryRowsLogger.Warningf("%q exceeds warning threshold of max memory rows: %v. Actual memory rows: %v", piiSafeSQL, warnMemoryRows, srr.rowsReturned)
}

logStats.SaveEndTime()
Expand Down
Loading