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.

Sign up for GitHub

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

Jump to bottom

*: add jobID to ctx tags in bulk distsql processors #77397

Merged
merged 6 commits into from
Mar 6, 2022
Merged

*: add jobID to ctx tags in bulk distsql processors #77397

merged 6 commits into from
Mar 6, 2022

Conversation

dt
Copy link
Member

@dt dt commented Mar 5, 2022 •
edited
Loading

It is in the tags in the job's initial context which is what plans and runs these flows, but those tags get lost on the way to the remote processors, so only the one processor that happens to be on the gateway logs with its ID. Instead, these changes now put it explicitly in the processor specs so that we can explicitly add it to the local ctx tags in each processor's initialization.

Release note: none.

Release justification: logging only change.

@dt dt requested review from andreimatei, knz and a team March 5, 2022 13:04
@dt dt requested a review from a team as a code owner March 5, 2022 13:04
@dt dt requested review from shermanCRL and removed request for a team March 5, 2022 13:04
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@adityamaru
Copy link
Contributor

cc: @stevendanna since we briefly chatted about how we wished we had this when grepping for logs

@dt
Copy link
Member Author

dt commented Mar 5, 2022

TFTR!

CI failure looks like a flake; race-timeout in TestGCDropIndexSpanExpansion.

bors r+

@dt
Copy link
Member Author

dt commented Mar 5, 2022

bors r-

@craig
Copy link
Contributor

craig bot commented Mar 5, 2022

Canceled.

dt added 6 commits March 5, 2022 15:48
@dt
sql/importer: tag context with job ID
63c50d3 
Release note: none.

Release justification: logging change.
@dt
backupccl: add jobID to context tags
ef0b3aa 
Release note: none.

Release justification: logging change.
@dt
backupccl: add jobid to RESTORE ctx tags
d4abc72 
Release note: none.

Release justification: logging change.
@dt
changefeedccl: add jobID to processor ctx tags
582ad7b 
Release note: none.

Release justification: logging cahnge.
@dt
rowexec: add jobID to backfiller ctx tags
8bace74 
Release note: none.

Release justification: logging change.
@dt
streamingccl/streamingest: add job ID to context tags
4b334c0 
Release note: none.

Release justification: logging change.
@dt
Copy link
Member Author

dt commented Mar 5, 2022

bors r+

@craig
Copy link
Contributor

craig bot commented Mar 5, 2022

Build failed:

@dt
Copy link
Member Author

dt commented Mar 5, 2022

bors r+

@craig
Copy link
Contributor

craig bot commented Mar 5, 2022

Build failed:

@dt
Copy link
Member Author

dt commented Mar 5, 2022

Still TestGCDropIndexSpanExpansion.

bors r+

@craig
Copy link
Contributor

craig bot commented Mar 6, 2022

Build succeeded:

@craig craig bot merged commit 3b7f761 into cockroachdb:master Mar 6, 2022
@dt dt deleted the job-id-ctx branch March 6, 2022 01:42
@adityamaru
Copy link
Contributor

blathers backport 21.2

@blathers-crl
Copy link

blathers-crl bot commented Mar 20, 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 63c50d3 to blathers/backport-release-21.2-77397: 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 21.2 failed. See errors above.

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

@lidorcarmel lidorcarmel mentioned this pull request Nov 10, 2022
Merged
lidorcarmel added a commit to lidorcarmel/cockroach that referenced this pull request Nov 12, 2022
@lidorcarmel
distsql,jobs: propagate 'job' log tag and pprof label to remote nodes
a83a4fd 
Previously, the job log tag and the recently added job pprof label
were only used locally and not sent to remote nodes. Which means that,
for example, pprof did not show the job ids of jobs that consume cpu
but were started on another node.

This PR propagates the job tag to the remote nodes, and there it
adds that tag to be used when logging. It also adds the same tag as
a pprof label on the remote node. With this change we no longer need
to set the tags that were added in cockroachdb#77397, therefore those are
reverted here.

Note that this PR changes the job log tag from `job=<job id>` to
`job=<job type> id=<job id>`, to be the same as the pprof label.

Epic: None

