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

feat(crowtty): add host-side trace filtering #156

Merged
merged 3 commits into from
Jul 14, 2023
Merged

Conversation

hawkw
Copy link
Contributor

@hawkw hawkw commented Jul 13, 2023

Currently, our ability to filter traces is limited to setting a max
level on the target. This is not ideal --- sometimes, we may want
TRACE events from one module, but enabling TRACE globally results in
an unreadably verbose firehose log.

In the rest of the tracing ecosystem, this is generally solved by
using filters, typically setting a separate maximum level based on
trace targets (module paths). We can't easily implement dynamic target
filtering in the kernel itself, because mnemos_alloc doesn't currently
provide us a nice way of working with owned strings for mudle paths.

However, we can easily implement this kind of filtering on the host
side, in crowtty. This branch adds support for module-path-based
filtering in crowtty, controlled by a --trace flag, which replaces
the previous --trace-level. I felt like breaking the CLI arguments was
probably fine but if someone really cares about backwards compatibility,
we could make the --trace-level flag an alias for --trace <LEVEL>.

In the future, when we have the capacity to do nice string handling in
the kernel, it would be preferable to have these trace filter
configurations be sent to the target device, and have it perform
filtering. That would allow us to actually skip recording unwanted
traces, reducing the performance impact of turning on more verbose
traces. However, with the current design, we will still send the max
level to the target, so if we enable INFO globally and DEBUG in some
specific module, we will, at least, still disable the TRACE level on
the target.

When the capacity to perform filtering in the target is added, the
existing flag can be kept, and the behavior modified to send a
traceproto message that selects that filter configuration.

For example, here's a crowtty session where I enable the TRACE level
for the TWI driver, but INFO for all other modules. Note that none of
the traces from other parts of mnemOS, such as comms::bbq or the SHARP
display driver, are displayed, so we only see the very verbose
diagnostics from the TWI driver.
image

Fixes #153

Currently, our ability to filter traces is limited to setting a max
level on the target. This is not ideal --- sometimes, we may want
`TRACE` events from one module, but enabling `TRACE` globally results in
an unreadably verbose firehose log.

In the rest of the `tracing` ecosystem, this is generally solved by
using *filters*, typically setting a separate maximum level based on
trace targets (module paths). We can't easily implement dynamic target
filtering in the kernel itself, because `mnemos_alloc` doesn't currently
provide us a nice way of working with owned strings for mudle paths.

However, we *can* easily implement this kind of filtering on the host
side, in `crowtty`. This branch adds support for module-path-based
filtering in `crowtty`, controlled by a `--trace` flag, which replaces
the previous `--trace-level`. I felt like breaking the CLI arguments was
probably fine but if someone really cares about backwards compatibility,
we could make the `--trace-level` flag an alias for `--trace <LEVEL>`.

In the future, when we have the capacity to do nice string handling in
the kernel, it would be preferable to have these trace filter
configurations be sent to the target device, and have it perform
filtering. That would allow us to actually skip recording unwanted
traces, reducing the performance impact of turning on more verbose
traces. However, with the current design, we will still send the max
level to the target, so if we enable `INFO` globally and `DEBUG` in some
specific module, we will, at least, still disable the `TRACE` level on
the target.

When the capacity to perform filtering in the target is added, the
existing flag can be kept, and the behavior modified to send a
traceproto message that selects that filter configuration.

Fixes #153
@hawkw hawkw added the area: tools & build Related to host developer tools, including tracing, Crowtty and build processes label Jul 13, 2023
@hawkw hawkw requested a review from jamesmunns July 13, 2023 18:50
@hawkw hawkw self-assigned this Jul 13, 2023
/// maximum `tracing` level to request from the target.
#[arg(short, long, global = true, default_value_t = LevelFilter::INFO)]
trace_level: LevelFilter,
/// a comma-separated list of `tracing` targets and levels to enable.
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be nice to have an example of how to use this in the CLI output, or maybe in the crowtty readme (does crowtty have a readme?)? Maybe something like --trace=kernel=log,maitake=trace,postcard::flavors=debug or whatever.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

crowtty has basically no docs lol...we should fix that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

f5b5250 adds a quick description of how this works.

Copy link
Contributor

@jamesmunns jamesmunns left a comment

Choose a reason for hiding this comment

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

One docs nit, but no complaints and I'm not worried about breaking cli arg changes atm.

@hawkw hawkw merged commit 5f9a743 into main Jul 14, 2023
5 checks passed
@hawkw hawkw deleted the eliza/trace-filter branch July 14, 2023 00:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: tools & build Related to host developer tools, including tracing, Crowtty and build processes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

crowtty should have client side trace filtering
2 participants