From 32a3b75348d5fd05b89663e989df334bc311791b Mon Sep 17 00:00:00 2001 From: Christophe Bedard Date: Mon, 10 Apr 2023 13:37:41 -0700 Subject: [PATCH 1/2] Add tests for new append_trace option And add test for existing append_timestamp Trace launch action option. Signed-off-by: Christophe Bedard --- ros2trace/test/ros2trace/test_trace.py | 31 +++++ .../test_trace_action.py | 112 +++++++++++++++--- 2 files changed, 124 insertions(+), 19 deletions(-) diff --git a/ros2trace/test/ros2trace/test_trace.py b/ros2trace/test/ros2trace/test_trace.py index b6af13b5..de8e35a4 100644 --- a/ros2trace/test/ros2trace/test_trace.py +++ b/ros2trace/test/ros2trace/test_trace.py @@ -349,3 +349,34 @@ def test_unknown_context_field(self) -> None: self.assertEqual(1, ret) shutil.rmtree(tmpdir) + + def test_append_trace(self) -> None: + tmpdir = self.create_test_tmpdir('test_append_trace') + + # Generate a normal trace + ret = self.run_trace_command( + ['--path', tmpdir, '--session-name', 'test_append_trace'], + ) + self.assertEqual(0, ret) + trace_dir = os.path.join(tmpdir, 'test_append_trace') + self.assertTraceExist(trace_dir) + + # Generating another trace with the same path should error out + ret = self.run_trace_command( + ['--path', tmpdir, '--session-name', 'test_append_trace'], + ) + self.assertEqual(1, ret) + self.assertTraceExist(trace_dir) + + # But it should work if we use the '--append-trace' option + ret = self.run_trace_command( + [ + '--path', tmpdir, + '--session-name', 'test_append_trace', + '--append-trace', + ], + ) + self.assertEqual(0, ret) + self.assertTraceExist(trace_dir) + + shutil.rmtree(tmpdir) diff --git a/test_tracetools_launch/test/test_tracetools_launch/test_trace_action.py b/test_tracetools_launch/test/test_tracetools_launch/test_trace_action.py index 144b57ad..75579029 100644 --- a/test_tracetools_launch/test/test_tracetools_launch/test_trace_action.py +++ b/test_tracetools_launch/test/test_tracetools_launch/test_trace_action.py @@ -20,6 +20,7 @@ import tempfile import textwrap from typing import List +from typing import Optional import unittest from launch import LaunchDescription @@ -43,18 +44,31 @@ def __init__(self, *args) -> None: *args, ) - def _assert_launch_no_errors(self, actions) -> None: + def setUp(self) -> None: + self.assertIsNone(os.environ.get('LD_PRELOAD')) + + def tearDown(self) -> None: + if 'LD_PRELOAD' in os.environ: + del os.environ['LD_PRELOAD'] + + def _assert_launch(self, actions) -> int: ld = LaunchDescription(actions) ls = LaunchService(debug=True) ls.include_launch_description(ld) - assert 0 == ls.run() + return ls.run() + + def _assert_launch_no_errors(self, actions) -> None: + self.assertEqual(0, self._assert_launch(actions), 'expected no errors') + + def _assert_launch_errors(self, actions) -> None: + self.assertEqual(1, self._assert_launch(actions), 'expected errors') def _assert_launch_frontend_no_errors(self, file) -> Trace: root_entity, parser = Parser.load(file) ld = parser.parse_description(root_entity) ls = LaunchService() ls.include_launch_description(ld) - assert 0 == ls.run() + self.assertEqual(0, ls.run(), 'expected no errors') trace_action = ld.describe_sub_entities()[0] return trace_action @@ -62,14 +76,18 @@ def _check_trace_action( self, action, tmpdir, - session_name: str = 'my-session-name', + *, + session_name: Optional[str] = 'my-session-name', + append_trace: bool = False, events_ust: List[str] = ['ros2:*', '*'], subbuffer_size_ust: int = 524288, subbuffer_size_kernel: int = 1048576, ) -> None: - self.assertEqual(session_name, action.session_name) + if session_name is not None: + self.assertEqual(session_name, action.session_name) self.assertEqual(tmpdir, action.base_path) self.assertTrue(action.trace_directory.startswith(tmpdir)) + self.assertEqual(append_trace, action.append_trace) self.assertEqual([], action.events_kernel) self.assertEqual(events_ust, action.events_ust) self.assertTrue(pathlib.Path(tmpdir).exists()) @@ -77,7 +95,6 @@ def _check_trace_action( self.assertEqual(subbuffer_size_kernel, action.subbuffer_size_kernel) def test_action(self) -> None: - self.assertIsNone(os.environ.get('LD_PRELOAD')) tmpdir = tempfile.mkdtemp(prefix='TestTraceAction__test_action') # Disable kernel events just to not require kernel tracing for the test @@ -96,10 +113,8 @@ def test_action(self) -> None: self._check_trace_action(action, tmpdir) shutil.rmtree(tmpdir) - del os.environ['LD_PRELOAD'] def test_action_frontend_xml(self) -> None: - self.assertIsNone(os.environ.get('LD_PRELOAD')) tmpdir = tempfile.mkdtemp(prefix='TestTraceAction__test_frontend_xml') xml_file = textwrap.dedent( @@ -107,7 +122,9 @@ def test_action_frontend_xml(self) -> None: None: with io.StringIO(xml_file) as f: trace_action = self._assert_launch_frontend_no_errors(f) - self._check_trace_action(trace_action, tmpdir) + self._check_trace_action(trace_action, tmpdir, append_trace=True) shutil.rmtree(tmpdir) - del os.environ['LD_PRELOAD'] def test_action_frontend_yaml(self) -> None: - self.assertIsNone(os.environ.get('LD_PRELOAD')) tmpdir = tempfile.mkdtemp(prefix='TestTraceAction__test_frontend_yaml') yaml_file = textwrap.dedent( @@ -135,7 +150,9 @@ def test_action_frontend_yaml(self) -> None: launch: - trace: session-name: my-session-name + append-timestamp: false base-path: {} + append-trace: true events-kernel: "" events-ust: ros2:* * subbuffer-size-ust: 524288 @@ -147,13 +164,11 @@ def test_action_frontend_yaml(self) -> None: with io.StringIO(yaml_file) as f: trace_action = self._assert_launch_frontend_no_errors(f) - self._check_trace_action(trace_action, tmpdir) + self._check_trace_action(trace_action, tmpdir, append_trace=True) shutil.rmtree(tmpdir) - del os.environ['LD_PRELOAD'] def test_action_context_per_domain(self) -> None: - self.assertIsNone(os.environ.get('LD_PRELOAD')) tmpdir = tempfile.mkdtemp(prefix='TestTraceAction__test_action_context_per_domain') action = Trace( @@ -183,10 +198,8 @@ def test_action_context_per_domain(self) -> None: ) shutil.rmtree(tmpdir) - del os.environ['LD_PRELOAD'] def test_action_substitutions(self) -> None: - self.assertIsNone(os.environ.get('LD_PRELOAD')) tmpdir = tempfile.mkdtemp(prefix='TestTraceAction__test_action_substitutions') self.assertIsNone(os.environ.get('TestTraceAction__event_ust', None)) @@ -231,10 +244,8 @@ def test_action_substitutions(self) -> None: shutil.rmtree(tmpdir) del os.environ['TestTraceAction__event_ust'] del os.environ['TestTraceAction__context_field'] - del os.environ['LD_PRELOAD'] def test_action_ld_preload(self) -> None: - self.assertIsNone(os.environ.get('LD_PRELOAD')) tmpdir = tempfile.mkdtemp(prefix='TestTraceAction__test_action_ld_preload') action = Trace( @@ -285,7 +296,70 @@ def test_action_ld_preload(self) -> None: self.assertTrue(any(p.endswith('liblttng-ust-dl.so') for p in paths)) shutil.rmtree(tmpdir) - del os.environ['LD_PRELOAD'] + + def test_append_timestamp(self) -> None: + tmpdir = tempfile.mkdtemp(prefix='TestTraceAction__test_append_timestamp') + + action = Trace( + session_name='my-session-name', + append_timestamp=True, + base_path=tmpdir, + events_kernel=[], + events_ust=[ + 'ros2:*', + '*', + ], + subbuffer_size_ust=524288, + subbuffer_size_kernel=1048576, + ) + self._assert_launch_no_errors([action]) + self._check_trace_action(action, tmpdir, session_name=None) + # Session name should start with the given prefix and end with the timestamp, but don't + # bother validating the timestamp here + self.assertTrue(action.session_name.startswith('my-session-name-')) + self.assertNotEqual('my-session-name-', action.session_name) + + shutil.rmtree(tmpdir) + + def test_append_trace(self) -> None: + tmpdir = tempfile.mkdtemp(prefix='TestTraceAction__test_append_trace') + + # Generate a normal trace + action = Trace( + session_name='my-session-name', + base_path=tmpdir, + append_trace=False, + events_kernel=[], + events_ust=[ + 'ros2:*', + '*', + ], + subbuffer_size_ust=524288, + subbuffer_size_kernel=1048576, + ) + self._assert_launch_no_errors([action]) + self._check_trace_action(action, tmpdir, append_trace=False) + + # Generating another trace with the same path should error out + self._assert_launch_errors([action]) + + # But it should work if we use the append_trace option + action = Trace( + session_name='my-session-name', + base_path=tmpdir, + append_trace=True, + events_kernel=[], + events_ust=[ + 'ros2:*', + '*', + ], + subbuffer_size_ust=524288, + subbuffer_size_kernel=1048576, + ) + self._assert_launch_no_errors([action]) + self._check_trace_action(action, tmpdir, append_trace=True) + + shutil.rmtree(tmpdir) if __name__ == '__main__': From 403b30603d1bc1c1aa9ae14e13db6ccea286a130 Mon Sep 17 00:00:00 2001 From: Christophe Bedard Date: Mon, 10 Apr 2023 13:42:12 -0700 Subject: [PATCH 2/2] Error out if trace already exists unless 'append trace' option is used Signed-off-by: Christophe Bedard --- tracetools_launch/tracetools_launch/action.py | 14 ++++++++++++++ tracetools_trace/tracetools_trace/tools/args.py | 3 +++ .../tracetools_trace/tools/lttng_impl.py | 11 +++++++++-- tracetools_trace/tracetools_trace/trace.py | 5 +++++ 4 files changed, 31 insertions(+), 2 deletions(-) diff --git a/tracetools_launch/tracetools_launch/action.py b/tracetools_launch/tracetools_launch/action.py index aceb7569..bc1473cf 100644 --- a/tracetools_launch/tracetools_launch/action.py +++ b/tracetools_launch/tracetools_launch/action.py @@ -109,6 +109,7 @@ def __init__( session_name: SomeSubstitutionsType, append_timestamp: bool = False, base_path: Optional[SomeSubstitutionsType] = None, + append_trace: bool = False, events_ust: Iterable[SomeSubstitutionsType] = names.DEFAULT_EVENTS_ROS, events_kernel: Iterable[SomeSubstitutionsType] = [], context_fields: @@ -135,6 +136,8 @@ def __init__( :param append_timestamp: whether to append timestamp to the session name :param base_path: the path to the base directory in which to create the session directory, or `None` for default + :param append_trace: whether to append to the trace directory if it already exists, + otherwise an error is reported :param events_ust: the list of ROS UST events to enable :param events_kernel: the list of kernel events to enable :param context_fields: the names of context fields to enable @@ -152,6 +155,7 @@ def __init__( self.__session_name = normalize_to_list_of_substitutions(session_name) self.__base_path = base_path \ if base_path is None else normalize_to_list_of_substitutions(base_path) + self.__append_trace = append_trace self.__trace_directory = None self.__events_ust = [normalize_to_list_of_substitutions(x) for x in events_ust] self.__events_kernel = [normalize_to_list_of_substitutions(x) for x in events_kernel] @@ -174,6 +178,10 @@ def session_name(self): def base_path(self): return self.__base_path + @property + def append_trace(self): + return self.__append_trace + @property def trace_directory(self): return self.__trace_directory @@ -272,6 +280,10 @@ def parse(cls, entity: Entity, parser: Parser): base_path = entity.get_attr('base-path', optional=True) if base_path: kwargs['base_path'] = parser.parse_substitution(base_path) + append_trace = entity.get_attr( + 'append-trace', data_type=bool, optional=True, can_be_str=False) + if append_trace is not None: + kwargs['append_trace'] = append_trace # Make sure to handle empty strings and replace with empty lists, # otherwise an empty string enables all events events_ust = entity.get_attr('events-ust', optional=True) @@ -402,6 +414,7 @@ def _setup(self) -> bool: self.__trace_directory = lttng.lttng_init( session_name=self.__session_name, base_path=self.__base_path, + append_trace=self.__append_trace, ros_events=self.__events_ust, kernel_events=self.__events_kernel, context_fields=self.__context_fields, @@ -436,6 +449,7 @@ def __repr__(self): 'Trace(' f'session_name={self.__session_name}, ' f'base_path={self.__base_path}, ' + f'append_trace={self.__append_trace}, ' f'trace_directory={self.__trace_directory}, ' f'events_ust={self.__events_ust}, ' f'events_kernel={self.__events_kernel}, ' diff --git a/tracetools_trace/tracetools_trace/tools/args.py b/tracetools_trace/tracetools_trace/tools/args.py index 70526554..05e41a59 100644 --- a/tracetools_trace/tracetools_trace/tools/args.py +++ b/tracetools_trace/tracetools_trace/tools/args.py @@ -76,3 +76,6 @@ def add_arguments(parser: argparse.ArgumentParser) -> None: parser.add_argument( '-l', '--list', dest='list', action='store_true', help='display lists of enabled events and context names (default: %(default)s)') + parser.add_argument( + '-a', '--append-trace', dest='append_trace', action='store_true', + help='append to trace if it already exists, otherwise error out (default: %(default)s)') diff --git a/tracetools_trace/tracetools_trace/tools/lttng_impl.py b/tracetools_trace/tracetools_trace/tools/lttng_impl.py index 9624e99d..6579efba 100644 --- a/tracetools_trace/tracetools_trace/tools/lttng_impl.py +++ b/tracetools_trace/tracetools_trace/tools/lttng_impl.py @@ -68,6 +68,7 @@ def setup( *, session_name: str, base_path: str, + append_trace: bool = False, ros_events: Union[List[str], Set[str]] = DEFAULT_EVENTS_ROS, kernel_events: Union[List[str], Set[str]] = [], context_fields: Union[List[str], Set[str], Dict[str, List[str]]] = DEFAULT_CONTEXT, @@ -89,6 +90,8 @@ def setup( :param session_name: the name of the session :param base_path: the path to the directory in which to create the tracing session directory, which will be created if needed + :param append_trace: whether to append to the trace directory if it already exists, otherwise + an error is reported :param ros_events: list of ROS events to enable :param kernel_events: list of kernel events to enable :param context_fields: the names of context fields to enable @@ -106,6 +109,11 @@ def setup( # Validate parameters if not session_name: raise RuntimeError('empty session name') + # Resolve full tracing directory path + full_path = os.path.join(base_path, session_name) + if os.path.isdir(full_path) and not append_trace: + raise RuntimeError( + f'trace directory already exists, use the append option to append to it: {full_path}') # Check if there is a session daemon running if lttng.session_daemon_alive() == 0: @@ -179,9 +187,8 @@ def setup( channel_kernel.attr.output = lttng.EVENT_MMAP events_list_kernel = _create_events(kernel_events) - # Resolve full tracing directory path and create session + # Create session # LTTng will create the parent directories if needed - full_path = os.path.join(base_path, session_name) _create_session(session_name, full_path) # Handles, channels, events diff --git a/tracetools_trace/tracetools_trace/trace.py b/tracetools_trace/tracetools_trace/trace.py index 735af201..1d0f8148 100644 --- a/tracetools_trace/tracetools_trace/trace.py +++ b/tracetools_trace/tracetools_trace/trace.py @@ -33,6 +33,7 @@ def init( *, session_name: str, base_path: Optional[str], + append_trace: bool, ros_events: List[str], kernel_events: List[str], context_fields: List[str], @@ -46,6 +47,8 @@ def init( :param session_name: the name of the session :param base_path: the path to the directory in which to create the tracing session directory, or `None` for default + :param append_trace: whether to append to the trace directory if it already exists, otherwise + an error is reported :param ros_events: list of ROS events to enable :param kernel_events: list of kernel events to enable :param context_fields: list of context fields to enable @@ -84,6 +87,7 @@ def init( trace_directory = lttng.lttng_init( session_name=session_name, base_path=base_path, + append_trace=append_trace, ros_events=ros_events, kernel_events=kernel_events, context_fields=context_fields, @@ -137,6 +141,7 @@ def trace(args: argparse.Namespace) -> int: if not init( session_name=args.session_name, base_path=args.path, + append_trace=args.append_trace, ros_events=args.events_ust, kernel_events=args.events_kernel, context_fields=args.context_fields,