server: don't print trace if empty #106280
Conversation
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
pkg/server/node.go
Outdated
| pErr.GoError().Error(), sp.GetConfiguredRecording().String()) | ||
| recording := sp.GetConfiguredRecording() | ||
| if recording.Len() != 0 { | ||
| log.Infof(ctx, "batch request %s failed with error: %s\ntrace:\n%s", args.String(), |
There was a problem hiding this comment.
don't we want to see the batch error object even if the recording is empty?
Also, plse print pErr.GoError() and not call .Error(). This will ensure that the non-redactable bits of the error show up in logs properly.
(nit Also use %v for the error. )
There was a problem hiding this comment.
The errors will always be context canceled or context deadline exceeded because we only log the line if ctx.Err() != nil. I don't have a sense for whether this is useful for the HeartbeatTxn, QueryTxn, and EndTxn requests that @erikgrinaker pointed out as spamming the logs in some roachtests?
Thanks for the pointer on the error message format, fixing that up now.
There was a problem hiding this comment.
For ExportRequests by backups (which is what motivated me to add this logline) we always expect a non-empty tracing span as they are sent with a Structured recording. If for some reason there is no trace, we'd see the context cancelled/timed out error on the client side anyways so there isn't much value in printing it here too.
There was a problem hiding this comment.
Yeah, let's err on the side of not logging here. Generally, we only want to see this when we're reasonably sure that there's a human who requested the trace (is there a better way to determine that?). If they trigger for any random request, we'll get thousands of log lines for internal requests that noone cares about.
This change adds a conditional to only print the trace of context cancelled request if the trace is not empty. This avoids log spam noticed in cockroachdb#105378. Fixes: cockroachdb#105378 Release note: None
|
bors r=knz |
|
Build succeeded: |
This change adds a conditional to only print the
trace of context cancelled request if the trace is not empty. This avoids log spam noticed in #105378.
Fixes: #105378
Release note: None