counters: add --ipcs command for showing IPC counters by interface
#452
Conversation
don't love the output though...
|
I admit I still find the arrows and brackets confusing, particularly the converging ones, but that might just be me. (I'd use words, personally. I implemented the e.g. I read this as "we called send_packet 1380 times, of which 1380 times we sent Ok and, uh, udpbroadcast?" I could surely get used to any format we choose, so don't interpret this as a hard no. What I think this is saying is
Off the top of my head, one alternate way of presenting this that I'd find clearer might be (sketchy): ...with a flag for showing only a single task if desired. My implicit assumptions here include, but are likely not limited to:
You get the idea, I hope, and maybe there are things here that are useful to you. FYI, I am sensitive to dismissal of UX issues as "bikeshedding" and would prefer to avoid that. |
|
Okay, I've made a great deal of changes to the output format based on @cbiffle's suggestions, and updated the PR description to show the new output. Also, I've added a |
| /// when used with `--ipc`, show only IPC counters originating from tasks | ||
| /// whose name contain the given substring. | ||
| /// | ||
| /// multiple values may be provided to select more than one client task. | ||
| #[clap(long, short, conflicts_with = "list", requires = "ipc")] | ||
| client: Vec<String>, |
There was a problem hiding this comment.
As there's now a flag that's only relevant when --ipc is passed, I wonder if it would be a good idea to change --ipc from a flag to an ipc subcommand, so that the --client flag can only be defined for that subcommand? On the other hand, I haven't seen many other nested subcommands in Humility --- do we generally avoid nesting subcommands by convention?
There was a problem hiding this comment.
Bryan tends not to use more than one level of subcommand; I tend to use subcommands and positional arguments in preference to flags in a lot of situations. Kind of depends on who wrote the command.
I'd enthusiastically support subcommanding this case.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
license-eye has totally checked 101 files.
| Valid | Invalid | Ignored | Fixed |
|---|---|---|---|
| 100 | 1 | 0 | 0 |
Click to see the invalid file list
- cmd/counters/src/ipc.rs
cmd/counters/src/lib.rs
Outdated
| let err_str = | ||
| if num_important_tasks > 1 { "+ 0" } else { "= 0" }; | ||
| let pad1 = 80usize | ||
| .saturating_sub(err_str.len()) |
There was a problem hiding this comment.
All these are almost an argument for using Saturating here, buuuut I think it'd wind up just as verbose and I'm not sure.
There was a problem hiding this comment.
TIL about Saturating! That must be new!
There was a problem hiding this comment.
Oh, it is --- stabilized in 1.74.0!
I think these would be a bit less verbose using Saturating, since we could just wrap the 80 in Saturating and then do the rest of the arithmetic normally, but it would require updating our MSRV to 1.74.0...
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
As suggested by @bcantrill in [this comment][1], recent changes from PRs #452 and # 453 have added new user-visible features, so we ought to bump the patch version of Humility to indicate this. [1]: #453 (review)
As suggested by @bcantrill in [this comment][1], recent changes from PRs #452 and # 453 have added new user-visible features, so we ought to bump the patch version of Humility to indicate this. [1]: #453 (review)
PR oxidecomputer/idolatry#43 enhanced Idolatry's codegen to support
automatically generating counters of IPC operations in the generated IPC
client and server stubs. By using client-side counters, we can display
both a total count of IPC operations for a particular IPC interface,
and a breakdown of those IPC oeprations by client task. However, the
current
humility counterscommand can only do the latter, as itdisplays each instance of a counter variable separately, even when
multiple tasks contain the same counter.
Therefore, this PR adds a new
--ipcflag tohumility counters. Thisflag will sum the IPC client counters by IPC interface, and then display
the total count for each operation followed by a breakdown by response
type, and then by client. This allows us to see which clients called
which IPC methods, what responses were received by each client, and the
fractions of the total number of IPC requests that each client was
responsible for.
Example output:
Displaying counters for a particular IPC interface:
Using the `--client` flag to only display counts recorded by specific tasks::
By default, all non-zero counters are displayed:
The `--full` argument includes all counters, including those which are zero: