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

Move oxql from oxdb to mainline omdb #5988

Merged
merged 4 commits into from
Jul 9, 2024
Merged

Move oxql from oxdb to mainline omdb #5988

merged 4 commits into from
Jul 9, 2024

Conversation

zeeshanlakhani
Copy link
Collaborator

@zeeshanlakhani zeeshanlakhani commented Jul 2, 2024

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:

  • Moves oxql_shell into the oximeter_db lib for use by both omdb and oxdb.
  • 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.

@zeeshanlakhani zeeshanlakhani marked this pull request as ready for review July 2, 2024 10:50
@zeeshanlakhani zeeshanlakhani force-pushed the zl/omdb-oxql branch 2 times, most recently from 6fad0a7 to 9addfda Compare July 2, 2024 11:55
@zeeshanlakhani zeeshanlakhani marked this pull request as draft July 2, 2024 14:22
@zeeshanlakhani
Copy link
Collaborator Author

Last update: fix ordering as output will switch order.

@zeeshanlakhani zeeshanlakhani force-pushed the zl/omdb-oxql branch 3 times, most recently from 383128c to 03ab429 Compare July 3, 2024 16:58
@zeeshanlakhani zeeshanlakhani marked this pull request as ready for review July 3, 2024 16:59
Copy link
Collaborator

@bnaecker bnaecker left a 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!

dev-tools/omdb/src/bin/omdb/oximeter.rs Outdated Show resolved Hide resolved
dev-tools/omdb/tests/env.out Show resolved Hide resolved
dev-tools/omdb/tests/successes.out Show resolved Hide resolved
dev-tools/omdb/tests/test_all_output.rs Outdated Show resolved Hide resolved
dev-tools/omdb/tests/test_all_output.rs Outdated Show resolved Hide resolved
nexus/db-model/src/tuf_repo.rs Outdated Show resolved Hide resolved
test-utils/src/dev/test_cmds.rs Outdated Show resolved Hide resolved
oximeter/db/Cargo.toml Outdated Show resolved Hide resolved
oximeter/db/src/bin/oxdb/main.rs Show resolved Hide resolved
dev-tools/omdb/src/bin/omdb/oxql.rs Show resolved Hide resolved
@rcgoodfellow
Copy link
Contributor

I just have a high-level comment here.

While having OxQL capabilities in omdb will certainly make it convenient for Oxide folks to work with metrics inside the rack (virtual or otherwise), it's important to keep in mind that a major driving use case is having metrics query capabilities through the external API. I recognize that it takes prototyping and iteration to get toward a stable API, and omdb can make doing some of that lighter weight than having to plumb things all the way through. But we should get things into the experimental API as soon as possible to get laps on the OxQL mechanisms in the form they'll be exposed to users and operators to deliver a polished and well-exercised experience.

@bnaecker
Copy link
Collaborator

bnaecker commented Jul 5, 2024

@rcgoodfellow Running OxQL queries is currently supported via the CLI, under the oxide experimental timeseries query subcommand. You can also list available schema with oxide experiment timeseries schema list, and if you're feeling fancy, plot a query with oxide experimental timeseries dashboard.

@rcgoodfellow
Copy link
Contributor

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 omdb.

@bnaecker
Copy link
Collaborator

bnaecker commented Jul 5, 2024

Oh, that context is helpful @rcgoodfellow. I agree we should limit the degree to which we build specific functionality in omdb, rather than making the language or CLI more useful. My main goal here is to make it easier for us to dogfood the language, since we're often in a situation where omdb is available, but neither the Oxide CLI nor the oxdb binary which currently implements the OxQL shell are.

@zeeshanlakhani
Copy link
Collaborator Author

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 omdb.

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.

@davepacheco
Copy link
Collaborator

My main goal here is to make it easier for us to dogfood the language, since we're often in a situation where omdb is available, but neither the Oxide CLI nor the oxdb binary which currently implements the OxQL shell are.

Would it address this problem to have the switch zone ship oxdb the same way it ships omdb? I'm guessing that wouldn't really help @rcgoodfellow's concern because oxdb isn't using the external API? I don't have an opinion on this either way -- both concerns make sense -- I'm just curious if that would help.

@bnaecker
Copy link
Collaborator

bnaecker commented Jul 6, 2024

@davepacheco I think that would work, though oxdb has a lot of other subcommands that I'm more wary of including. For example, one can easily wipe the whole ClickHouse database! I also think there's some value in continuing to make omdb the one-stop-shop for debugging control plane issues.

@zeeshanlakhani
Copy link
Collaborator Author

@davepacheco, in talking to @bnaecker, it seemed like we should graduate oxql for helpful data debugging, more dogfooding, and to help build more streamlined tools and API endpoints. Like @bnaecker mentioned, it seemed like the sql and other commands were a bit more in the prototype stages, and having both omdb and oxdb could be more confusing generally.

Copy link
Collaborator

@bnaecker bnaecker left a 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!

oximeter/impl/src/schema/mod.rs Outdated Show resolved Hide resolved
oximeter/impl/src/schema/mod.rs Outdated Show resolved Hide resolved
dev-tools/omdb/tests/test_all_output.rs Outdated Show resolved Hide resolved
dev-tools/omdb/tests/test_all_output.rs Outdated Show resolved Hide resolved
dev-tools/omdb/tests/test_all_output.rs Outdated Show resolved Hide resolved
dev-tools/omdb/src/bin/omdb/oxql.rs Outdated Show resolved Hide resolved
dev-tools/omdb/src/bin/omdb/oxql.rs Show resolved Hide resolved
oximeter/db/src/bin/oxdb/oxql.rs Show resolved Hide resolved
dev-tools/omdb/tests/test_all_output.rs Outdated Show resolved Hide resolved
oximeter/db/src/lib.rs Show resolved Hide resolved
zeeshanlakhani and others added 2 commits July 8, 2024 21:00
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]>
Copy link
Collaborator

@bnaecker bnaecker left a 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!

@zeeshanlakhani zeeshanlakhani enabled auto-merge (squash) July 9, 2024 01:57
@zeeshanlakhani zeeshanlakhani merged commit dcfac83 into main Jul 9, 2024
21 checks passed
@zeeshanlakhani zeeshanlakhani deleted the zl/omdb-oxql branch July 9, 2024 03:16
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.

4 participants