ftrace: Update ftrace event regex#262
ftrace: Update ftrace event regex#262felix-khlmnn wants to merge 2 commits intoeclipse-tracecompass-incubator:masterfrom
Conversation
The regex used for the parsing of ftrace events proved inadequate for parsing certain events, namely those that started with opening parentheses, as described in issue eclipse-tracecompass-incubator#239. The solution is an update to the regex which I have attempted to verify using regex101 in Java mode. Fixes: eclipse-tracecompass-incubator#239 Signed-off-by: Felix Kuhlmann <felix-kuhlmann@gmx.de>
📝 WalkthroughWalkthroughThe FTRACE_PATTERN regex was changed to capture the entire remainder of the line (including any parentheses) instead of stopping before a closing parenthesis. A unit test was added to validate parsing of event names that begin with "(". Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tracetypes/org.eclipse.tracecompass.incubator.ftrace.core/src/org/eclipse/tracecompass/incubator/internal/ftrace/core/event/IGenericFtraceConstants.java (1)
60-71: The regex change has been applied, but test coverage for the issue is incomplete.The regex pattern on line 70 has been updated to
(?<data>.*)$(merged in commit ff0676e to fix issue #239). The change correctly makes the data capture group more permissive by capturing everything until end-of-line, which allows handling of events with parentheses.However, while the test suite includes
testSpecialCharsInComm()which covers brackets likerunc:[2:INIT]and spaces in comm fields, there is no specific test case for the actual issue being fixed: parentheses in the comm field (e.g.,(udev-worker)-PID).Add a test case to
FtraceFieldTest.javathat covers the specific scenario described in issue #239:Suggested test case
new ResultsParse("(udev-worker)-1234 [002] d... 149.136514: sched_process_fork: prev_comm=(udev-worker) prev_pid=1234 child_comm=systemd child_pid=1", 2, "sched_process_fork", Objects.requireNonNull(Map.of("prev_comm", "(udev-worker)")), 3)This ensures the regex properly handles comm fields enclosed in parentheses as intended by the fix.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tracetypes/org.eclipse.tracecompass.incubator.ftrace.core/src/org/eclipse/tracecompass/incubator/internal/ftrace/core/event/IGenericFtraceConstants.java
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tracetypes/org.eclipse.tracecompass.incubator.ftrace.core.tests/src/org/eclipse/tracecompass/incubator/ftrace/core/tests/event/FtraceFieldTest.java (1)
194-212: Consider validating all content fields and TID for test completeness.The test correctly validates the parentheses parsing fix. However, for consistency with other tests in this class:
- Line 204 asserts 4 content fields exist, but only 3 are validated (comm, runtime, vruntime). The
pidfield from the event content is not checked.- Similar tests (e.g., lines 49-50 in
testParseSchedWakeupLine) also assert the TID value.🔎 Suggested additions for completeness
assertNotNull(field); assertEquals(4, field.getContent().getFields().size()); assertEquals((Integer) 0, field.getCpu()); assertEquals((Integer) 299, field.getPid()); + assertEquals((Integer) 299, field.getTid()); assertEquals("sched_stat_runtime", field.getName()); assertEquals("systemd-udevd", field.getContent().getFieldValue(String.class, "comm")); + assertEquals((Long) 299L, field.getContent().getFieldValue(Long.class, "pid")); assertEquals((Long) 65790, field.getContent().getFieldValue(Long.class, "runtime")); assertEquals((Long) 6269486605L, field.getContent().getFieldValue(Long.class, "vruntime"));
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tracetypes/org.eclipse.tracecompass.incubator.ftrace.core.tests/src/org/eclipse/tracecompass/incubator/ftrace/core/tests/event/FtraceFieldTest.java
|
I have added a unit test as per CodeRabbits recommendation that tests the specific use case that doesn't with status quo. If the test should instead be moved to the special chars test case, I'll adapt the test. |
MatthewKhouzam
left a comment
MatthewKhouzam
left a comment
There was a problem hiding this comment.
This is a no-brainer, thanks for finding and fixing the bug!
|
Let's wait for CI thought. |
Had a minor mistake in the test implementation, the test itself should be correctly implemented now. |
|
Could you please squash the last two commits? I approve them! ;) |
Implements a test case for the parentheses error described in eclipse-tracecompass-incubator#239. Signed-off-by: Felix Kuhlmann <felix-kuhlmann@gmx.de>
|
That's not good, the CI fails at another test which means I need to rework the string. I'll still squash the commits and fix the regex afterwards. |
|
FtraceFieldTest.testParseSysCallEnterFTrace:108 expected:<0> but was: |
|
Perfect, again, thanks! |
felix-khlmnn
force-pushed
the
ftrace-regex-update
branch
from
514d557 to
4e75b76
Compare
|
Alright, the original regex works as intended with the Thanks @MatthewKhouzam for the support :) |
|
I checked the code... This is because of the greedy .*. I am tempted to implement a basic bracket parser. i.e. a +1 on ( and -1 on ) to reliably fix it. But it may be overkill. |
2 participants
What it does
Fixes: #239
As described in issue #239, the regex used for parsing ftrace events currently fails to correctly parse text files under specific circumstances. I have found that to be the case with event names that start with opening parentheses. It only occurs when parsing text files, which was necessary in my case.
In my case, the event
(udev-worker)caused the parsing of the file to be halted. It only managed to fully parse the file when I changed every occurence of(udev-worker)toudev-worker. This led me to assume that the parentheses mess with the parsing of the data fields.Using regex101, I have adopted the regex to correctly parse the trace provided in the "How to test" section. No further testing could be done yet as explained in the issue.
How to test
The testing can be done using an example of a trace I have provided here.
trace-cmd report -i long_program.dat > long_program.txtFollow-ups
If I encounter similar issues I will report them in reference to this topic and update the regex if appropriate.
Review checklist
(To the best of my ability, see linked issue)
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.