Skip to content

kvserver: annotate Replica.Send with pprof labels#86130

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
pav-kv:replica_send_pprof
Aug 17, 2022
Merged

kvserver: annotate Replica.Send with pprof labels#86130
craig[bot] merged 1 commit intocockroachdb:masterfrom
pav-kv:replica_send_pprof

Conversation

@pav-kv
Copy link
Copy Markdown
Collaborator

@pav-kv pav-kv commented Aug 15, 2022 •
edited
Loading

This commit adds pprof labels to Send methods, as a proper replacement of the
no longer working (since Go 1.17) unnamed parameters hack. Adding labels is
conditional to the CPU profile cluster setting, to avoid the profiling cost and
extra allocations in pprof.Do context construction during normal operation.

Fixes #85948

Release justification: Profiling labels help investigating customer issues
Release note: None

@pav-kv pav-kv requested a review from tbg August 15, 2022 12:19
@pav-kv pav-kv requested a review from a team as a code owner August 15, 2022 12:19
@cockroach-teamcity
Copy link
Copy Markdown
Member

cockroach-teamcity commented Aug 15, 2022

This change is Reviewable

@pav-kv pav-kv force-pushed the replica_send_pprof branch from e45784b to db3e7ea Compare August 15, 2022 12:20
pav-kv
pav-kv commented Aug 15, 2022
View reviewed changes
tbg
tbg requested changes Aug 15, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@tbg tbg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically looks good! I think we should avoid pprof.Do here but functionally this will do it. Mind trying it out, too, and posting results for eye candy?

roachprod create local -n 3
roachprod put local ./cockroach
roachprod start local
./cockroach workload init kv --splits 500
./cockroach workload run kv --sequential --read-percent 95

should show up as a nice hotspot on one range in the profile.

@pav-kv pav-kv force-pushed the replica_send_pprof branch 2 times, most recently from d55c0f6 to 77b7193 Compare August 15, 2022 15:51
@pav-kv pav-kv requested a review from tbg August 15, 2022 15:52
@pav-kv
Copy link
Copy Markdown
Collaborator Author

pav-kv commented Aug 15, 2022

should show up as a nice hotspot on one range in the profile.

...

 range_str: Total 280.0ms
            270.0ms (96.43%): 416/1:/Table/114/1/{-18409…-41552}
             10.0ms ( 3.57%): 40/1:/Table/{39-40}

...

@pav-kv
Copy link
Copy Markdown
Collaborator Author

pav-kv commented Aug 16, 2022

PTAL

tbg
tbg approved these changes Aug 17, 2022
View reviewed changes
@pav-kv pav-kv force-pushed the replica_send_pprof branch from 77b7193 to d6db017 Compare August 17, 2022 10:54
@pav-kv
kvserver: annotate Replica.Send with pprof labels
9869df0 
This commit adds pprof labels to Send methods, as a proper replacement of the
no longer working (since Go 1.17) unnamed parameters hack. Adding labels is
conditional to the CPU profile cluster setting, to avoid the profiling cost and
extra allocations in pprof.Do context construction during normal operation.

Release note: None
@pav-kv pav-kv force-pushed the replica_send_pprof branch from d6db017 to 9869df0 Compare August 17, 2022 11:02
@pav-kv
Copy link
Copy Markdown
Collaborator Author

pav-kv commented Aug 17, 2022

bors r=tbg

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Aug 17, 2022

Build succeeded:

@craig craig bot merged commit 39a86d3 into cockroachdb:master Aug 17, 2022
@pav-kv pav-kv deleted the replica_send_pprof branch August 17, 2022 14:50
@tbg
Copy link
Copy Markdown
Member

tbg commented Aug 17, 2022

blathers backport 22.1

@blathers-crl
Copy link
Copy Markdown

blathers-crl bot commented Aug 17, 2022

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

error creating merge commit from 9869df0 to blathers/backport-release-22.1-86130: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.1 failed. See errors above.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@tbg
Copy link
Copy Markdown
Member

tbg commented Aug 17, 2022

It was worth a try. @erikgrinaker think we should backport this?

If so @pavelkalinnikov you can use the instructions here to automate the boring parts: https://cockroachlabs.atlassian.net/wiki/spaces/CRDB/pages/900005932/Backporting+a+change+to+a+release+branch

Since there's a conflict, you'll need to resolve the conflict, then invoke backport again as backport --continue.

@pav-kv pav-kv mentioned this pull request Aug 18, 2022
Merged
@tbg tbg mentioned this pull request Aug 25, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tbg tbg tbg approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

kvserver: use pprof label for Replica.Send (replace sendWithRangeID)

3 participants

@pav-kv @cockroach-teamcity @tbg