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

sql: improve and clean up tracing a bit #87317

Merged
merged 3 commits into from
Sep 8, 2022

Conversation

yuzefovich
Copy link
Member

@yuzefovich yuzefovich commented Sep 1, 2022

tracing: omit distsql ids from SHOW TRACE

This commit removes the custom handling of tracing tags with
cockroach. prefix when populating the output of SHOW TRACE.
Previously, all tags with this prefix would be included into the "start
span" message, possibly taking up multiple lines in the SHOW TRACE
output. However, there is only one user of those tags - ids of different
components of DistSQL infrastructure, and I don't think it's helpful to
have those ids in the output at all, so this commit removes this ability
and makes the "start span" message nicer.

This special handling was introduced four years ago in
60978aa and at that time there might
have been a reason to have some special handling of these tags (so that
they become visible when viewing the jaeger trace), but that is not
necessary anymore (I believe because we now always propagate all tags
across nodes).

Release justification: low-risk cleanup.

Release note: None

execinfra: clean up ProcessorBase a bit

This commit performs the following cleanup:

  • it removes the redundant InternalClose implementations. At some
    point last year an "extended" version was introduced to take in
    a closure to be called when the processor is being closed. There is only
    one user for that, and it can itself do the necessary cleanup before
    calling InternalClose
  • it removes the update to rowIdx of ProcOutputHelper (which tracks
    how many rows the helper has emitted) when the processor is closed. The
    idea behind this was to protect from the future calls to Next method
    so that the helper doesn't emit more rows once it is closed, but it is
    not allowed by the interface anyway - once the processor is closed, no
    new calls to Next are allowed, so this protection was meaningless.
    However, what prompted me to look into this was the fact that the
    rowIdx field was being set to MaxInt64 which would trip up the stats
    collection change in the following commit.

Release justification: low-risk cleanup.

Release note: None

sql: improve tracing of some things

This commit makes it so that we create a tracing span for all
processors. Previously, out of performance considerations, we elided the
spans for the columnarizer, materializer, planNodeToRowSource, and
flowCoordinator, but given the improvements to tracing in the last year
or so it doesn't seem necessary to do that anymore. In particular so
given that we don't create tracing spans by default any way, only when
the tracing is enabled for the statement.

Additionally, this commit adds a couple of tags to the tracing span of
the vectorized outbox (similar to what we have in the row-by-row
engine).

Release justification: low-risk improvement.

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@yuzefovich yuzefovich force-pushed the vec-tracing-spans branch 3 times, most recently from 7d80b38 to 7e93cfa Compare September 1, 2022 23:59
@yuzefovich yuzefovich changed the title sql: improve tracing sql: improve and clean up tracing a bit Sep 2, 2022
@yuzefovich yuzefovich force-pushed the vec-tracing-spans branch 8 times, most recently from 473c9c5 to 5414a57 Compare September 7, 2022 04:19
@yuzefovich yuzefovich marked this pull request as ready for review September 7, 2022 04:21
@yuzefovich yuzefovich requested review from a team September 7, 2022 04:21
@yuzefovich yuzefovich requested a review from a team as a code owner September 7, 2022 04:21
@yuzefovich
Copy link
Member Author

TFTR!

bors r+

@craig
Copy link
Contributor

craig bot commented Sep 7, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Sep 7, 2022

This PR was included in a batch that successfully built, but then failed to merge into master. It will not be retried.

Additional information:

{"message":"1 review requesting changes by reviewers with write access.","documentation_url":"https://docs.github.com/articles/about-protected-branches"}

@yuzefovich
Copy link
Member Author

This now needs a rebase.

bors r-

This commit removes the custom handling of tracing tags with
`cockroach.` prefix when populating the output of SHOW TRACE.
Previously, all tags with this prefix would be included into the "start
span" message, possibly taking up multiple lines in the SHOW TRACE
output. However, there is only one user of those tags - ids of different
components of DistSQL infrastructure, and I don't think it's helpful to
have those ids in the output at all, so this commit removes this ability
and makes the "start span" message nicer.

This special handling was introduced four years ago in
60978aa and at that time there might
have been a reason to have some special handling of these tags (so that
they become visible when viewing the jaeger trace), but that is not
necessary anymore (I believe because we now always propagate all tags
across nodes).

Release justification: low-risk cleanup.

Release note: None
This commit performs the following cleanup:
- it removes the redundant `InternalClose` implementations. At some
point last year an "extended" version was introduced to take in
a closure to be called when the processor is being closed. There is only
one user for that, and it can itself do the necessary cleanup before
calling `InternalClose`
- it removes the update to `rowIdx` of `ProcOutputHelper` (which tracks
how many rows the helper has emitted) when the processor is closed. The
idea behind this was to protect from the future calls to `Next` method
so that the helper doesn't emit more rows once it is closed, but it is
not allowed by the interface anyway - once the processor is closed, no
new calls to `Next` are allowed, so this protection was meaningless.
However, what prompted me to look into this was the fact that the
`rowIdx` field was being set to `MaxInt64` which would trip up the stats
collection change in the following commit.

Release justification: low-risk cleanup.

Release note: None
This commit makes it so that we create a tracing span for all
processors. Previously, out of performance considerations, we elided the
spans for the columnarizer, materializer, planNodeToRowSource, and
flowCoordinator, but given the improvements to tracing in the last year
or so it doesn't seem necessary to do that anymore. In particular so
given that we don't create tracing spans by default any way, only when
the tracing is enabled for the statement.

Additionally, this commit adds a couple of tags to the tracing span of
the vectorized outbox (similar to what we have in the row-by-row
engine).

Release justification: low-risk improvement.

Release note: None
@yuzefovich
Copy link
Member Author

bors r+

@craig
Copy link
Contributor

craig bot commented Sep 8, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Sep 8, 2022

This PR was included in a batch that successfully built, but then failed to merge into master. It will not be retried.

Additional information:

{"message":"At least 1 approving review is required by reviewers with write access.","documentation_url":"https://docs.github.com/articles/about-protected-branches"}

@yuzefovich
Copy link
Member Author

bors r+

@craig
Copy link
Contributor

craig bot commented Sep 8, 2022

Build succeeded:

@craig craig bot merged commit ce55e1b into cockroachdb:master Sep 8, 2022
@yuzefovich yuzefovich deleted the vec-tracing-spans branch September 8, 2022 17:33
@yuzefovich
Copy link
Member Author

blathers backport 22.2

@blathers-crl
Copy link

blathers-crl bot commented Dec 9, 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 694c41a to blathers/backport-release-22.2-87317: 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.2 failed. See errors above.


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

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.

3 participants