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 request unit to tracing sampled query event #107703

Closed
wants to merge 1 commit into from
Closed

sql: add request unit to tracing sampled query event #107703

wants to merge 1 commit into from

Conversation

j82w
Copy link
Contributor

@j82w j82w commented Jul 27, 2023

Fixes: #95194

Release note (sql change): Add the request unit estimate to the sampled query event.

@j82w j82w requested a review from a team July 27, 2023 13:18
@j82w j82w requested a review from a team as a code owner July 27, 2023 13:18
@j82w j82w requested review from nkodali and DrewKimball and removed request for a team July 27, 2023 13:18
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Fixes: #95194

Release note (sql change): Add the request unit estimate to the sampled
query event.
@DrewKimball
Copy link
Collaborator

I don't think we can do this yet, since the network contribution to RU consumption is only measured for EXPLAIN ANALYZE right now, not all sampled queries. Also worth mentioning that the estimate is currently only shown for tenants, and only for vectorized execution.

@kevin-v-ngo
Copy link

kevin-v-ngo commented Jul 27, 2023

Hi @DrewKimball, do you know how we get network information (bytes) today for SQL stats? Is this a different network metric used for RUs today?

Just want to understand relationship so we have consistent/correlated metrics

@DrewKimball
Copy link
Collaborator

Hi @DrewKimball, do you know how we get network information (bytes) today for SQL stats? Is this a different network metric used for RUs today?

Do you mean NetworkBytesSent? I believe that measures the number of bytes sent internally during execution of a SQL query from node to node, not from node to client. It also doesn't include remote requests made through KV (e.g. for a scan doing some remote reads). RUs currently don't care about internal network usage, only the bytes sent to the client.

During normal execution we track the bytes sent to the client during normal execution as the results are encoded. During a command like EXPLAIN ANALYZE, the results aren't actually sent to the client, so instead we pass them through a helper that encodes the result, measures the size, and then discards the encoded bytes (see here). Since estimated RUs have so far only been set up to work with EXPLAIN ANALYZE, they aren't hooked up to the network egress measured during normal execution.

@kevin-v-ngo
Copy link

Hi @DrewKimball, do you know how we get network information (bytes) today for SQL stats? Is this a different network metric used for RUs today?

Do you mean NetworkBytesSent? I believe that measures the number of bytes sent internally during execution of a SQL query from node to node, not from node to client. It also doesn't include remote requests made through KV (e.g. for a scan doing some remote reads). RUs currently don't care about internal network usage, only the bytes sent to the client.

During normal execution we track the bytes sent to the client during normal execution as the results are encoded. During a command like EXPLAIN ANALYZE, the results aren't actually sent to the client, so instead we pass them through a helper that encodes the result, measures the size, and then discards the encoded bytes (see here). Since estimated RUs have so far only been set up to work with EXPLAIN ANALYZE, they aren't hooked up to the network egress measured during normal execution.

Thank you @DrewKimball for the explanation. I've filed a related issue here tracking better observability into RUs: #108436.

@dhartunian dhartunian removed the request for review from nkodali August 30, 2023 14:04
@j82w
Copy link
Contributor Author

j82w commented Oct 2, 2023

Closing this based on the feedback on ru accuracy.

@j82w j82w closed this Oct 2, 2023
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.

Collect RU consumption per statement as telemetry data
4 participants