-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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.
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
executor, session: replace new line and add user in query log #6748
Conversation
So we can grep a keyword to get the query, without the need to look up the query in the full log. user name is also needed for auditing purpose.
executor/adapter.go
Outdated
"[QUERY] cost_time:%v succ:%v connection_id:%v txn_start_ts:%v database:%v table_ids:%v index_ids:%v sql:%v", | ||
costTime, succ, connID, txnTS, currentDB, tableIDs, indexIDs, sql) | ||
"[QUERY] cost_time:%v succ:%v connection_id:%v user:%s txn_start_ts:%v database:%v %v%vsql:%v", | ||
costTime, succ, connID, user, txnTS, currentDB, tableIDs, indexIDs, sql) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the user contain host-ip?
executor/adapter.go
Outdated
@@ -324,6 +324,9 @@ func (a *ExecStmt) buildExecutor(ctx sessionctx.Context) (Executor, error) { | |||
return e, nil | |||
} | |||
|
|||
// QueryReplacer replaces new line and tab for grep result including query string. | |||
var QueryReplacer = strings.NewReplacer("\r", "", "\n", "", "\t", " ") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about there is a newline or tab in the string literal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be escaped as three characters `\' '\' 'n' instead of a single character '\n'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This query is used for logging, it's ok even if we replace new line in the string.
session/session.go
Outdated
@@ -1468,6 +1468,7 @@ func logStmt(node ast.StmtNode, vars *variable.SessionVars) { | |||
|
|||
func logQuery(query string, vars *variable.SessionVars) { | |||
if atomic.LoadUint32(&variable.ProcessGeneralLog) != 0 && !vars.InRestrictedSQL { | |||
log.Infof("[con:%d][schema ver:%d][txn:%d] %s", vars.ConnectionID, vars.TxnCtx.SchemaVersion, vars.TxnCtx.StartTS, query) | |||
query = executor.QueryReplacer.Replace(query) | |||
log.Infof("[con:%d][user:%s][schema_ver:%d][txn:%d] %s", vars.ConnectionID, vars.User, vars.TxnCtx.SchemaVersion, vars.TxnCtx.StartTS, query) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you post an example of the new log?
general log example: 2018/06/05 10:19:29.944 session.go:1472: [info] [GENERAL_LOG] con:2 user:root@127.0.0.1 schema_ver:15 start_ts:0 sql:select 1 |
@@ -1457,9 +1457,9 @@ func logStmt(node ast.StmtNode, vars *variable.SessionVars) { | |||
user := vars.User | |||
schemaVersion := vars.TxnCtx.SchemaVersion | |||
if ss, ok := node.(ast.SensitiveStmtNode); ok { | |||
log.Infof("[CRUCIAL OPERATION] [con:%d][schema ver:%d] %s (by %s).", vars.ConnectionID, schemaVersion, ss.SecureText(), user) | |||
log.Infof("[CRUCIAL OPERATION] con:%d schema_ver:%d %s (by %s).", vars.ConnectionID, schemaVersion, ss.SecureText(), user) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to make the same changes to unify the log format in line 503?
@zimulala PTAL |
currentDB := sessVars.CurrentDB | ||
var tableIDs, indexIDs string | ||
if len(sessVars.StmtCtx.TableIDs) > 0 { | ||
tableIDs = strings.Replace(fmt.Sprintf("table_ids:%v ", a.Ctx.GetSessionVars().StmtCtx.TableIDs), " ", ",", -1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you want to replace spaces with commas? I think it has a certain loss of performance. If it is not necessary, it may not be done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is useful when we need to use ' ' as fields separator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or should we Itoa
to []string
and strings.Join
if performance is critial?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The performance is not critical here, we better use a simpler solution.
will look like
|
@jackysp PTAL |
executor/adapter.go
Outdated
@@ -324,6 +324,9 @@ func (a *ExecStmt) buildExecutor(ctx sessionctx.Context) (Executor, error) { | |||
return e, nil | |||
} | |||
|
|||
// QueryReplacer replaces new line and tab for grep result including query string. | |||
var QueryReplacer = strings.NewReplacer("\r", "", "\n", " ", "\t", " ") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe use var QueryReplacer = strings.NewReplacer("\r", " ", "\n", " ", "\t", " ")
although give more 1 space ?
if len(new) == len(old) == 1 replace will be more quicker~
func BenchmarkPureBinary(b *testing.B) {
var r = strings.NewReplacer(",", " ", "-", " ")
for i := 0; i < b.N; i++ {
a := r.Replace("sadfsadfa,sdfsd,dsfsa,sdfsad,dsfsa-sdfa-sdf-dsfad-fsadf-sa,sadfas")
a = a
}
}
func BenchmarkBinaryString(b *testing.B) {
var r = strings.NewReplacer(",", "", "-", "")
for i := 0; i < b.N; i++ {
a := r.Replace("sadfsadfa,sdfsd,dsfsa,sdfsad,dsfsa-sdfa-sdf-dsfad-fsadf-sa,sadfas")
a = a
}
}
BenchmarkPureBinary-8 10000000 144 ns/op 160 B/op 2 allocs/op
BenchmarkBinaryString-8 5000000 267 ns/op 128 B/op 2 allocs/op
currentDB := sessVars.CurrentDB | ||
var tableIDs, indexIDs string | ||
if len(sessVars.StmtCtx.TableIDs) > 0 { | ||
tableIDs = strings.Replace(fmt.Sprintf("table_ids:%v ", a.Ctx.GetSessionVars().StmtCtx.TableIDs), " ", ",", -1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or should we Itoa
to []string
and strings.Join
if performance is critial?
if costTime < threshold { | ||
logutil.SlowQueryLogger.Debugf( | ||
"[QUERY] cost_time:%v succ:%v connection_id:%v txn_start_ts:%v database:%v table_ids:%v index_ids:%v sql:%v", | ||
costTime, succ, connID, txnTS, currentDB, tableIDs, indexIDs, sql) | ||
"[QUERY] cost_time:%v succ:%v con:%v user:%s txn_start_ts:%v database:%v %v%vsql:%v", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe not use %v
if it know real type~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it doesn't matter much.
@lysu PTAL |
@zimulala PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
LGTM |
/run-all-tests |
/run-unit-test |
So we can grep a keyword to get the query, without the need to look up the query in the full log.
user name is also needed for auditing purpose.
Fixes #6673