trace: Add DmesgCollector #376
Conversation
devlib/trace/dmesg.py
Outdated
| """ | ||
|
|
||
| def parse_ts_msg(line): | ||
| ts_msg_regex = r'\[(.*?)\] (.*)' |
There was a problem hiding this comment.
Pre-compile the regex and reuse instead of compiling for each line?
devlib/trace/dmesg.py
Outdated
| ts, msg = parse_ts_msg(line) | ||
| except ValueError: | ||
| facility, level, remainder = line.split(':', 2) | ||
| ts, msg = parse_ts_msg(remainder) |
There was a problem hiding this comment.
Wrap this in a try catch as well in case the second parsing attempt also fails?
There was a problem hiding this comment.
If this one fails, that means the parser is broken so we want to know about it to fix it. The 1st failure is somehow expected and should always detect correctly what it is supposed to detect (i.e. something before the timestamp [42.4242] or not).
devlib/trace/dmesg.py
Outdated
| :param level: log level | ||
| :type level: str | ||
|
|
||
| :param ts: Timestamp of the entry |
There was a problem hiding this comment.
Nit: it wouldn't hurt to name this "timestamp"
| "debug", # debug-level messages | ||
| ] | ||
|
|
||
| def __init__(self, target, level=LOG_LEVELS[-1], facility='kern'): |
There was a problem hiding this comment.
Could we have a check for that facility being available (other than the dmesg exit code check) ?
I don't know how much you can rely on the output of dmesg -h but you could parse facilities (and log levels BTW) from there. Sort of something like this (but as a static/classmethod maybe) https://github.com/ARM-software/devlib/blob/master/devlib/trace/systrace.py#L61
There was a problem hiding this comment.
Here is the busybox's dmesg help :-)
# dmesg -h
dmesg: invalid option -- 'h'
BusyBox v1.29.2 (2018-12-18 16:55:05 GMT) multi-call binary.
Usage: dmesg [-c] [-n LEVEL] [-s SIZE]
Print or control the kernel ring buffer
-c Clear ring buffer after printing
-n LEVEL Set console logging level
-s SIZE Buffer size
-r Print raw message buffer
Note that there isn't even a --help option, so we can safely forget about any "introspection" capabilities being always available.
There is also relatively little value in practice, since 99% of devlib users will probably restrict that to kern. I had to get the list of log levels for another reason: util-linux dmesg wants an explicit list of all levels to show, i.e. it will not show "levels above X", it will show "level X". Since we usually want the "levels above X" behavior, I had to get the ordered list of levels.
douglas-raillard-arm
force-pushed
the
add_dmesg_collector
branch
from
6231d30 to
c5db648
Compare
Allows collecting dmesg output and parses it for easy filtering.
douglas-raillard-arm
force-pushed
the
add_dmesg_collector
branch
from
c5db648 to
8805e0b
Compare
| ) | ||
|
|
||
|
|
||
| class DmesgCollector(TraceCollector): |
There was a problem hiding this comment.
Not your problem, but I find this hierarchy confusing. Could the base class be named something different like BufferCollector? I appreciate this is probably outside the scope of this patch by the way. But dmesg and tracing has no relationship at all.
There was a problem hiding this comment.
Devlib's notion of tracing != Ftrace. Here it's just something you can start, stop, and get a log/trace/item/<find name you like> containing something that happening during this start & stop.
There was a problem hiding this comment.
Tracing doesn't have to be ftrace. But tracing implies a step by step break down of something. Again it seems you stared at this code for too long.
Anyways. This comment was directed towards the maintainers if they'd like to consider renaming following Douglas suggestion.
|
|
||
| self.dmesg_out = self.target.execute(cmd) | ||
|
|
||
| def get_trace(self, outfile): |
There was a problem hiding this comment.
Nit: get_log() or get_buffer() makes more sense here if it can be changed. I appreciate this is overriding a similarly named method though. So again out of the scope of this patch probably.
There was a problem hiding this comment.
get_trace() is part of the TraceCollector API (https://github.com/ARM-software/devlib/blob/master/devlib/trace/__init__.py), so we have to provide it.
In the end dmesg is a trace itself (you get events at given timestamps), so it's not too outlandish if you look at it the right way.
There was a problem hiding this comment.
Yes I know. I did say that in my comment that I appreciate it's overriding a similarly named function and that it's probably outside the scope of this patch.
n the end dmesg is a trace itself (you get events at given timestamps), so it's not too outlandish if you look at it the right way.
I think you stared at these stuff for too long. A dmesg is not a trace. Not even remotely.
There was a problem hiding this comment.
I guess it depends on the usage of printk: sometimes the order does not hugely matter and it's just a way to log stuff to show to the user (some boot-time messages). Sometimes printk can be treated as timestamped events, like when enabling cpufreq debugging, in which case it is very much like a basic trace system (and I intend to mix that with other actual ftrace events on a timeline, so they can definitely be treated as equivalent).
There was a problem hiding this comment.
In any case, I agree that TraceCollector is not the best name for the collector's base. Something agnostic like CollectorBase may be more appropriate, but that would mean rename + alias on the old name.
Agreed. The naming issue is orthogonal to the particulars of this PR, and is worth putting some thought into to get right; there is no point in blocking this PR on that. I agree that "trace" is not an appropriate term for everything that's currently being collected via the |
|
In case my comment was seeing as an objection. I'm okay with this change. As I mentioned my comments are probably outside the scope of this patch, so certainly shouldn't block this PR :) |
Allows collecting dmesg output and parses it for easy filtering.