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

janus_cli: add DAP message decoder #450

Merged
merged 1 commit into from
Sep 1, 2022
Merged

Conversation

tgeoghegan
Copy link
Contributor

janus_cli decode-dap-message can read binary encoded DAP wire messages
from either stdin or a file and then decode them as a specified message
type, printing the std::fmt::Debug representation of the decoded
message to stdout.

janus_cli uses some of the scaffolding from the janus server
binaries for option parsing, and so it used to expect to be provided a
configuration file with logging and particularly datbase config. That
isn't needed for the decode-dap-message command, so we also make the
config file path optional.

Resolves #441

@tgeoghegan tgeoghegan requested a review from a team as a code owner August 31, 2022 02:24
@tgeoghegan
Copy link
Contributor Author

I felt a certain tension while working on this PR: janus_cli ought to eventually become a task provisioning, control plane type service that runs in cluster, and so it behaves in a lot of ways like other janus server binaries, particularly in how it handles options and a config file. But this new functionality doesn't need all that. I considered adding a whole new binary target that wouldn't take nearly so many flags as janus_cli, but that didn't feel right either. I settled for making janus_cli's more cumbersome config optional, with the expectation that someday we'll tear the control plane-y bits out of janus_cli.


let mut binary_message = Cursor::new(message_buf.as_slice());

let decoded = match media_type {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opted to only support the top-level messages that go on the wire as opposed to every single struct with a prio::codec::Decode implementation, mainly so this match wouldn't get too sprawling and huge.

Copy link
Collaborator

@divergentdave divergentdave left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, modulo updating the CLI reftests

Copy link
Contributor

@branlwyd branlwyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I felt a certain tension while working on this PR: janus_cli ought to eventually become a task provisioning, control plane type service that runs in cluster, and so it behaves in a lot of ways like other janus server binaries, particularly in how it handles options and a config file.

I think I disagree with the direction stated here -- janus_cli is a CLI tool, intended to be used manually by a human operator. Automated "control-plane" work would ideally be done by a separate job. (of course, that's the ideal/aspiration--I've seen plenty of times where "control plane work" actually was done by invoking some CLI tool with a cron job or the like--just trying to define how I see the Janus CLI's purpose for use when deciding how to grow it in the future)

My conception of how the configs would work is that you'd keep one config per environment around on your workstation, and refer to them as needed -- they won't change much/ever (barring backwards-incompatible changes to our config formats, of course), so they're effectively static config data.

@branlwyd
Copy link
Contributor

Oh! I think you meant that janus_cli would talk to a control-plane service -- which might very well be the case, eventually, for at least some of its operations. But either way, janus_cli itself is still intended to be the human-operator CLI tool IMO.

@tgeoghegan
Copy link
Contributor Author

Yes, you're right, we will want a tool that can talk to the control plane, and janus_cli ought to be it (I might argue for renaming it to janusctl because I think that's cooler). However I think we may want to distribute functionality like decode-dap-message separately, in a more lightweight tool, so that people outside of Divvi Up can use it without having to put together a configuration file. Simon Friedberger had the idea of moving all the DAP message definitions to a standalone crate that janus could then depend on, which would be a good place from which to publish a message decoder tool, but there's a whole lot of dependency and repository wrangling entailed by that idea that I don't want to tackle at the moment.

@tgeoghegan tgeoghegan force-pushed the timg/protocol-message-decoder branch 2 times, most recently from d25240a to 4a05e16 Compare August 31, 2022 22:44
@tgeoghegan
Copy link
Contributor Author

Downgrading to draft while I re-work the argument handing here and in #448

@tgeoghegan tgeoghegan marked this pull request as draft August 31, 2022 22:46
@tgeoghegan tgeoghegan force-pushed the timg/protocol-message-decoder branch from 4a05e16 to 8f51d7d Compare August 31, 2022 23:27
`janus_cli decode-dap-message` can read binary encoded DAP wire messages
from either stdin or a file and then decode them as a specified message
type, printing the `std::fmt::Debug` representation of the decoded
message to stdout.

`janus_cli` uses some of the scaffolding from the `janus` server
binaries for option parsing, and so it used to expect to be provided a
configuration file with logging and particularly datbase config. That
isn't needed for the `decode-dap-message` command, so we lower the
`CommonBinaryOptions` from `janus_cli::Options` down into those variants
of `janus_cli::Command` that actually need those flags.

Resolves #441
@tgeoghegan tgeoghegan force-pushed the timg/protocol-message-decoder branch from 8f51d7d to d2f0e96 Compare August 31, 2022 23:41
@tgeoghegan tgeoghegan marked this pull request as ready for review September 1, 2022 00:06
@tgeoghegan tgeoghegan merged commit 03c32a4 into main Sep 1, 2022
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.

Protocol message decoding tool
3 participants