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: add trace_id to show sessions #118002

Merged
merged 1 commit into from Jan 23, 2024
Merged

Conversation

maryliag
Copy link
Contributor

@maryliag maryliag commented Jan 19, 2024

Add the column trace_id to the result of SHOW SESSIONS.
Also adding the column to the views:
crdb_internal.node_sessions and crdb_internal.cluster_sessions.

Part Of #117625

Release note (sql change): Add column trace_id to the response of the SHOW SESSIONS command.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@maryliag maryliag force-pushed the session-info branch 3 times, most recently from 65a3167 to 303b2e6 Compare January 22, 2024 16:29
@maryliag maryliag changed the title sql: add trace_id and goroutine_id to show sessions sql: add trace_id to show sessions Jan 22, 2024
@maryliag maryliag force-pushed the session-info branch 6 times, most recently from cfbf52f to 8bbd85e Compare January 22, 2024 20:21
@maryliag maryliag requested review from a team and xinhaoz and removed request for a team January 22, 2024 21:10
@maryliag maryliag marked this pull request as ready for review January 22, 2024 21:10
@maryliag maryliag requested review from a team as code owners January 22, 2024 21:10
@maryliag maryliag removed request for a team January 22, 2024 21:10
Copy link
Collaborator

@fqazi fqazi left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 7 of 10 files at r1, 5 of 5 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @xinhaoz)

Copy link
Collaborator

@dhartunian dhartunian left a comment

Choose a reason for hiding this comment

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

:lgtm: just a nit and a question, not blocking.

Reviewed 7 of 10 files at r1, 5 of 5 files at r2, all commit messages.
Reviewable status: :shipit: complete! 2 of 0 LGTMs obtained (waiting on @maryliag and @xinhaoz)


pkg/server/serverpb/status.proto line 1057 at r1 (raw file):

  uint32 pg_backend_pid = 19 [ (gogoproto.customname) = "PGBackendPID" ];

  // The ID the session's trace.

nit: ID of the session's trace


pkg/sql/conn_executor.go line 4163 at r2 (raw file):

		TotalActiveTime:            sessionActiveTime,
		PGBackendPID:               ex.planner.extendedEvalCtx.QueryCancelKey.GetPGBackendPID(),
		TraceID:                    uint64(ex.planner.extendedEvalCtx.Tracing.connSpan.TraceID()),

weird that we have to plumb so far to get the trace. there's no context that we could extract the trace from? that's how most of the other code works.

or does the caller already have a handle on a span that could be passed in?

Add the column trace_id to the result of `SHOW SESSIONS`.
Also adding the column to the views:
`crdb_internal.node_sessions` and `crdb_internal.cluster_sessions`.

Part Of cockroachdb#117625

Release note (sql change): Add column `trace_id`
to the response of the `SHOW SESSIONS` command.
Copy link
Contributor Author

@maryliag maryliag left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @dhartunian, @fqazi, and @xinhaoz)


pkg/server/serverpb/status.proto line 1057 at r1 (raw file):

Previously, dhartunian (David Hartunian) wrote…

nit: ID of the session's trace

Done


pkg/sql/conn_executor.go line 4163 at r2 (raw file):

Previously, dhartunian (David Hartunian) wrote…

weird that we have to plumb so far to get the trace. there's no context that we could extract the trace from? that's how most of the other code works.

or does the caller already have a handle on a span that could be passed in?

In this case we need to go this far. You can notice something similar happens on the line right above for the pg backend PID. The function I'm calling already handles the case when the tracing is on/off, is the span was closed, etc. It might be complicated to use something else and having to handle all of that again.

Copy link
Collaborator

@dhartunian dhartunian left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @maryliag and @xinhaoz)


pkg/ccl/logictestccl/testdata/logic_test/crdb_internal_tenant line 266 at r3 (raw file):

id  node_id  session_id  start  txn_string  application_name  num_stmts  num_retries  num_auto_retries  last_auto_retry_reason  isolation_level  priority  quality_of_service

query ITTTTTTTTTTTTTTTI colnames

Why does this new column have an I here? The pg_backend_pid is also an int, right? but here it's a T which I think implies Text.

@maryliag
Copy link
Contributor Author

bors r+

Copy link
Contributor Author

@maryliag maryliag left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @dhartunian and @xinhaoz)


pkg/ccl/logictestccl/testdata/logic_test/crdb_internal_tenant line 266 at r3 (raw file):

Previously, dhartunian (David Hartunian) wrote…

Why does this new column have an I here? The pg_backend_pid is also an int, right? but here it's a T which I think implies Text.

I added an I since is an INT, the same way there is an I for the node_id, maybe the T on others were an oversight or the format itself didn't matter.
I have a following PR where I'm adding one more column, so I can use that and align all columns with the correct types.

@craig
Copy link
Contributor

craig bot commented Jan 23, 2024

Build succeeded:

@craig craig bot merged commit 5db0b83 into cockroachdb:master Jan 23, 2024
7 of 9 checks passed
@maryliag maryliag deleted the session-info branch January 23, 2024 17:47
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.

None yet

4 participants