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

StatementStatistics.Nodes appears to be incomplete #96647

Closed
matthewtodd opened this issue Feb 6, 2023 · 1 comment · Fixed by #106587
Closed

StatementStatistics.Nodes appears to be incomplete #96647

matthewtodd opened this issue Feb 6, 2023 · 1 comment · Fixed by #106587
Assignees
Labels
A-sql-observability Related to observability of the SQL layer C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.

Comments

@matthewtodd
Copy link
Contributor

matthewtodd commented Feb 6, 2023

In my investigations for #89949, I believe I've discovered we are under-reporting the nodes involved in a distsql query in crdb_internal.statement_statistics and friends.

To reproduce, create a multi-region database with a populated regional by row table, and select * from it. The resulting statistics->>'statistics'->>'nodes' value in crdb_internal.statement_statistics should show at least one node from each region, but it will instead only show the gateway node. (Note that EXPLAIN (DISTSQL) however does show all the involved nodes.)

I believe the suspect code is in getNodesFromPlanner in executor_statement_metrics.go.

Observed on a 22.2 release as well as on (current) pre-23.1 master.

Jira issue: CRDB-24263

@matthewtodd matthewtodd added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-sql-observability Related to observability of the SQL layer T-sql-observability labels Feb 6, 2023
@xinhaoz
Copy link
Member

xinhaoz commented Feb 15, 2023

It looks like that getNodesFromPlanner only works when tracing is on while we always provide at least the gateway node as part of the list. Maybe we need to figure out how to better communicate that there was no observability here (and it could have run on more nodes) when we don't have the trace? @kevin-v-ngo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sql-observability Related to observability of the SQL layer C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants