Synchronize the start/stop LTTng trace in Control view with the external trace#406
Synchronize the start/stop LTTng trace in Control view with the external trace#406tuanvtdeptre199 wants to merge 13 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughAdds TMF signals for LTTng session lifecycle (external start/stop and internal start/stop/destroy), updates StartHandler and StopHandler to register with TmfSignalManager and dispatch/handle those signals on the SWT UI thread, and bumps the plugin version while exporting the signals package. ChangesLTTng2 Control UI Signal-Based Session Lifecycle
Sequence Diagram(s)sequenceDiagram
participant StartHandler
participant TmfSignalManager
participant StopHandler
participant UIThread as SWT_Display
participant ExternalProducer
StartHandler->>TmfSignalManager: dispatch LTTngSessionStartSignal(source)
TmfSignalManager->>StopHandler: deliver LTTngSessionStartSignal
StopHandler->>UIThread: asyncExec (add TraceSessionComponent to fSessions)
ExternalProducer->>TmfSignalManager: ExternalTraceStartSignal(sessionName)
TmfSignalManager->>StartHandler: deliver ExternalTraceStartSignal
StartHandler->>UIThread: asyncExec execute(null)
StopHandler->>TmfSignalManager: dispatch LTTngSessionStopSignal(source)
TmfSignalManager->>StartHandler: deliver LTTngSessionStopSignal
StartHandler->>UIThread: asyncExec (record session in fSessions)
ExternalProducer->>TmfSignalManager: ExternalTraceStopSignal(sessionName)
TmfSignalManager->>StopHandler: deliver ExternalTraceStopSignal
StopHandler->>UIThread: asyncExec execute(null)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@lttng/org.eclipse.tracecompass.lttng2.control.ui/src/org/eclipse/tracecompass/internal/lttng2/control/ui/views/handlers/StartHandler.java`:
- Around line 68-78: Remove the debug System.out.println call from
StartHandler.handle(ExternalTraceStartSignal) and update the method Javadoc to
refer to "external trace start signal" / "start external trace" (matching
ExternalTraceStartSignal and the handler's behavior); also replace the
e.printStackTrace() call with the plugin logging facility (e.g., use the bundle
Activator or TraceControlLog to log the exception with an error message) so the
exception is recorded via the plugin logger instead of printing to stderr.
- Around line 39-42: StartHandler currently registers itself with
TmfSignalManager in the constructor (TmfSignalManager.register(this)) but never
deregisters, uses println/printStackTrace in handle(ExternalTraceStartSignal),
has wrong Javadoc, mutates shared fSessions in handle(LTTngSessionStopSignal)
without using the session lock fLock, and calls execute(null); fix by overriding
dispose() to call TmfSignalManager.deregister(this), remove System.out.println
and e.printStackTrace and replace with proper logging, correct the Javadoc on
handle(ExternalTraceStartSignal) to describe start behavior, ensure any
modifications to fSessions (the same collection used by
ChangeSessionStateHandler) are performed while holding fLock (use the same
locking strategy as execute()/isEnabled()), and avoid calling execute(null) from
signal handlers—either invoke the underlying state-change method directly with a
proper parameter or adapt execute to accept a safely-constructed request object
instead.
In
`@lttng/org.eclipse.tracecompass.lttng2.control.ui/src/org/eclipse/tracecompass/internal/lttng2/control/ui/views/handlers/StopHandler.java`:
- Around line 68-78: The method handle(ExternalTraceStopSignal) in StopHandler
contains a debug System.out.println and a printStackTrace call and its Javadoc
incorrectly refers to "start" instead of "stop"; replace the
System.out.println(...) with the plugin logger (use the class's logger instance
to log at debug/info level and include signal.getSessionName()), replace
e.printStackTrace() with the plugin logger.error(..., e), and update the method
Javadoc to describe handling an ExternalTraceStopSignal / stopping an external
trace; leave the execute(null) call as-is.
In
`@lttng/org.eclipse.tracecompass.lttng2.control.ui/src/org/eclipse/tracecompass/lttng2/control/ui/views/signals/ExternalTraceStartSignal.java`:
- Line 1: This file (ExternalTraceStartSignal) and the other new signal classes
in package org.eclipse.tracecompass.lttng2.control.ui.views.signals are missing
the required EPL-2.0 license header; prepend the standard Eclipse Public License
2.0 header used across the project (copy the exact header from existing files
such as StartHandler.java or StopHandler.java) to the top of each new source
file before the package declaration so the license/API verification passes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c03ff260-22e8-4cf6-9700-f910a42bc419
📒 Files selected for processing (8)
lttng/org.eclipse.tracecompass.lttng2.control.ui/META-INF/MANIFEST.MFlttng/org.eclipse.tracecompass.lttng2.control.ui/src/org/eclipse/tracecompass/internal/lttng2/control/ui/views/handlers/StartHandler.javalttng/org.eclipse.tracecompass.lttng2.control.ui/src/org/eclipse/tracecompass/internal/lttng2/control/ui/views/handlers/StopHandler.javalttng/org.eclipse.tracecompass.lttng2.control.ui/src/org/eclipse/tracecompass/lttng2/control/ui/views/signals/ExternalTraceStartSignal.javalttng/org.eclipse.tracecompass.lttng2.control.ui/src/org/eclipse/tracecompass/lttng2/control/ui/views/signals/ExternalTraceStopSignal.javalttng/org.eclipse.tracecompass.lttng2.control.ui/src/org/eclipse/tracecompass/lttng2/control/ui/views/signals/LTTngSessionDestroySignal.javalttng/org.eclipse.tracecompass.lttng2.control.ui/src/org/eclipse/tracecompass/lttng2/control/ui/views/signals/LTTngSessionStartSignal.javalttng/org.eclipse.tracecompass.lttng2.control.ui/src/org/eclipse/tracecompass/lttng2/control/ui/views/signals/LTTngSessionStopSignal.java
|
Hi @jonahgraham, could you please help me review this commit? Thank you! |
MatthewKhouzam
left a comment
MatthewKhouzam
left a comment
There was a problem hiding this comment.
General question: Should this be tracersignal or lttng? is it too generic?
| @@ -1,3 +1,17 @@ | |||
| /******************************************************************************* | |||
| * Copyright (c) 2012, 2015 Ericsson | |||
There was a problem hiding this comment.
change to relevant ;)
There was a problem hiding this comment.
Hi @MatthewKhouzam, I have updated it. Please help me recheck it. (Honestly, I am not familiar with the EPL copyright/license. Sorry for the inconvenience)
There was a problem hiding this comment.
No worries, you're doing great.
| @@ -1,3 +1,17 @@ | |||
| /******************************************************************************* | |||
| * Copyright (c) 2012, 2015 Ericsson | |||
There was a problem hiding this comment.
I have updated it
| @@ -1,3 +1,17 @@ | |||
| /******************************************************************************* | |||
| * Copyright (c) 2012, 2015 Ericsson | |||
There was a problem hiding this comment.
Same as above
| @@ -1,3 +1,17 @@ | |||
| /******************************************************************************* | |||
| * Copyright (c) 2012, 2015 Ericsson | |||
There was a problem hiding this comment.
Same as above
| @@ -1,3 +1,17 @@ | |||
| /******************************************************************************* | |||
| * Copyright (c) 2012, 2015 Ericsson | |||
There was a problem hiding this comment.
Same as above
|
Hi @MatthewKhouzam, regarding your question "Should this be tracersignal or lttng? Is it too generic?". Maybe I do not fully understand your idea. Could you please explain more details about your opinion? Do you want me to change the class/method/var name, or something else? |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@tmf/org.eclipse.tracecompass.tmf.ui.swtbot.tests/src/org/eclipse/tracecompass/tmf/ui/swtbot/tests/project/AddProjectNatureTest.java`:
- Around line 255-279: The fallback currently retries
dialogBot.button(APPLY_BUTTON).click() without handling the case where the Apply
button exists but remains disabled; update the catch block after fBot.waitUntil
(around fBot.waitUntil(...) and the subsequent catch) to first obtain the
SWTBotButton for APPLY_BUTTON (via dialogBot.button(APPLY_BUTTON)), check
button.isEnabled(), and if true call click(), otherwise treat it as the disabled
case and fall back to trying dialogBot.button("OK").click(); keep
WidgetNotFoundException handling for the case the Apply button truly doesn't
exist and preserve the existing available-buttons diagnostic logic when both
Apply and OK are unavailable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cbc0f6fd-8a5a-49e4-a118-3cef2f38a2cc
📒 Files selected for processing (1)
tmf/org.eclipse.tracecompass.tmf.ui.swtbot.tests/src/org/eclipse/tracecompass/tmf/ui/swtbot/tests/project/AddProjectNatureTest.java
| try { | ||
| // Wait for the Apply button to be enabled (up to 5 seconds) | ||
| fBot.waitUntil(org.eclipse.swtbot.swt.finder.waits.Conditions.widgetIsEnabled( | ||
| dialogBot.button(APPLY_BUTTON)), 5000); | ||
| } catch (Exception e) { | ||
| // If button is not found or doesn't enable, try alternative approaches | ||
| // First, wait a bit more for UI to settle | ||
| fBot.sleep(1000); | ||
| try { | ||
| dialogBot.button(APPLY_BUTTON).click(); | ||
| } catch (WidgetNotFoundException e2) { | ||
| // If Apply button is still not found, try OK button as fallback | ||
| try { | ||
| dialogBot.button("OK").click(); | ||
| return; | ||
| } catch (WidgetNotFoundException e3) { | ||
| // Log available buttons for debugging | ||
| org.eclipse.swtbot.swt.finder.widgets.SWTBotButton[] buttons = dialogBot.allButtons(); | ||
| StringBuilder availableButtons = new StringBuilder("Available buttons: "); | ||
| for (org.eclipse.swtbot.swt.finder.widgets.SWTBotButton btn : buttons) { | ||
| availableButtons.append("[").append(btn.getText()).append("] "); | ||
| } | ||
| throw new WidgetNotFoundException(availableButtons.toString(), e3); | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle the disabled-Apply case in the fallback path.
When the wait on Lines 257-258 fails because Apply exists but never becomes enabled, this branch still retries dialogBot.button(APPLY_BUTTON).click() and only falls back to OK if the button is missing. That means the exact timeout case this change is trying to absorb can still fail instead of taking the fallback path.
Suggested fix
SWTBot dialogBot = shell.bot();
try {
// Wait for the Apply button to be enabled (up to 5 seconds)
fBot.waitUntil(org.eclipse.swtbot.swt.finder.waits.Conditions.widgetIsEnabled(
dialogBot.button(APPLY_BUTTON)), 5000);
} catch (Exception e) {
// If button is not found or doesn't enable, try alternative approaches
// First, wait a bit more for UI to settle
fBot.sleep(1000);
try {
- dialogBot.button(APPLY_BUTTON).click();
+ if (dialogBot.button(APPLY_BUTTON).isEnabled()) {
+ dialogBot.button(APPLY_BUTTON).click();
+ return;
+ }
+ dialogBot.button("OK").click();
+ return;
} catch (WidgetNotFoundException e2) {
// If Apply button is still not found, try OK button as fallback
try {
dialogBot.button("OK").click();
return;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@tmf/org.eclipse.tracecompass.tmf.ui.swtbot.tests/src/org/eclipse/tracecompass/tmf/ui/swtbot/tests/project/AddProjectNatureTest.java`
around lines 255 - 279, The fallback currently retries
dialogBot.button(APPLY_BUTTON).click() without handling the case where the Apply
button exists but remains disabled; update the catch block after fBot.waitUntil
(around fBot.waitUntil(...) and the subsequent catch) to first obtain the
SWTBotButton for APPLY_BUTTON (via dialogBot.button(APPLY_BUTTON)), check
button.isEnabled(), and if true call click(), otherwise treat it as the disabled
case and fall back to trying dialogBot.button("OK").click(); keep
WidgetNotFoundException handling for the case the Apply button truly doesn't
exist and preserve the existing available-buttons diagnostic logic when both
Apply and OK are unavailable.
|
Hi @MatthewKhouzam , |
|
Hi, regarding copyright headers, since I don't think you work at Ericsson, add your own company. this isn't our code ;) About SWTBot, right now the framework changed as we are in the RC cycle. rebase on master, it should fix it. |
|
Hi @MatthewKhouzam , |
|
My thought. LTTng is one tracer we support like that. should we make it generic and rename the signal? what I will propose, we keep it as-is. and only after we have a need we cross that bridge. |
MatthewKhouzam
left a comment
MatthewKhouzam
left a comment
There was a problem hiding this comment.
I am approving this, but only for merge after the release.
|
@MatthewKhouzam, Thank you for your comment. Could you please share with me about the trace tools that you are interested in? In Trace Compass, I use the Control view and the other views to work with only the LTTng tool. So I think it is great if my 3rd trace tool can work with more trace tools than the LTTng. |
|
I can see eventually otel/jaeger, perf, ftrace, uftrace control. but lttng should be first. |
2 participants
Synchronize the start/stop LTTng trace in Control view with the external trace
What it does
I want to synchronize the start/stop action in the Control view with the start/stop action in my 3rd trace view, so I can synchronize the LTTng tracing with my 3rd tool tracing. Therefore, I want to:
+) Create new signals to sync the start/stop actions in the Control view with the other views
+) Update the start/stop handler classes to sync the start/stop LTTng trace action with the other views
How to test
Test the start/stop action in the Control view: In case using the Trace Compass software, the start/stop action in the Control view still works normally, as it does not contain my 3rd view.
Follow-ups
Review checklist
Summary by CodeRabbit
New Features
Chores
Tests