Skip to content
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

kvserver: store.Enqueue shouldn't start verbose tracing #132713

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

andrewbaptist
Copy link
Collaborator

@andrewbaptist andrewbaptist commented Oct 15, 2024

Previously in server.Enqueue, we unconditionally started verbose
tracing. This is expensive and unnessary since most traces were never
processed. Additionally, with the new raft level logging for verbose
traces this resulted in unnecessary traces being created and logged to
the main cockroach.log. This change removes the automatic tracing inside
Enqueue and instead converts tests and callers to explicitly start a
span prior to calling the queue.

As part of this change the Enqueue method no longer returns a Recording.

Epic: none

Release note: None

Copy link

blathers-crl bot commented Oct 15, 2024

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.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@andrewbaptist
Copy link
Collaborator Author

Making this change enables default raft logging as part of #131850.

@andrewbaptist andrewbaptist changed the title kvserver: don't start unrequested verbose logging kvserver: store.Enqueue shouldn't start verbose tracing Oct 16, 2024
@andrewbaptist andrewbaptist force-pushed the 2024-10-15-change-logging branch 2 times, most recently from a25f8ea to df5523f Compare October 16, 2024 18:15
@andrewbaptist andrewbaptist marked this pull request as ready for review October 16, 2024 18:16
@andrewbaptist andrewbaptist requested review from a team as code owners October 16, 2024 18:16
@andrewbaptist andrewbaptist requested review from jeffswenson, angles-n-daemons and rytaft and removed request for a team October 16, 2024 18:16
@rytaft rytaft removed their request for review October 16, 2024 18:25
Previously in server.Enqueue, we unconditionally started verbose
tracing. This is expensive and unnessary since most traces were never
processed. Additionally, with the new raft level logging for verbose
traces this resulted in unnecessary traces being created and logged to
the main cockroach.log. This change removes the automatic tracing inside
Enqueue and instead converts tests and callers to explicitly start a
span prior to calling the queue.

As part of this change the Enqueue method no longer returns a Recording.

Epic: none

Release note: None
@andrewbaptist
Copy link
Collaborator Author

There are only 3 meaningful "non-test" changes, and only the first one changes any behavior.

  • replicate_queue.go where we no longer trace every attempt. This changes behavior in that if the replicate queue is called without a tracing span, we might have less visibility into the underlying problem. However this is probably an acceptable tradeoff. This behavior was added in kvserver: log traces from replicate queue on errors or slow processing #86007.
  • split_queue.go where we will only start a tracing span if log.ExpensiveLogEnabled is set. We would only log at the end if it was anyway, so this shouldn’t change things. I didn’t love how this was done, but didn’t want to increase scope by changing it. This won't change any behavior, but might still log more than we want.
  • store.go - Enqueue no longer constructs a tracing span. Instead callers to it do. This won't change any behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants