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

ccl/sqlproxyccl: add connection migration-related metrics #77700

Merged

Conversation

jaylim-crl
Copy link
Collaborator

@jaylim-crl jaylim-crl commented Mar 11, 2022

ccl/sqlproxyccl: add metric that measures connection migration latency

Previously, we added support for connection migration in #76805. This commit
adds a new proxy.conn_migration.attempted.latency metric that tracks latency
for attempted connection migrations. Having this metric would be beneficial
as we can now know how long users were blocked during connection migrations.

Release justification: Low-risk metric-only change within sqlproxy.

Release note: None

ccl/sqlproxyccl: add metric that records the transfer response message size

Informs #76000, and follow up to #76805.

This commit adds a new proxy.conn_migration.transfer_response.message_size
metric that will track the distribution of the transfer response message size.
This will be used to tune a value for the SQL cluster setting:
sql.session_transfer.max_session_size.

Release justification: Low-risk metric-only change within sqlproxy.

Release note: None

@jaylim-crl jaylim-crl requested review from a team as code owners March 11, 2022 20:00
@jaylim-crl jaylim-crl requested review from jeffswenson and removed request for a team March 11, 2022 20:00
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@jaylim-crl jaylim-crl force-pushed the jay/sqlproxy-add-transfer-latency branch from 2f232a8 to c1ddc64 Compare March 13, 2022 17:06
@jaylim-crl jaylim-crl changed the title ccl/sqlproxyccl: add metric that measures connection migration latency ccl/sqlproxyccl: add connection migration-related metrics Mar 13, 2022
@jaylim-crl jaylim-crl force-pushed the jay/sqlproxy-add-transfer-latency branch from c1ddc64 to ba4c58e Compare March 13, 2022 19:25
Copy link
Collaborator

@jeffswenson jeffswenson left a comment

Choose a reason for hiding this comment

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

LGTM

pkg/ccl/sqlproxyccl/conn_migration.go Outdated Show resolved Hide resolved
@jaylim-crl jaylim-crl force-pushed the jay/sqlproxy-add-transfer-latency branch from ba4c58e to 0e7266b Compare March 14, 2022 14:11
Copy link
Collaborator Author

@jaylim-crl jaylim-crl left a comment

Choose a reason for hiding this comment

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

TFTR!

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @jeffswenson)


pkg/ccl/sqlproxyccl/conn_migration.go, line 140 at r2 (raw file):

Previously, JeffSwenson (Jeff Swenson) wrote…

It would be nice to add the transfer latency to the log statements in the defer. That way we can dig into the source of unexpectedly high/low latency.

Done.

Previously, we added support for connection migration in cockroachdb#76805. This commit
adds a new `proxy.conn_migration.attempted.latency` metric that tracks latency
for attempted connection migrations. Having this metric would be beneficial
as we can now know how long users were blocked during connection migrations.

Release justification: Low-risk metric-only change within sqlproxy.

Release note: None
…e size

Informs cockroachdb#76000, and follow up to cockroachdb#76805.

This commit adds a new proxy.conn_migration.transfer_response.message_size
metric that will track the distribution of the transfer response message size.
This will be used to tune a value for the SQL cluster setting:
sql.session_transfer.max_session_size.

Release justification: Low-risk metric-only change within sqlproxy.

Release note: None
@jaylim-crl jaylim-crl force-pushed the jay/sqlproxy-add-transfer-latency branch from 0e7266b to 454b896 Compare March 14, 2022 14:13
@jaylim-crl
Copy link
Collaborator Author

bors r=JeffSwenson

@craig
Copy link
Contributor

craig bot commented Mar 14, 2022

Build succeeded:

@craig craig bot merged commit 51cbdce into cockroachdb:master Mar 14, 2022
@jaylim-crl jaylim-crl deleted the jay/sqlproxy-add-transfer-latency branch March 14, 2022 17:43
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