Release note: None
lidorcarmel added a commit to lidorcarmel/cockroach that referenced this pull request Nov 13, 2022
@lidorcarmel
distsql,jobs: propagate 'job' log tag and pprof label to remote nodes
92bf199 
Previously, the job log tagand the recently added job pprof labels
were only used locally and not sent to remote nodes. Which means that,
for example, pprof did not show the job ids of jobs that consume cpu
but were started on another node.

This PR propagates the job tag to the remote nodes, and there it
adds that tag to be used when logging. It also adds the same tag as
a pprof label on the remote node. With this change we no longer need
to set the tags that were added in cockroachdb#77397, therefore those are
reverted here.

Note that this PR changes the job log tag from `job=<job id>` to
`job=<job type> id=<job id>`, to be the same as the pprof label.

Epic: None

Release note: None
lidorcarmel added a commit to lidorcarmel/cockroach that referenced this pull request Nov 21, 2022
@lidorcarmel
distsql,jobs: propagate 'job' log tag and pprof label to remote nodes
1a7ec75 
Previously, the recently added job log tag and pprof label
were only used locally and not sent to remote nodes. Which means that,
for example, pprof did not show the job ids of jobs that consume cpu
but were started on another node.

This PR propagates the job tag to the remote nodes, and there it
adds that tag to be used when logging. It also adds the same tag as
a pprof label on the remote node. With this change we no longer need
to set the tags that were added in cockroachdb#77397, but we cannot remove those
yet because we want the old job tags to still be sent and applied
during upgrades. Once 23.1 is out, we can remove the old implementation
from 23.2.

Note that this PR changes the job log tag from `job=<job id>` to
`job=<job type> id=<job id>`, to be the same as the pprof label.

Epic: None

Release note: None
craig bot pushed a commit that referenced this pull request Nov 22, 2022
@lidorcarmel @e-mbrown
Merge #91630 #92068
59de8e8 
91630: distsql,jobs: propagate 'job' log tag and pprof label to remote nodes r=lidorcarmel a=lidorcarmel

Previously, the job log tags and the recently added job pprof labels were only used locally and not sent to remote nodes. Which means that, for example, pprof did not show the job ids of jobs that consume cpu but were started on another node.

This PR propagates the job tag to the remote nodes, and there it adds that tag to be used when logging. It also adds the same tag as a pprof label on the remote node. With this change we no longer need to set the tags that were added in #77397, therefore those are reverted here.

Note that this PR changes the job log tag from `job=<job id>` to `job=<job type> id=<job id>`, to be the same as the pprof label.

See this gist for a before/after example https://gist.github.com/lidorcarmel/2c147b76d814e29682cccbdcadb5d7bb.

And here is pprof graph example with those tags:

<img width="2531" alt="pprof_with_job_tags" src="https://user-images.githubusercontent.com/51982110/201506953-acf2675d-28a2-471e-9ebf-5441b4728978.png">

There is another commit in this PR which is fixing a small bug in the recently added pprofutil package.

Epic: None

Release note: None

92068: sql/opt: Support regtype in foldOIDFamilyCast  r=rafiss a=e-mbrown

Informs: #91022

Type cast to regtype can now use the index on pgtype(oid).

Release note: None

Co-authored-by: Lidor Carmel <[email protected]>
Co-authored-by: e-mbrown <[email protected]>
lidorcarmel added a commit to lidorcarmel/cockroach that referenced this pull request Nov 28, 2022
@lidorcarmel
distsql,jobs: propagate 'job' log tag and pprof label to remote nodes
12f79e7 
Previously, the recently added job log tag and pprof label
were only used locally and not sent to remote nodes. Which means that,
for example, pprof did not show the job ids of jobs that consume cpu
but were started on another node.

This PR propagates the job tag to the remote nodes, and there it
adds that tag to be used when logging. It also adds the same tag as
a pprof label on the remote node. With this change we no longer need
to set the tags that were added in cockroachdb#77397, but we cannot remove those
yet because we want the old job tags to still be sent and applied
during upgrades. Once 23.1 is out, we can remove the old implementation
from 23.2.

Note that this PR changes the job log tag from `job=<job id>` to
`job=<job type> id=<job id>`, to be the same as the pprof label.

Epic: None

Release note: None
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@knz knz knz approved these changes

@andreimatei andreimatei Awaiting requested review from andreimatei

@shermanCRL shermanCRL Awaiting requested review from shermanCRL

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dt @cockroach-teamcity @adityamaru @knz