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

util/tracing: fix String() for recordings of child spans #50914

Merged
merged 3 commits into from
Jul 8, 2020
Merged

util/tracing: fix String() for recordings of child spans #50914

merged 3 commits into from
Jul 8, 2020

Commits on Jul 8, 2020

  1. sql: simplify a test 

    This index drop test was a bit out there. It was tracing a transaction
through SQL, and then hackily getting its hands on the respective trace
from KV. In doing that, it was relying on GetRecording(sp) to return the
recoding of one of sp's ancestors. That behavior is about to change.

This test doesn't need tracing to do what it wants, and it's cleaner
without it.

Release note: None
    @andreimatei
    andreimatei committed Jul 8, 2020
    Configuration menu
    Copy the full SHA
    a7a8a87 View commit details
    Browse the repository at this point in the history

  2. util/tracing: rework trace recording 

    Before this patch, the behavior of a recording inside a recording was no
good because of shortcuts taken in the implementation of the recording
mechanism. Before, there was really no concept of a recording inside a
recording; a child's recording was the same as the parent's. So, if a
child had been created in a parent that was already recording, and then
you tried to get the recording from the child, you'd get the parent's
recording. To make it more confusing, if you explicitly started a
child's recording, then the child would get a separate recording indeed,
but the parent's recording would not contain the child's (for extra
extra confusion, the parent's recording would actually the log messages
from the child spans, but not any info on grandchildren).

This was a problem for SQL tracing, which has different features that
enable recording at different levels - full txn recording vs single-stmt
recording. When both were enabled, the interaction buggy.

This patch reworks the code around recording traces. We do away with the
spanGroup; instead recording spans now keep track of their children
internally.  We get the flexibility of reading a child's recording from
inside a parent's recording. The structure keeping track of recorded
info (spanGroup) is now hierarchical, with each span getting its own
group.

As a consequence, this patch fixes the recent Executor testing knob
WithStatementTrace, which was failing to include sub-queries ran using
the InternalExecutor in the recordings it was collecting. As such, it
was under-counting KV requests in the ddl_analysis test. Those counts
have been corrected (sadly, upwards).

Release note: None
    @andreimatei
    andreimatei committed Jul 8, 2020
    Configuration menu
    Copy the full SHA
    36ebed5 View commit details
    Browse the repository at this point in the history

  3. util/tracing: fix String() for recordings of child spans 

    The String() method wasn't working for recordings of non-root spans,
because it was looking for a root span (parent id = 0). A recording for
a span that had a parent doesn't have a root span. Things mostly worked
in practice, though, as children of no-op spans had a parent id = 0, so
they looked like roots.

This patch switches to assuming that the first span in the recording is
the "root of that recording".

Release note (bug fix): Fixed a bug causing the raw trace file collected
inside a statement diagnostics bundle to be sometimes empty when the
cluster setting sql.trace.txn.enable_threshold was in use.
    @andreimatei
    andreimatei committed Jul 8, 2020
    Configuration menu
    Copy the full SHA
    8969acb View commit details
    Browse the repository at this point in the history