Update rosbag2 filename format to index+name+timestamp #2265
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Change rosbag2 bag filename format to {counter}_{prefix}_{timestamp}.
Extract prefix from directory name by removing timestamp pattern,
and use file creation timestamp instead of directory timestamp
to avoid duplicate timestamps in filenames.
- Update sequential writer and reindexer to generate filenames with index+name+timestamp format
- Update and refactor filename testing
Signed-off-by: Daisuke Sato <[email protected]>
MichaelOrlov
requested changes
Jan 1, 2026
Contributor
MichaelOrlov
left a comment
MichaelOrlov
left a comment
There was a problem hiding this comment.
@Tiryoh Thank you for your contribution.
While the changes in the code itself are pretty much minor, I would say. However, the changes in the tests are trickier and needs to be adjusted and cleaned up.
Can you please rename and redesign the std::filesystem::path RecordFixture::get_actual_bag_file_path(int split_index = 0) const method to be get_bag_file_path_from_metadata(BagMetadata metadata, int split_index = 0). i.e. we can wait for metadata explicitly with another helper functions in tests and this API will be more clear to understand how are we getting a filename.
Also, please move it to the rosbag2_test_common in a new bag_files_helpers.hpp file as well as other bag file helper functions such as
wait_for_metadata(std::chrono::duration<float> timeout = std::chrono::seconds(5)) conststd::filesystem::path get_relative_bag_file_path(int split_index)std::filesystem::path get_compressed_bag_file_path(int split_index)void wait_for_storage_file(const std::filesystem::path & root_bag_path, std::chrono::duration<float> timeout)- etc...
Contributor
There was a problem hiding this comment.
Please declare the following helper functions as public methods of the SequentialCompressionWriterTest test fixture to check the path in multiple places, the same way, and avoid code duplication.
bool path_match_expected_regex(const std::string & path) const
{
// New filename format: {counter}_{bag_base_dir}_{timestamp}.{compressor}
// Timestamp is generated at runtime, so validate using regex
// Use static to avoid recompiling regex on each test run
static const std::string file_pattern_str =
R"(\d+_)" + bag_base_dir_ + R"(_)" +
std::string(rosbag2_cpp::writers::TIMESTAMP_PATTERN) +
R"(\.)" + std::string(DefaultTestCompressor);
static const std::regex file_pattern(file_pattern_str);
return std::regex_match(path, file_pattern);
// "Path '" << path << "' does not match expected pattern for file " << counter;
}
::testing::AssertionResult file_counter_at_start(const std::string & path, size_t counter) const
{
std::stringstream expected_prefix;
expected_prefix << counter << "_" << bag_base_dir_ << "_";
if (path.find(expected_prefix.str()) == 0) {
return ::testing::AssertionSuccess();
}
return ::testing::AssertionFailure() << "Path '" << path <<
"' does not start with expected prefix '" << expected_prefix.str() << "'";
}Expected usage:
size_t counter = 0;
for (const auto & path : intercepted_write_metadata_.relative_file_paths) {
// Verify that filename matches expected format
EXPECT_TRUE(path_match_expected_regex(path)) <<
"Path '" << path << "' does not match expected pattern for file " << counter;
// Verify that counter is at the correct position
EXPECT_TRUE(file_counter_at_start(path, counter));
counter++;
}
Comment on lines
295
to
297
| // Right now `base_folder_` is always just the folder name for where to install the bagfile. | ||
| // The name of the folder needs to be queried in case | ||
| // SequentialWriter is opened with a relative path. |
Contributor
There was a problem hiding this comment.
I think we can delete the original comment it has become irrelevant and outdated
Suggested change
| // Right now `base_folder_` is always just the folder name for where to install the bagfile. | |
| // The name of the folder needs to be queried in case | |
| // SequentialWriter is opened with a relative path. |
Comment on lines
+301
to
+337
|
|
||
| // Extract prefix from directory name by removing timestamp pattern if present | ||
| // This handles the case when --output is not specified and default timestamped directory is used | ||
| // Currently, the default timestamp format is `YYYY_MM_DD-HH_MM_SS` | ||
| std::string dir_name = fs::path(base_folder).filename().generic_string(); | ||
| // Handle edge case where filename() returns empty (e.g., base_folder is "/" or ".") | ||
| // This should not happen in practice since base_folder is validated in open(), but | ||
| // we add this check for defensive programming. | ||
| if (dir_name.empty()) { | ||
| dir_name = "rosbag2"; // Use default prefix | ||
| } | ||
| static std::regex timestamp_pattern("_" + std::string(TIMESTAMP_PATTERN) + "$"); | ||
| std::string prefix = std::regex_replace(dir_name, timestamp_pattern, ""); | ||
|
|
||
| // Generate timestamp at file creation time | ||
| // Timestamp is generated in local time. | ||
| // During DST switches the same string may occur twice. | ||
| // The sequence counter is part of the filename, so duplicates | ||
| // still remain distinguishable. | ||
| auto now = std::chrono::system_clock::now(); | ||
| auto time_t = std::chrono::system_clock::to_time_t(now); | ||
| std::tm tm_buf; | ||
| #ifdef _WIN32 | ||
| localtime_s(&tm_buf, &time_t); | ||
| #else | ||
| localtime_r(&time_t, &tm_buf); | ||
| #endif | ||
|
|
||
| std::stringstream timestamp_stream; | ||
| timestamp_stream << std::put_time(&tm_buf, "%Y_%m_%d-%H_%M_%S"); | ||
| std::string timestamp = timestamp_stream.str(); | ||
|
|
||
| // Generate filename in format {storage_count}_{prefix}_{timestamp} | ||
| // Note: Underscores are used as separators. If the prefix contains underscores, | ||
| // this creates theoretical ambiguity when parsing filenames. However, parsing is | ||
| // typically done by matching the timestamp pattern from the end, which avoids | ||
| // ambiguity in practice. |
Contributor
There was a problem hiding this comment.
Nitpick: to shorten the body and rephrase a bit comments.
Suggested change
| // Extract prefix from directory name by removing timestamp pattern if present | |
| // This handles the case when --output is not specified and default timestamped directory is used | |
| // Currently, the default timestamp format is `YYYY_MM_DD-HH_MM_SS` | |
| std::string dir_name = fs::path(base_folder).filename().generic_string(); | |
| // Handle edge case where filename() returns empty (e.g., base_folder is "/" or ".") | |
| // This should not happen in practice since base_folder is validated in open(), but | |
| // we add this check for defensive programming. | |
| if (dir_name.empty()) { | |
| dir_name = "rosbag2"; // Use default prefix | |
| } | |
| static std::regex timestamp_pattern("_" + std::string(TIMESTAMP_PATTERN) + "$"); | |
| std::string prefix = std::regex_replace(dir_name, timestamp_pattern, ""); | |
| // Generate timestamp at file creation time | |
| // Timestamp is generated in local time. | |
| // During DST switches the same string may occur twice. | |
| // The sequence counter is part of the filename, so duplicates | |
| // still remain distinguishable. | |
| auto now = std::chrono::system_clock::now(); | |
| auto time_t = std::chrono::system_clock::to_time_t(now); | |
| std::tm tm_buf; | |
| #ifdef _WIN32 | |
| localtime_s(&tm_buf, &time_t); | |
| #else | |
| localtime_r(&time_t, &tm_buf); | |
| #endif | |
| std::stringstream timestamp_stream; | |
| timestamp_stream << std::put_time(&tm_buf, "%Y_%m_%d-%H_%M_%S"); | |
| std::string timestamp = timestamp_stream.str(); | |
| // Generate filename in format {storage_count}_{prefix}_{timestamp} | |
| // Note: Underscores are used as separators. If the prefix contains underscores, | |
| // this creates theoretical ambiguity when parsing filenames. However, parsing is | |
| // typically done by matching the timestamp pattern from the end, which avoids | |
| // ambiguity in practice. | |
| // Extract prefix from directory name by removing timestamp pattern if present | |
| // This handles the case when --output is not specified and default timestamped directory is used | |
| // Currently, the default timestamp format is `YYYY_MM_DD-HH_MM_SS` | |
| std::string dir_name = fs::path(base_folder).filename().generic_string(); | |
| // Handle edge case where filename() returns empty (e.g., base_folder is "/" or ".") | |
| // This should not happen in practice since base_folder is validated in open(), but | |
| // we add this check for defensive programming. | |
| if (dir_name.empty()) { | |
| dir_name = "rosbag2"; // Use default prefix | |
| } | |
| static std::regex timestamp_pattern("_" + std::string(TIMESTAMP_PATTERN) + "$"); | |
| std::string prefix = std::regex_replace(dir_name, timestamp_pattern, ""); | |
| // Generate timestamp in local time. | |
| // Note: During DST switches the same string may occur twice. However, we're also adding the | |
| // sequence counter as part of the filename, so duplicates still remain distinguishable. | |
| auto time_t = std::chrono::system_clock::to_time_t(std::chrono::system_clock::now()); | |
| std::tm timestamp{}; | |
| #ifdef _WIN32 | |
| localtime_s(×tamp, &time_t); | |
| #else | |
| localtime_r(&time_t, ×tamp); | |
| #endif | |
| // Generate filename in format {storage_count}_{prefix}_{timestamp} | |
| // Note: Underscores are used as separators. If the prefix contains underscores, this creates | |
| // theoretical ambiguity when parsing filenames. However, parsing is typically done by matching | |
| // the timestamp pattern from the end, which avoids ambiguity in practice. | |
| std::stringstream storage_file_name; | |
| storage_file_name << storage_count << "_" << prefix << "_" << | |
| std::put_time(×tamp, "%Y_%m_%d-%H_%M_%S"); | |
| return (fs::path(base_folder) / storage_file_name.str()).generic_string(); |
Comment on lines
+92
to
+93
| std::string new_format_regex_; | ||
| std::string old_format_regex_; |
Contributor
There was a problem hiding this comment.
Nitpick: Please rename to the new{old}_file_format_regex_str_
I would also make them as a const and init right away here using rosbag2_cpp::writers::TIMESTAMP_PATTERN.
const std::string new_file_format_regex_str_ =
R"((\d+)_(.*)_)" + std::string(rosbag2_cpp::writers::TIMESTAMP_PATTERN) +
R"(\.[a-zA-Z0-9]+){1,2})";
const std::string old_file_format_regex_str_ = R"((.*)_(\d+)(\.[a-zA-Z0-9]+){1,2})";
Comment on lines
+53
to
+58
| bool matches_filename_pattern( | ||
| const std::string & filename, const std::string & prefix, | ||
| uint64_t expected_counter) | ||
| { | ||
| // Expected format: {counter}_{prefix}_{timestamp} | ||
| std::string expected_prefix = std::to_string(expected_counter) + "_" + prefix + "_"; |
Contributor
There was a problem hiding this comment.
Please rewrite matches_filename_pattern(const std::string & path, const std::string & prefix,, size_t expected_counter) to include stripping file extension to reduce boilerplate code in tests.
i.e.
// Extract filename from path (remove directory part if present)
std::string filename = fs::path(path).filename().generic_string();
// Remove extension if present (e.g., .mcap)
std::string filename_no_ext = fs::path(filename).stem().generic_string();- Also suggest using the
size_ttype for the counter to avoid type casts. - Also, please move helper functions inside the test fixture class. To have the ability to use
ASSERT_TRUEinside helper functions if needed.
| R"(\.)" + std::string(DefaultTestCompressor); | ||
| static const std::regex closed_file_pattern(closed_file_pattern_str); | ||
| static const std::string opened_file_pattern_str = | ||
| R"(\d+_)" + std::string("test_bag") + R"(_)" + |
Contributor
There was a problem hiding this comment.
Suggested change
| R"(\d+_)" + std::string("test_bag") + R"(_)" + | |
| R"(\d+_)" + bag_base_dir_ + R"(_)" + |
Comment on lines
+147
to
+154
| // Wait for metadata file to exist | ||
| const auto start_time = std::chrono::steady_clock::now(); | ||
| while (std::chrono::steady_clock::now() - start_time < std::chrono::seconds(10)) { | ||
| if (metadata_io.metadata_file_exists(bag_path)) { | ||
| break; | ||
| } | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(50)); | ||
| } |
Contributor
There was a problem hiding this comment.
Suggested change
| // Wait for metadata file to exist | |
| const auto start_time = std::chrono::steady_clock::now(); | |
| while (std::chrono::steady_clock::now() - start_time < std::chrono::seconds(10)) { | |
| if (metadata_io.metadata_file_exists(bag_path)) { | |
| break; | |
| } | |
| std::this_thread::sleep_for(std::chrono::milliseconds(50)); | |
| } | |
| wait_for_metadata(std::chrono::seconds(10)); |
Comment on lines
41
to
59
| std::string get_bag_file_name(int split_index) const | ||
| { | ||
| std::stringstream bag_file_name; | ||
| bag_file_name << get_test_name() << "_" << GetParam() << "_" << split_index; | ||
|
|
||
| return bag_file_name.str(); | ||
| } | ||
|
|
||
| fs::path get_bag_file_path(int split_index) | ||
| { | ||
| return root_bag_path_ / get_relative_bag_file_path(split_index); | ||
| } | ||
|
|
||
| fs::path get_relative_bag_file_path(int split_index) const | ||
| { | ||
| const auto storage_id = GetParam(); | ||
| return fs::path( | ||
| rosbag2_test_common::bag_filename_for_storage_id(get_bag_file_name(split_index), storage_id)); | ||
| } |
Contributor
There was a problem hiding this comment.
These helper functions are no longer needed and can be deleted to prevent incorrect usage in the future.
| << "Could not find metadata file."; | ||
| } | ||
|
|
||
| std::filesystem::path get_actual_bag_file_path(int split_index = 0) const |
Contributor
There was a problem hiding this comment.
Please rename and redesign this method to be get_bag_file_path_from_metadata(BagMetadata metadata, int split_index = 0).
Also, please move it to the rosbag2_test_common in a new bag_files_helpers.hpp file as well as other bag file helper functions such as
wait_for_metadata(std::chrono::duration<float> timeout = std::chrono::seconds(5)) conststd::filesystem::path get_relative_bag_file_path(int split_index)std::filesystem::path get_compressed_bag_file_path(int split_index)void wait_for_storage_file(const std::filesystem::path & root_bag_path, std::chrono::duration<float> timeout)- etc...
| return rosbag2_test_common::bag_filename_for_storage_id(bag_file_name.str(), storage_id); | ||
| } | ||
|
|
||
| std::filesystem::path get_actual_bag_file_path(int split_index = 0) const |
Contributor
There was a problem hiding this comment.
Please use previously suggested get_bag_file_path_from_metadata(BagMetadata metadata, int split_index = 0) instead of this duplicating function, which is going to be in the new bag_files_helpers.hpp in the rosbag2_test_common package.
Author
|
@MichaelOrlov Thank you for the code review. I will go through the comments one by one and address them accordingly. |
fujitatomoya
reviewed
Jan 6, 2026
Contributor
fujitatomoya
left a comment
fujitatomoya
left a comment
There was a problem hiding this comment.
lgtm with green CI and @MichaelOrlov 's comment resolved.
most of the changes are adjustment for tests, implementation looks good to me.
the only concern is that, breaking change for scripts. any external tooling assuming the old pattern (mybag_0.mcap) will need updates as acknowledged in the PR.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR changes the automatically generated bagfile (split) filenames produced by the sequential writer from the current
"{prefix}_{counter}"pattern to a new pattern:counter: split index (integer starting from 0, not zero-padded)prefix: derived from the bag directory name, with any default timestamp suffix removedtimestamp: local time at file creation, formatted asYYYY_MM_DD-HH_MM_SSThis change was discussed by @fujitatomoya, @MichaelOrlov, and me in [rosbag2#2198](#2198).
It makes each split file self-descriptive and chronologically traceable, improving usability for both human inspection and external tooling.
Key points / possible concerns
User-visible filename change for new recordings only
The reader behavior remains unchanged (it uses metadata)
Backward compatibility is preserved: existing bags can still be replayed and inspected
Custom scripts that assume the old pattern (e.g.,
mybag_0.mcap,mybag_1.mcap) may need to adjust globs or regexesTimestamp is generated in local time for human readability (UTC could be added later if desired)
The
reindexerwas updated to recognize the new filename pattern (including optional double extensions), and corresponding tests/fixtures were adjustedreindexerstill does not decompress file-compressed bags; compressed files must be accessible or decompressed beforehandIs this user-facing behavior change?
Yes.
When a custom name is specified (e.g.,
ros2 bag record -o mybag -a):mybag/mybag_0.mcapmybag/0_mybag_2025_11_04-12_30_20.mcapmybag/mybag_1.mcapmybag/1_mybag_2025_11_04-12_45_20.mcapThis represents the case where the user explicitly specified the output name with
-o.When no name is specified (default timestamped directory):
rosbag2_2025_09_30-09_11_30/rosbag2_2025_09_30-09_11_30_0.mcaprosbag2_2025_09_30-09_11_30/0_rosbag2_2025_09_30-09_11_30.mcaprosbag2_2025_09_30-09_11_30/rosbag2_2025_09_30-09_11_30_1.mcaprosbag2_2025_09_30-09_11_30/1_rosbag2_2025_09_30-09_12_30.mcapThis represents the default behavior when no
-oargument is provided, and the bag directory name is automatically timestamped.Impact:
Did you use Generative AI?
Yes.
Additional Information
Timestamp format:
YYYY_MM_DD-HH_MM_SS(local time)Implementation modifies
format_storage_uriwithin the sequential writer, updatesreindexerfilename parsing, and refreshes related unit/integration testsNo metadata or storage layout changes; behavior is limited to filename generation/recognition
Optionally, UTC timestamps or configurable formats could be considered in a future PR
Test resources in
rosbag2_tests/resources/*/cdr_test/metadata.yamlwere intentionally not updatedget_actual_cdr_test_filename()to retrieve actual filenames from metadata at runtime, eliminating the need to regenerate test bags for the new naming conventionManual verification
Record behavior with split service
While recording with
ros2 bag record -a, the split service was called during recording and the generated filenames were verified.Before (legacy filename pattern):
After (new filename pattern):
This confirms that split files created during an active recording session follow the new naming convention for both default timestamped directories and explicitly named bags.
Reindex behavior
Reindexing was tested using the updated
ros2 bagcommand on both legacy and new filename patterns.Legacy filename pattern:
New filename pattern:
This confirms that
ros2 bag reindexworks correctly with both legacy and new split filename patterns.