Add source_stmt in Context for test logging.
#1103
Conversation
Signed-off-by: ekexium <[email protected]>
Signed-off-by: ekexium <[email protected]>
Signed-off-by: ekexium <[email protected]>
proto/kvrpcpb.proto
Outdated
| @@ -916,6 +920,11 @@ enum APIVersion { | |||
| V2 = 2; | |||
| } | |||
|
|
|||
| message TidbSource { | |||
There was a problem hiding this comment.
How about QueryCtx or StmtCtx? Our goal is to establish a connection between the execution flow of transactions or statements.
connection_id may not be globally unique, to clarify which query the request belongs to something like stmt_id or uuid is needed but it looks a bit complex.
There was a problem hiding this comment.
I think ctx is too general and it can cover almost everything... How about source_stmt?
stmt_id is a good idea. We can simply assign an incrementing id in each session, and print it together with conn_id in general log.
ekexium
changed the title
source_stmt in Context for test logging.
proto/kvrpcpb.proto
Outdated
|
|
||
| // It tells us where the request comes from in TiDB. If it isn't from TiDB, leave it blank. | ||
| // This is for tests only and thus can be safely changed/removed without affecting compatibility. | ||
| SourceStmt source_stmt = 33; |
There was a problem hiding this comment.
If this field is only used for logging with context info, I think a formated string is enough (just like request_source).
There was a problem hiding this comment.
IMO string is less structured and less flexible.
There was a problem hiding this comment.
What I'm concerned about is: do we really have to introduce some sql concepts (stmt_id, conn_id) to the kv rpc?
Besides, I think string is more flexible, it's allow callers define/update the context info freely. The drawback of string is that it's not compacted.
There was a problem hiding this comment.
do we really have to introduce some sql concepts (stmt_id, conn_id) to the kv rpc?
Yeah I agree. I hope we could use something like SpanContext, making it part of the tracing framework. However that's kinda infeasible for now.
it's allow callers define/update the context info freely
True. But I don't quite view it as a pure advantage. If it changes often, we will easily forget what is included and have to jump forth and back in code to find out. I'd prefer an explicit definition to avoid such effort.
By flexible I mean TiKV can't do anything except simply printing a string, like structured logging, filtering, etc.
| uint64 start_ts = 1; | ||
| uint64 connection_id = 2; | ||
| uint64 stmt_id = 3; | ||
| } |
There was a problem hiding this comment.
Since the connection_id can be identical across different TiDB servers, it may be necessary to use a unique identifier such as uuid to indicate which statement and instance this query originates from.
Besides, how should the stmt_id be generated?
There was a problem hiding this comment.
how should the stmt_id be generated
Each connection generates its own stmt_id.
Merging them together into a UUID can make it harder for humans to read. Since it's only used in tests, the performance gain doesn't matter a lot.
Yes.
Makes sense. Since it's only used in tests, I still prefer more simplicity at the cost of redundancy. |
|
We can wait until the corresponding TiKV PR completes its review before merging this PR |
Signed-off-by: ekexium <[email protected]>
ref tikv/tikv#14474