-
Notifications
You must be signed in to change notification settings - Fork 40
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
Move oxql
from oxdb
to mainline omdb
#5988
Conversation
6fad0a7
to
9addfda
Compare
Last update: fix ordering as output will switch order. |
383128c
to
03ab429
Compare
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.
Thanks for putting this together! It's great to see this in the more widely-used omdb
tool, and will really help use dogfood OxQL ourselves.
I have a few small comments and questions, but I think the main issue is about where the shell implementation lives. I would prefer to keep it in oximeter-db
, given the alpha nature of the language itself, and how tightly-coupled it is to the database. That would help limit the sprawl of the code while we continue to improve both the shell and the language.
Thanks!
03ab429
to
e6e052d
Compare
I just have a high-level comment here. While having OxQL capabilities in |
@rcgoodfellow Running OxQL queries is currently supported via the CLI, under the |
Yup. I'm aware of (and use!) that API endpoint. I'm mostly concerned about us developing tooling around OxQL that would be useful to operators but never makes it out of |
Oh, that context is helpful @rcgoodfellow. I agree we should limit the degree to which we build specific functionality in |
To add to what @bnaecker said, @rcgoodfellow, this helps with introspecting data in a4x2 (e.g. bgp stats), for example, since omdb is installable there, which does not come through running omicron on local machines. To your point @rcgoodfellow, the main code I'm working on is exposing the data via API, and even something open-metrics formatted (accessed via an API call) over pre-selected queries. |
Would it address this problem to have the switch zone ship |
@davepacheco I think that would work, though |
@davepacheco, in talking to @bnaecker, it seemed like we should graduate |
e6e052d
to
6e4f9c5
Compare
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.
Looks good! I've got a few nits and small fixes, but I think this is in a good place. Thanks!
Why?: Concerning [network observability work](https://github.com/orgs/oxidecomputer/projects/55/views/1?filterQuery=&pane=issue&itemId=68336554), this makes the [`oxql`](https://rfd.shared.oxide.computer/rfd/0463) interactive query repl accessible via omdb, as we start to give users and ourselves the ability to query timeseries and metrics more easily. Additionally, in the "now", this aids in debugging through our metrics set and makes it available, via omdb, throughout our ecosystem/a4x2. Includes: * Moves `oxql` and `sql` shells (from oxdb) into the oximeter_db src for use by both omdb (mainline tool) and oxdb. * simplifies the oxdb bin to leverage the new `shells` module * If no URL is given to `omdb oxql`, it will leverage internal DNS. * Update the oximeter omdb call (for listing producers) to leverage internal DNS if no URL is given. * Update command/output tests/generations and collector specific tests for list producers. Notes: * The oxql client still expects an socket address as liked it typed specifically v.s. a String. Instead, upon running the `omdb oxql` command, we take in a URL String and parse it into the socket address directly. Co-authored-by: Benjamin Naecker <[email protected]>
531fcb1
to
d31a847
Compare
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.
Looks solid! Thanks for taking this on, and all the back-and-forth. I've one style nit, but otherwise merge away!
Why?:
Concerning network observability work, this makes the
oxql
interactive query repl accessible via omdb, as we start to give users and ourselves the ability to query timeseries and metrics more easily. Additionally, in the "now", this aids in debugging through our metrics set and makes it available, via omdb, throughout our ecosystem/a4x2.Includes:
oxql_shell
into the oximeter_db lib for use by both omdb and oxdb.omdb oxql
, it will leverage internal DNS.DNS if no URL is given.
Notes:
specifically v.s. a String. Instead, upon running the
omdb oxql
command,we take in a URL String and parse it into the socket address directly.