-
Notifications
You must be signed in to change notification settings - Fork 3.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
util/tracing: change the default of trace.debug.enable to false #41497
util/tracing: change the default of trace.debug.enable to false #41497
Conversation
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.
unless @tbg has any reservations.
Reviewed 2 of 2 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @tbg)
No reservation
…On Thu, Oct 10, 2019, 20:45 Nathan VanBenschoten ***@***.***> wrote:
***@***.**** approved this pull request.
[image: <img class="emoji" title=":lgtm_strong:" alt=":lgtm_strong:" align="absmiddle" src="https://reviewable.io/lgtm_strong.png" height="20" width="61"/>]
<https://camo.githubusercontent.com/aef7967e4818b7dc97469bd2853c2255784874e2/68747470733a2f2f72657669657761626c652e696f2f6c67746d5f7374726f6e672e706e67>
unless @tbg <https://github.com/tbg> has any reservations.
Reviewed 2 of 2 files at r1.
*Reviewable
<https://reviewable.io/reviews/cockroachdb/cockroach/41497#-:-LqqzBhNCTf-4AqCuFA0:b-bke68v>*
status: [image: ] complete! 1 of 0 LGTMs obtained (waiting on @tbg
<https://github.com/tbg>)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#41497?email_source=notifications&email_token=ABGXPZBVEAHB2NAPLFFK77LQN5Z35A5CNFSM4I7OYXG2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCHS55AY#pullrequestreview-300277379>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABGXPZA2WNM7XN46ZXXCEC3QN5Z35ANCNFSM4I7OYXGQ>
.
|
abe4f4a
to
3e8aa28
Compare
Before this patch, we weirdly defaulted that setting to true and then had a migration setting it to false. The reason for this dance was to trace the node startup (or the cluster bootstrap?) (i.e. the debug/requests web page would show one trace for the cluster bootstrap always). Cute, but I doubt this has been useful recently and is too magic. It also has unintended consequences, as global variables do. Tests and benchmarks that don't start a server end up running with this sort of tracing enabled, which has effects for logging and other things. In particular, I have another change where the size of Raft proposal messages changes some when tracing is enabled. And so, I'd like to get rid of the magic. Release note: None
3e8aa28
to
51a326f
Compare
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.
bors r+
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten and @tbg)
41497: util/tracing: change the default of trace.debug.enable to false r=andreimatei a=andreimatei Before this patch, we weirdly defaulted that setting to true and then had a migration setting it to false. The reason for this dance was to trace the node startup (or the cluster bootstrap?) (i.e. the debug/requests web page would show one trace for the cluster bootstrap always). Cute, but I doubt this has been useful recently and is too magic. It also has unintended consequences, as global variables do. Tests and benchmarks that don't start a server end up running with this sort of tracing enabled, which has effects for logging and other things. In particular, I have another change where the size of Raft proposal messages changes some when tracing is enabled. And so, I'd like to get rid of the magic. Release note: None 41725: pkg: fix formatting for some span tag stats r=andreimatei a=andreimatei They were formatted as bytes. Release note: None Co-authored-by: Andrei Matei <andrei@cockroachlabs.com>
Build succeeded |
Before this patch, we weirdly defaulted that setting to true and then
had a migration setting it to false. The reason for this dance was to
trace the node startup (or the cluster bootstrap?) (i.e. the
debug/requests web page would show one trace for the cluster bootstrap
always). Cute, but I doubt this has been useful recently and is too
magic. It also has unintended consequences, as global variables do.
Tests and benchmarks that don't start a server end up running with this
sort of tracing enabled, which has effects for logging and other things.
In particular, I have another change where the size of Raft proposal
messages changes some when tracing is enabled. And so, I'd like to get
rid of the magic.
Release note: None