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: refactor code shared by diagnostics and EXPLAIN ANALYZE (DEBUG) #56262

Merged
merged 2 commits into from
Nov 5, 2020

Conversation

RaduBerinde
Copy link
Member

sql: pass recording to WithStatementTrace

Pass the recording instead of the span.

Release note: None

sql: refactor code shared by diagnostics and EXPLAIN ANALYZE (DEBUG)

There are two paths that result in a bundle being collected and inserted
into the diagnostics tables. Each path uses its own code to insert the
diagnostics bundle, which leads to unnecessary complication.

This change simplifies the stmtdiagnostics code to let the caller do
the insertion and streamlines the higher level code.

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

Pass the recording instead of the span.

Release note: None
Copy link
Contributor

@andreimatei andreimatei left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner and @andreimatei)

There are two paths that result in a bundle being collected and inserted
into the diagnostics tables. Each path uses its own code to insert the
diagnostics bundle, which leads to unnecessary complication.

This change simplifies the stmtdiagnostics code to let the caller do
the insertion and streamlines the higher level code.

Release note: None
@RaduBerinde
Copy link
Member Author

bors r+

@craig
Copy link
Contributor

craig bot commented Nov 5, 2020

Build failed:

@RaduBerinde
Copy link
Member Author

Looks like a couple of flaky tests.

bors r+

craig bot pushed a commit that referenced this pull request Nov 5, 2020
56262: sql: refactor code shared by diagnostics and EXPLAIN ANALYZE (DEBUG) r=RaduBerinde a=RaduBerinde

#### sql: pass recording to WithStatementTrace

Pass the recording instead of the span.

Release note: None

#### sql: refactor code shared by diagnostics and EXPLAIN ANALYZE (DEBUG)

There are two paths that result in a bundle being collected and inserted
into the diagnostics tables. Each path uses its own code to insert the
diagnostics bundle, which leads to unnecessary complication.

This change simplifies the stmtdiagnostics code to let the caller do
the insertion and streamlines the higher level code.

Release note: None

Co-authored-by: Radu Berinde <[email protected]>
@craig
Copy link
Contributor

craig bot commented Nov 5, 2020

Build failed:

@RaduBerinde
Copy link
Member Author

Hit #56283

bors r+

@craig
Copy link
Contributor

craig bot commented Nov 5, 2020

Build succeeded:

@craig craig bot merged commit 3e9ad0f into cockroachdb:master Nov 5, 2020
@RaduBerinde RaduBerinde deleted the minor-refactors branch November 5, 2020 19:20
craig bot pushed a commit that referenced this pull request Nov 6, 2020
56286: sql: refactor instrumentation of query execution r=RaduBerinde a=RaduBerinde

Note: this PR covers only the two top commits; the others are #56262.

#### sql: refactor instrumentation of query execution

This change separates the code around setting up tracing and
collecting bundles into a new instrumentationHelper. The intention is
that more related logic (like collecting plans for the UI) will be
incorporated.

Release note: None

#### sql: move more logic into instrumentationHelper

This commit moves the logic around collecting explain plans (for
bundles and for UI) into `instrumentationHelper`.

Release note: None

Co-authored-by: Radu Berinde <[email protected]>
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