sql/sem/builtins: fix span double finish#82532
sql/sem/builtins: fix span double finish#82532craig[bot] merged 4 commits intocockroachdb:masterfrom
Conversation
|
This change is |
andreimatei
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @Santamaura)
pkg/sql/sem/builtins/generator_probe_ranges.go line 157 at r1 (raw file):
}) if err != nil { return nil, errors.Wrapf(err, "%s", sp.FinishAndGetConfiguredRecording().String())
I'm pretty sure Wrapf is not the right thing to use here, but I don't know what is. Wrapf is supposed to take a relatively small message. Here the trace will probably completely obscure the error.
There's errors.WithDetail, but then the detail only gets printed if the error is rendered with %+v. I doubt that a SQL client gets a %+v rendering.
Perhaps we should implement our own error type to control the rendering.
@knz what's the right thing to do?
knz
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sem/builtins/generator_probe_ranges.go line 157 at r1 (raw file):
There's errors.WithDetail, I doubt that a SQL client gets a %+v rendering.
It doesn't get a %+v rendering, but it gets the details! (as a pg DETAILS payload). So maybe the details is the right way to do this. Have you tried?
611e9d1 to
e451034
Compare
andreimatei
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sem/builtins/generator_probe_ranges.go line 157 at r1 (raw file):
Previously, knz (kena) wrote…
There's errors.WithDetail, I doubt that a SQL client gets a %+v rendering.
It doesn't get a %+v rendering, but it gets the details! (as a pg DETAILS payload). So maybe the details is the right way to do this. Have you tried?
yup, neato. Massaged this a bit and now it looks nice:
root@localhost:26257/defaultdb> select * from crdb_internal.probe_ranges(INTERVAL '1000ms', 'read') limit 1;
ERROR: error scanning meta ranges: !!! xxx
DETAIL: trace:
0.000ms 0.000ms === operation:meta2scan _verbose:1 client:127.0.0.1:40314 node:1 user:root
0.009ms 0.009ms === operation:txn coordinator send _verbose:1 client:127.0.0.1:40314 node:1 txnID:fa023bd2-91e3-4a75-94af-a07e537724ef user:root
0.018ms 0.008ms === operation:dist sender send _verbose:1 client:127.0.0.1:40314 node:1 txn:fa023bd2 user:root
0.072ms 0.054ms event:kv/kvclient/kvcoord/range_iter.go:175 [n1,client=127.0.0.1:40314,user=root,txn=fa023bd2] querying next range at /Meta2/"\x00"
0.124ms 0.053ms event:kv/kvclient/kvcoord/dist_sender.go:2032 [n1,client=127.0.0.1:40314,user=root,txn=fa023bd2] r1: sending batch 1 Scan to (n1,s1):1
0.141ms 0.017ms event:rpc/nodedialer/nodedialer.go:161 [n1,client=127.0.0.1:40314,user=root,txn=fa023bd2] sending request to local client
0.147ms 0.005ms === operation:/cockroach.roachpb.Internal/Batch _verbose:1 node:1 span.kind:server
...
This code was also broken - it had a double finish in the error case, which I've now fixed in a prepended commit. PTAL.
e451034 to
87df256
Compare
knz
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status:
The tracing span was finished twice on the error path, which is illegal. Release note: None
We were producing a very ugly error by wrapping a cause with a trace.
The trace, used as a message, completely obscured the error. This patch
makes the trace a "detail", which shows up nicely to SQL clients:
root@localhost:26257/defaultdb> select * from crdb_internal.probe_ranges(INTERVAL '1000ms', 'read') limit 1;
ERROR: error scanning meta ranges: !!! xxx
DETAIL: trace:
0.000ms 0.000ms === operation:meta2scan _verbose:1 client:127.0.0.1:40314 node:1 user:root
0.009ms 0.009ms === operation:txn coordinator send _verbose:1 client:127.0.0.1:40314 node:1 txnID:fa023bd2-91e3-4a75-94af-a07e537724ef user:root
0.018ms 0.008ms === operation:dist sender send _verbose:1 client:127.0.0.1:40314 node:1 txn:fa023bd2 user:root
0.072ms 0.054ms event:kv/kvclient/kvcoord/range_iter.go:175 [n1,client=127.0.0.1:40314,user=root,txn=fa023bd2] querying next range at /Meta2/"\x00"
0.124ms 0.053ms event:kv/kvclient/kvcoord/dist_sender.go:2032 [n1,client=127.0.0.1:40314,user=root,txn=fa023bd2] r1: sending batch 1 Scan to (n1,s1):1
0.141ms 0.017ms event:rpc/nodedialer/nodedialer.go:161 [n1,client=127.0.0.1:40314,user=root,txn=fa023bd2] sending request to local client
0.147ms 0.005ms === operation:/cockroach.roachpb.Internal/Batch _verbose:1 node:1 span.kind:server
Release note: None
Release note: None
The code was shadowing a variable, and jumping through hoops to avoid usiong the old one after the shadow. I think this passed the linter because we have an exception to allow shadowing of variables named "ctx", but the exception is about shadowing a context.Context with another one; here we were shadowing a different type. Release note: None
87df256 to
6dcb02c
Compare
andreimatei
left a comment
There was a problem hiding this comment.
bors r+
Reviewable status:
|
Build succeeded: |
The tracing span was finished twice on the error path, which is illegal.
Release note: None