-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
[vtadmin-api] Add /vtexplain endpoint #7528
[vtadmin-api] Add /vtexplain endpoint #7528
Conversation
d362b20
to
a938277
Compare
span, ctx := trace.NewSpan(ctx, "API.VTExplain") | ||
defer span.Finish() | ||
|
||
if req.Cluster == "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This parameter validation feels a bit clumsy so I'm open to suggestions. Since, in the HTTP handler, these parameters are GET params (i..e, /api/vtexplain?cluster=&keyspace=&sql=
) there's no guarantee they'll be defined.
Also, the VTExplain
function feels... rather lengthy in general. I'm open to suggestions there too. (That said: even though it's long, it's pretty straightforward, so maybe it's fine...?)
I briefly considered breaking the goroutines out into separate functions, but frankly it made it harder to read + I don't expect the functions have any reusability, since VTExplain is the only instance requiring all this string marshalling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This parameter validation feels a bit clumsy so I'm open to suggestions. Since, in the HTTP handler, these parameters are GET params (i..e, /api/vtexplain?cluster=&keyspace=&sql=) there's no guarantee they'll be defined.
Also, the VTExplain function feels... rather lengthy in general. I'm open to suggestions there too. (That said: even though it's long, it's pretty straightforward, so maybe it's fine...?)
It would be really cool if there was a good way to split out a mux route into ${parameter validation} + ${logic} phases, where, where the params extracted/parsed/validated by phase 1 are available in the context (not necessarily the literal Go context, but also not necessarily not the literal Go context) of phase 2, but I don't see that, or something like it, from a quick skim of the mux docs. Maybe we should keep thinking about this!
I briefly considered breaking the goroutines out into separate functions, but frankly it made it harder to read + I don't expect the functions have any reusability, since VTExplain is the only instance requiring all this string marshalling.
Agreed that would potentially make things harder to understand, and better to have the longer, but cohesive single function.
VTExplain is the only instance requiring all this string marshalling.
we can also ....................... add a more typed interface to vtexplain :llama_hands:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be really cool if there was a good way to split out a mux route into ${parameter validation} + ${logic} phases
Yeah, something along these lines would be useful. We should, at some point, revisit the HTTP response envelopes (particularly for errors) that we're using internally. I think there are ways we can improve on what we have. Or.. we could try gRPC web again, lol.
we can also ....................... add a more typed interface to vtexplain :llama_hands:
I really want this. The string marshalling is easy to mess up + is a bit tedious to debug.
Cool, glad to see this! In case it helps, I wrote down some ideas in #7333 about how arguments could potentially be supported for an integrated vtexplain. |
@jmoldow wow, this is great! Smart defaults! ✨ I think this would be a really fantastic addition. Big thanks for sharing that. I'll start thinking about this from the front-end perspective. I can definitely loop you in on further VTExplain-in-VTAdmin work too, if you're interested! And if you have any ideas or thoughts on UI, I'd particularly love to hear 'em. 😎 |
Re error messages, #7514 might help once it is completed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! Everything I have is just extra suggestions and nitpicks, but lg2m to merge as-is
@@ -534,6 +540,47 @@ func (api *API) getTablets(ctx context.Context, c *cluster.Cluster) ([]*vtadminp | |||
return ParseTablets(rows, c) | |||
} | |||
|
|||
// findTablet returns the first tablet in a given cluster that satisfies the filter function. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe these functions make more sense on the cluster.Cluster
type? That would at least help break this file up a bit. Totally up to you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would definitely like to move them elsewhere. I think, though, that moving them to cluster.Cluster
might introduce a weird dependency.
Specifically, per your suggestion, I want to move the following three functions out of vtadmin/api.go and into vtadmin/cluster/cluster.go:
- findTablet
- findTablets
- getTablets, which is called by the two above
However, getTablets
calls ParseTablets
, which is in vtadmin/tablets.go (package vtadmin
), which would (I think) cause a circular dependency. I assume this is undesirable in golang, right?
I have two ideas, not sure which (if either) make sense to do:
- Create a separate
package tablet
vtadmin/tablet/tablet.go
(analogous tovtadmin/cluster/cluster.go
) - Move the three functions listed above into
vtadmin/tablets.go
, since the other functions intablets.go
also take a cluster argument. It seems like not a bad fit.
Happy to take any suggestion you have!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think anything that is like func (api *API) DoThingWithTablets(ctx context.Context, c *cluster.Cluster)
we should try (it might have some other dependencies on api
, which could get uhhhh interesting) to make it a func (c *Cluster) DoThingWithTablets(ctx context.Context)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ajm188 hmmm ok. In that case, I will have to move tablet.go
(and its ParseTablet
function) into its own package tablet
so cluster.go
can use it without adding a (circular) dependency on package vtadmin
, right? It seems reasonable to me, I just want to double check before I move things around.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's what I was saying with where it should be func (c *Cluster) ParseTablets(ctx context.Context, ...) ([]*vtadminpb.Tablet, error)
instead of func (api *API) ParseTablets(ctx context.Context, c *cluster.Cluster, ...) (...)
.
Also, I'm not super convinced that needs to remain a public function. It doesn't really feel like part of the API of a cluster, while "find N tablets in the cluster" definitely does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ajm188 that sounds really good.
Moving things around ends up being a rather big + messy commit, and it'd be nice to merge this one in since I'm branched off of it for front-end reasons.
Would it be ok with you if I moved everything into clusters.go
in a separate PR? I can do it today, right after this, and add a bunch of tests as penance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, let's file an issue for that, because I'll forget, but sg2m to merge
span, ctx := trace.NewSpan(ctx, "API.VTExplain") | ||
defer span.Finish() | ||
|
||
if req.Cluster == "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This parameter validation feels a bit clumsy so I'm open to suggestions. Since, in the HTTP handler, these parameters are GET params (i..e, /api/vtexplain?cluster=&keyspace=&sql=) there's no guarantee they'll be defined.
Also, the VTExplain function feels... rather lengthy in general. I'm open to suggestions there too. (That said: even though it's long, it's pretty straightforward, so maybe it's fine...?)
It would be really cool if there was a good way to split out a mux route into ${parameter validation} + ${logic} phases, where, where the params extracted/parsed/validated by phase 1 are available in the context (not necessarily the literal Go context, but also not necessarily not the literal Go context) of phase 2, but I don't see that, or something like it, from a quick skim of the mux docs. Maybe we should keep thinking about this!
I briefly considered breaking the goroutines out into separate functions, but frankly it made it harder to read + I don't expect the functions have any reusability, since VTExplain is the only instance requiring all this string marshalling.
Agreed that would potentially make things harder to understand, and better to have the longer, but cohesive single function.
VTExplain is the only instance requiring all this string marshalling.
we can also ....................... add a more typed interface to vtexplain :llama_hands:
Signed-off-by: Sara Bee <855595+doeg@users.noreply.github.com>
Signed-off-by: Sara Bee <855595+doeg@users.noreply.github.com>
Signed-off-by: Sara Bee <855595+doeg@users.noreply.github.com>
3dfc53a
to
3a2b95e
Compare
Signed-off-by: Sara Bee <855595+doeg@users.noreply.github.com>
This is so awesome as a feature, I can't even! |
Signed-off-by: Sara Bee 855595+doeg@users.noreply.github.com
Description
This adds the simplest possible
/api/vtexplain
endpoint to vtadmin-api.I'm still new to golang, so I invite you to be nit-picky. 😎 It's useful.
Most features I've left for later, like returning JSON instead of text, supporting more (ok... any) of the vtexplain options, and so on. I think it's worth building out more of the front-end first.
Later on, I'd also like to refine some of the errors so they're parseable enough for the front-end to show (ideally) useful feedback. For example, if a serving replica in the keyspace can't be found,
"no such tablet"
is an accurate error message but not a particularly useful or actionable one for an end-user. I realize I could put more information in the error string, but ultimately I'd like something more robust, like an error envelope with acode
(similar to what we did for our internal HTTP handlers). Anyway... Much to consider.Staged at http://vtadmin.puppy.software:14200/api/vtexplain
Example: http://vtadmin.puppy.software:14200/api/vtexplain?cluster=id1&keyspace=commerce&sql=select%20*%20from%20product
curl "http://vtadmin.puppy.software:14200/api/vtexplain?cluster=id1&keyspace=commerce&sql=select%20*%20from%20product" | jq .
Related Issue(s)
N/A
Checklist
Deployment Notes
N/A
Impacted Areas in Vitess
Components that this PR will affect: