VTAdmin to use VTGate's vexplain #17508

c-r-dev · 2025-01-13T01:54:42Z

Description

VTAdmin's VTExplain feature currently uses VTExplain directly.

With this PR introduced

A new end point in VTADMIN-API /api/vexplain , which leverages VTGate sql connection to run vexplain [ALL|PLAN|QUERIES|TRACE|KEYS] <QUERY> ( https://vitess.io/docs/22.0/user-guides/sql/vexplain/ )
A new page in VTADMIN-WEB /vexplain which connect to VTADMIN-API /api/vexplain.

Please note - currently PR has /vexplain and /api/vexplain , will merge/patch these over /vtexplain and /api/vtexplain based on feedback.

While testing the changes on local , i came across below scenario , need inputs from team on #17508 (comment)

thanks to @beingnoble03 , i have tried to move things around and added a drop down for vexplain options with 4099f0e

Related Issue(s)

Fixes #16412

Checklist

"Backport to:" labels have been added if this change should be back-ported to release branches
If this change is to be back-ported to previous releases, a justification is included in the PR description
Tests were added or are not required
Did the new or modified tests pass consistently locally and on CI?
Documentation was added or is not required

Deployment Notes

Signed-off-by: c-r-dev <[email protected]>

vitess-bot · 2025-01-13T01:54:45Z

codecov · 2025-01-13T02:15:16Z

Codecov Report

Attention: Patch coverage is 56.58915% with 56 lines in your changes missing coverage. Please review.

Project coverage is 67.62%. Comparing base (a6c2896) to head (918d02e).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
go/vt/vtadmin/vtsql/vtsql.go	34.54%	36 Missing ⚠️
go/vt/vtadmin/api.go	70.73%	12 Missing ⚠️
go/vt/vtadmin/http/vexplain.go	0.00%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #17508      +/-   ##
==========================================
- Coverage   67.64%   67.62%   -0.02%     
==========================================
  Files        1586     1587       +1     
  Lines      255629   255758     +129     
==========================================
+ Hits       172910   172958      +48     
- Misses      82719    82800      +81

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: c-r-dev <[email protected]>

beingnoble03

Please have a look at the comments, rest looks good to me. Would lean on other reviewers for further review. Thank you!

web/vtadmin/src/components/routes/VExplain.tsx

go/vt/vtadmin/api.go

beingnoble03 · 2025-01-14T12:32:57Z

web/vtadmin/src/api/http.ts

+    // As an easy enhancement for later, we can also validate the request parameters on the front-end
+    // instead of defaulting to '', to save a round trip.
+    const req = new URLSearchParams();
+    req.append('cluster_id', cluster_id || '');
+    req.append('keyspace', keyspace || '');
+    req.append('sql', sql || '');


Can we check these parameters in the component itself? Maybe disabling the 'Run VExplain' button if keyspace is not selected or SQL is empty?

thanks , i have added client side validation to check and alert if sql and keyspace are empty. Please let me know.

Client side validation code

nit: I don't think we need alert here, is it possible to simply disable the button until all the parameters are non-empty?

go/vt/vtadmin/cluster/cluster.go

beingnoble03 · 2025-01-14T13:23:36Z

go/vt/vtadmin/cluster/cluster.go

@@ -2505,6 +2508,82 @@ func (c *Cluster) ToggleTabletReplication(ctx context.Context, tablet *vtadminpb
 	return err
 }

+// GetVExplain returns the VExplain json message.
+func (c *Cluster) GetVExplain(ctx context.Context, req *vtadminpb.VExplainRequest, vexplainStmt *sqlparser.VExplainStmt) (*vtadminpb.VExplainResponse, error) {


Not sure if we need this to be here as it doesn't really need any property from c *Cluster, maybe we can directly parse the result in c.DB.VExplain(), then we won't need it to be here. So, this would include moving convertVExplainResultToString and convertVExplainQueriesResultToString to vtsql.go.

thanks , incorporated with a79c71f

Can we remove this method from here, and call c.DB.VExplain directly in api.go?

c-r-dev · 2025-01-14T14:32:31Z

Thanks @beingnoble03 for helping to review and share feedback.

Will go over and incorporate before making PR ready for review.

Signed-off-by: c-r-dev <[email protected]>

c-r-dev · 2025-01-20T12:03:35Z

@systay , @beingnoble03 , @harshit-gangal , @deepthi , team,

While testing changes locally , came across below scenario :

from UI , we are letting choose keyspace (similar to VTExplain)
on the /api/vexplain , current code is not validating if table(s) mentioned in sql are present in keyspace passed from UI.
VTADMIN API Vexplain Code
in case user chooses keyspace1 in UI , but fires sql on a table present in keyspace2
/api/vexplain gets back with result ( as long as keyspace1 and keyspace2 belong to same vitess cluster)

To resolve this , couple of diff ways I can think off :

From UI , on VExplain screen - shall we ask use to just provide cluster name (and not keyspace)
Or in the api /api/vexplain , validate if the table(s) mentioned in sql belong to keyspace which is passed from UI.

Signed-off-by: c-r-dev <[email protected]>

beingnoble03

Rest LGTM. Tested this locally as well, works as expected. Thank you!

beingnoble03 · 2025-01-26T07:26:25Z

go/vt/vtadmin/api_test.go

+
+					// We don't particularly care to test the contents of the VExplain response,
+					// just that it exists.
+					print(resp.Response)


nit: I think this print statement can be removed?

beingnoble03 · 2025-01-27T06:05:33Z

web/vtadmin/src/api/http.ts

+    // As an easy enhancement for later, we can also validate the request parameters on the front-end
+    // instead of defaulting to '', to save a round trip.
+    const req = new URLSearchParams();
+    req.append('cluster_id', cluster_id || '');
+    req.append('keyspace', keyspace || '');
+    req.append('sql', sql || '');


nit: I don't think we need alert here, is it possible to simply disable the button until all the parameters are non-empty?

beingnoble03 · 2025-01-27T06:51:52Z

go/vt/vtadmin/vtsql/vtsql.go

+}
+func convertVExplainResultToString(rows *sql.Rows) (*vtadminpb.VExplainResponse, error) {


nit: I think we can leave a line between these 2 functions.

beingnoble03 · 2025-01-27T07:05:37Z

go/vt/vtadmin/cluster/cluster.go

@@ -2505,6 +2508,82 @@ func (c *Cluster) ToggleTabletReplication(ctx context.Context, tablet *vtadminpb
 	return err
 }

+// GetVExplain returns the VExplain json message.
+func (c *Cluster) GetVExplain(ctx context.Context, req *vtadminpb.VExplainRequest, vexplainStmt *sqlparser.VExplainStmt) (*vtadminpb.VExplainResponse, error) {


Can we remove this method from here, and call c.DB.VExplain directly in api.go?

VTAdmin to use VTGate's vexplain

c87a5d5

Signed-off-by: c-r-dev <[email protected]>

github-actions bot added this to the v22.0.0 milestone Jan 13, 2025

checking in vtadmin.proto (and auto generated files)

a2cb477

Signed-off-by: c-r-dev <[email protected]>

deepthi requested a review from beingnoble03 January 13, 2025 15:39

beingnoble03 reviewed Jan 14, 2025

View reviewed changes

c-r-dev added 3 commits January 19, 2025 17:57

vexplain ui side validation and screen changes

4099f0e

Signed-off-by: c-r-dev <[email protected]>

incorporated first set of feedback on backend /api/vexplain

a79c71f

Signed-off-by: c-r-dev <[email protected]>

check-in proto files

dc046c6

Signed-off-by: c-r-dev <[email protected]>

c-r-dev added 3 commits January 22, 2025 05:48

Merge branch 'vitessio:main' into vtadmin-vexplain

b4d5425

added api_test for vexplain end point

f8a3d76

Signed-off-by: c-r-dev <[email protected]>

Merge branch 'vitessio:main' into vtadmin-vexplain

918d02e

c-r-dev requested a review from beingnoble03 January 24, 2025 17:40

c-r-dev marked this pull request as ready for review January 24, 2025 17:41

c-r-dev requested review from deepthi, harshit-gangal, mattlord, ajm188, notfelineit and rohit-nayak-ps as code owners January 24, 2025 17:41

rohit-nayak-ps added Type: Enhancement Logical improvement (somewhere between a bug and feature) Component: VTAdmin VTadmin interface and removed NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work labels Jan 24, 2025

rohit-nayak-ps removed NeedsWebsiteDocsUpdate What it says NeedsBackportReason If backport labels have been applied to a PR, a justification is required labels Jan 24, 2025

beingnoble03 reviewed Jan 27, 2025

View reviewed changes

beingnoble03 removed the NeedsIssue A linked issue is missing for this Pull Request label Jan 27, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VTAdmin to use VTGate's vexplain #17508

VTAdmin to use VTGate's vexplain #17508

c-r-dev commented Jan 13, 2025 •

edited

Loading

vitess-bot bot commented Jan 13, 2025

codecov bot commented Jan 13, 2025 •

edited

Loading

beingnoble03 left a comment

beingnoble03 Jan 14, 2025

c-r-dev Jan 19, 2025

beingnoble03 Jan 27, 2025

beingnoble03 Jan 14, 2025

c-r-dev Jan 19, 2025

beingnoble03 Jan 27, 2025

c-r-dev commented Jan 14, 2025

c-r-dev commented Jan 20, 2025

beingnoble03 left a comment

beingnoble03 Jan 26, 2025

beingnoble03 Jan 27, 2025

beingnoble03 Jan 27, 2025

beingnoble03 Jan 27, 2025

		}
		func convertVExplainResultToString(rows sql.Rows) (vtadminpb.VExplainResponse, error) {

VTAdmin to use VTGate's vexplain #17508

Are you sure you want to change the base?

VTAdmin to use VTGate's vexplain #17508

Conversation

c-r-dev commented Jan 13, 2025 • edited Loading

Description

Related Issue(s)

Checklist

Deployment Notes

vitess-bot bot commented Jan 13, 2025

Review Checklist

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

codecov bot commented Jan 13, 2025 • edited Loading

Codecov Report

beingnoble03 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c-r-dev commented Jan 14, 2025

c-r-dev commented Jan 20, 2025

beingnoble03 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c-r-dev commented Jan 13, 2025 •

edited

Loading

codecov bot commented Jan 13, 2025 •

edited

Loading