Add log-level filtering methods and associated refactoring: #12
Conversation
WalkthroughThe changes involve modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant StreamReader
participant LogEventWithLevel
User->>StreamReader: filter_log_events(log_level_filter)
StreamReader->>StreamReader: Populate m_filtered_log_event_map
StreamReader->>LogEventWithLevel: Create LogEventWithLevel instances
StreamReader->>User: get_filtered_log_event_map()
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (4)
src/clp_ffi_js/ir/StreamReaderDataContext.hpp (1)
Line range hint
24-31: Constructor design is good, but check deserializer typeThe constructor is well-designed, using rvalue references and
std::movefor efficient resource transfer. However, there's a potential issue:The deserializer parameter type is
clp::ir::LogEventDeserializer<clp::ir::four_byte_encoded_variable_t>, but the class template usesencoded_variable_t. This mismatch might cause compilation errors or unexpected behaviour.Consider changing the deserializer parameter type to match the class template:
- clp::ir::LogEventDeserializer<clp::ir::four_byte_encoded_variable_t> deserializer + clp::ir::LogEventDeserializer<encoded_variable_t> deserializersrc/clp_ffi_js/ir/StreamReader.hpp (2)
73-78: Ensure consistent parameter naming in commentsThe parameter names in the comments (
beginIdx,endIdx,useFilter) should match the actual parameter names in the function signature (begin_idx,end_idx,use_filter). Consistent naming enhances readability and reduces confusion.
88-94: Maintain consistent parameter naming conventionsThe parameter
logLevelFilterinfilter_log_events()uses camelCase, while other parameters use snake_case (e.g.,begin_idx,end_idx,use_filter). Consider renaming it tolog_level_filterfor consistency.src/clp_ffi_js/ir/StreamReader.cpp (1)
104-104: Reconsider resettingm_is_filteredindeserialize_rangeCurrently,
m_is_filteredis set tofalseat the beginning ofdeserialize_range, which resets the filtered state whenever deserialization occurs. If the intent is to preserve the filter state across deserializations, consider removing this reset or providing an option to retain the filter.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- src/clp_ffi_js/ir/LogViewerEvent.hpp (1 hunks)
- src/clp_ffi_js/ir/StreamReader.cpp (7 hunks)
- src/clp_ffi_js/ir/StreamReader.hpp (4 hunks)
- src/clp_ffi_js/ir/StreamReaderDataContext.hpp (1 hunks)
🔇 Additional comments (6)
src/clp_ffi_js/ir/StreamReaderDataContext.hpp (3)
Line range hint
13-21: LGTM: Well-structured class declarationThe
StreamReaderDataContextclass is well-designed:
- The template allows flexibility for different encoded variable types.
- The class encapsulates related resources, adhering to the single responsibility principle.
- The namespace is consistent with the file structure.
Also applies to: 50-54
Line range hint
33-41: LGTM: Appropriate special member function declarationsThe special member functions are well-defined:
- Deleting copy operations prevents accidental copying of potentially large resources.
- Defaulting move operations allows for efficient transfer of ownership.
- Defaulting the destructor is appropriate as there are no custom cleanup operations needed.
This approach ensures proper resource management and follows the Rule of Five.
Line range hint
46-49: LGTM: Well-designed getter methodThe
get_deserializermethod is well-implemented:
- It returns a reference, avoiding unnecessary copying.
- The
[[nodiscard]]attribute is correctly used.- The method provides controlled access to the encapsulated deserializer.
src/clp_ffi_js/ir/StreamReader.hpp (3)
15-15: Include of 'LogViewerEvent.hpp' is appropriateThe inclusion of
LogViewerEvent.hppis necessary for the use ofLogViewerEventin the class.
20-20: Declaration of 'FilteredLogEventMapType' is correctNo issues found with the declaration of
FilteredLogEventMapType.
87-87: Verify all calls to 'decode_range()' include 'use_filter' parameterThe method signature of
decode_range()has been updated to include theuse_filterparameter. Please ensure all existing calls to this method are updated to include this new parameter to prevent compilation errors or unexpected behaviour.Run the following script to identify all usages of
decode_range():✅ Verification successful
All calls to
decode_range()include theuse_filterparameter.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all calls to 'decode_range()' and check for the 'use_filter' parameter. # Expected: All calls should include three arguments, including 'use_filter'. rg --type cpp --type c++ --type c -A 2 -B 2 'decode_range\('Length of output: 90
Script:
#!/bin/bash # Description: Find all calls to 'decode_range()' and check for the 'use_filter' parameter. # Expected: All calls should include three arguments, including 'use_filter'. rg --glob '*.cpp' --glob '*.cxx' --glob '*.cc' --glob '*.c++' --glob '*.c' 'decode_range\('Length of output: 243
src/clp_ffi_js/ir/StreamReader.hpp
Outdated
| /** | ||
| * @return The number of events buffered. | ||
| */ | ||
| [[nodiscard]] auto get_filtered_log_event_map() const -> FilteredLogEventMapType; |
There was a problem hiding this comment.
Correct the documentation for 'get_filtered_log_event_map()'
The comment for get_filtered_log_event_map() incorrectly states @return The number of events buffered. Please update the documentation to accurately reflect that this function returns the filtered log event map.
src/clp_ffi_js/ir/StreamReader.hpp
Outdated
| m_stream_reader_data_context; | ||
| clp::TimestampPattern m_ts_pattern; | ||
| std::vector<LogViewerEvent<clp::ir::four_byte_encoded_variable_t>> m_encoded_log_events; | ||
| std::vector<size_t> m_filtered_log_event_map; | ||
| bool m_is_filtered; |
There was a problem hiding this comment.
Initialize 'm_is_filtered' to prevent undefined behaviour
The member variable m_is_filtered is declared but not initialized. To avoid potential undefined behaviour, initialize m_is_filtered to false in its declaration or within the constructor.
src/clp_ffi_js/ir/StreamReader.cpp
Outdated
| std::vector<int> filter_levels; | ||
| unsigned int length = logLevelFilter["length"].as<unsigned int>(); | ||
| filter_levels.reserve(length); | ||
| for (unsigned int i = 0; i < length; ++i) { | ||
| filter_levels.push_back(logLevelFilter[i].as<int>()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve performance of log level filtering by using std::unordered_set
Using std::vector<int> for filter_levels and std::find to check for log levels results in O(n) lookup time for each log event. This can be inefficient for large datasets. Consider using std::unordered_set<int> to reduce the lookup time to O(1).
Apply this diff to modify filter_levels to an unordered set and update the lookup:
-std::vector<int> filter_levels;
+std::unordered_set<int> filter_levels;
unsigned int length = logLevelFilter["length"].as<unsigned int>();
filter_levels.reserve(length);
for (unsigned int i = 0; i < length; ++i) {
- filter_levels.push_back(logLevelFilter[i].as<int>());
+ filter_levels.insert(logLevelFilter[i].as<int>());
}
-// Filter log events based on the provided log levels
for (size_t index = 0; index < m_encoded_log_events.size(); ++index) {
const auto& logEvent = m_encoded_log_events[index];
- if (std::find(filter_levels.begin(), filter_levels.end(), logEvent.get_log_level()) != filter_levels.end()) {
+ if (filter_levels.count(logEvent.get_log_level()) > 0) {
m_filtered_log_event_map.push_back(index);
}
}Also applies to: 236-241
src/clp_ffi_js/ir/StreamReader.cpp
Outdated
| constexpr size_t cLogLevelPositionInMessages{1}; | ||
| size_t log_level{cLogLevelNone}; | ||
| // NOLINTNEXTLINE(readability-qualified-auto) | ||
| auto const log_level_name_it{std::find_if( | ||
| cLogLevelNames.begin() + cValidLogLevelsBeginIdx, | ||
| cLogLevelNames.end(), | ||
| [&](std::string_view level) { | ||
| return logtype.substr(cLogLevelPositionInMessages).starts_with(level); | ||
| } | ||
| )}; | ||
| if (log_level_name_it != cLogLevelNames.end()) { | ||
| log_level = std::distance(cLogLevelNames.begin(), log_level_name_it); | ||
| } | ||
|
|
There was a problem hiding this comment.
Ensure logtype length before calling substr to prevent out-of-range access
The code assumes that logtype has more than cLogLevelPositionInMessages characters when calling substr(cLogLevelPositionInMessages). If logtype is shorter, this could result in undefined behaviour or an out-of-range access. Consider adding a check to ensure that logtype.length() > cLogLevelPositionInMessages before performing substr.
Apply this diff to add the length check:
+if (logtype.length() > cLogLevelPositionInMessages) {
auto const log_level_name_it{std::find_if(
cLogLevelNames.begin() + cValidLogLevelsBeginIdx,
cLogLevelNames.end(),
[&](std::string_view level) {
return logtype.substr(cLogLevelPositionInMessages).starts_with(level);
}
)};
if (log_level_name_it != cLogLevelNames.end()) {
log_level = std::distance(cLogLevelNames.begin(), log_level_name_it);
}
+} else {
+ log_level = cLogLevelNone;
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| constexpr size_t cLogLevelPositionInMessages{1}; | |
| size_t log_level{cLogLevelNone}; | |
| // NOLINTNEXTLINE(readability-qualified-auto) | |
| auto const log_level_name_it{std::find_if( | |
| cLogLevelNames.begin() + cValidLogLevelsBeginIdx, | |
| cLogLevelNames.end(), | |
| [&](std::string_view level) { | |
| return logtype.substr(cLogLevelPositionInMessages).starts_with(level); | |
| } | |
| )}; | |
| if (log_level_name_it != cLogLevelNames.end()) { | |
| log_level = std::distance(cLogLevelNames.begin(), log_level_name_it); | |
| } | |
| constexpr size_t cLogLevelPositionInMessages{1}; | |
| size_t log_level{cLogLevelNone}; | |
| // NOLINTNEXTLINE(readability-qualified-auto) | |
| if (logtype.length() > cLogLevelPositionInMessages) { | |
| auto const log_level_name_it{std::find_if( | |
| cLogLevelNames.begin() + cValidLogLevelsBeginIdx, | |
| cLogLevelNames.end(), | |
| [&](std::string_view level) { | |
| return logtype.substr(cLogLevelPositionInMessages).starts_with(level); | |
| } | |
| )}; | |
| if (log_level_name_it != cLogLevelNames.end()) { | |
| log_level = std::distance(cLogLevelNames.begin(), log_level_name_it); | |
| } | |
| } else { | |
| log_level = cLogLevelNone; | |
| } |
src/clp_ffi_js/ir/StreamReader.cpp
Outdated
| return; | ||
| } | ||
|
|
||
| // Convert JavaScript array to C++ vector |
There was a problem hiding this comment.
Have we tried vecFromJSArray?
There was a problem hiding this comment.
No, but I think it works. Thanks!!! This was a bit rushed so did not read any emscripten docs...
src/clp_ffi_js/ir/StreamReader.cpp
Outdated
| emscripten::val results = emscripten::val::array(); | ||
| for (size_t index : m_filtered_log_event_map) { | ||
| EM_ASM_( | ||
| { | ||
| Emval.toValue($0).push($1); | ||
| }, | ||
| results.as_handle(), | ||
| index | ||
| ); | ||
| } |
There was a problem hiding this comment.
Let's see if this works.
| emscripten::val results = emscripten::val::array(); | |
| for (size_t index : m_filtered_log_event_map) { | |
| EM_ASM_( | |
| { | |
| Emval.toValue($0).push($1); | |
| }, | |
| results.as_handle(), | |
| index | |
| ); | |
| } | |
| emscripten::val::array(m_filtered_log_event_map); |
There was a problem hiding this comment.
Thank you!!! I think it works
src/clp_ffi_js/ir/StreamReader.cpp
Outdated
| namespace { | ||
| EMSCRIPTEN_BINDINGS(ClpIrStreamReader) { | ||
| emscripten::register_type<clp_ffi_js::ir::DataArrayTsType>("Uint8Array"); | ||
| emscripten::register_type<clp_ffi_js::ir::DecodedResultsTsType>( | ||
| "Array<[string, number, number, number]>" | ||
| ); | ||
| emscripten::register_type<clp_ffi_js::ir::FilteredLogEventMapType>( | ||
| "Array<number>" |
There was a problem hiding this comment.
Let's use the "array-simple" convention when typing arrays with TypeScript: https://typescript-eslint.io/rules/array-type#array-simple
| "Array<number>" | |
| "number[]" |
src/clp_ffi_js/ir/StreamReader.cpp
Outdated
| m_encoded_log_events(), | ||
| m_filtered_log_event_map(), | ||
| m_is_filtered(false) {} |
There was a problem hiding this comment.
- We do not need to init
m_encoded_log_eventsandm_filtered_log_event_mapgiven their default constructors will be invoked. - Let's use the brace syntax for inits.
| m_encoded_log_events(), | |
| m_filtered_log_event_map(), | |
| m_is_filtered(false) {} | |
| m_is_filtered{false} {} |
|
Completed my personal review. Thanks junhao for help with emscripten! Is it ready for review |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/clp_ffi_js/ir/StreamReader.hpp (1)
56-66: LGTM: New filtering methods added with appropriate documentationThe additions of
get_filtered_log_event_map()andfilter_log_events()methods are well-documented and align with the PR objective of adding log level filtering support. The documentation issue mentioned in a previous review has been addressed.Consider adding a brief description of the return type for
get_filtered_log_event_map()to enhance clarity.src/clp_ffi_js/ir/StreamReader.cpp (1)
131-131: Style Suggestion: Placebreak;on a new line after the error log.For better readability, it's recommended to place
break;on a new line after theSPDLOG_ERRORstatement.Apply this diff to adjust the code:
- SPDLOG_ERROR("Failed to extract log type for log level parsing."); break; + SPDLOG_ERROR("Failed to extract log type for log level parsing."); + break;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/clp_ffi_js/ir/LogViewerEvent.hpp (1 hunks)
- src/clp_ffi_js/ir/StreamReader.cpp (7 hunks)
- src/clp_ffi_js/ir/StreamReader.hpp (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/clp_ffi_js/ir/LogViewerEvent.hpp
🔇 Additional comments (15)
src/clp_ffi_js/ir/StreamReader.hpp (6)
6-6: LGTM: Appropriate include addedThe addition of the header is consistent with the introduction of the
m_filtered_log_event_mapmember variable. This change supports the new filtering functionality.
16-16: LGTM: Necessary include added for LogViewerEventThe inclusion of the LogViewerEvent header file is appropriate, given the change in the type of
m_encoded_log_events. This modification supports the updated event handling in the class.
21-21: LGTM: FilteredLogEventMapType declaration addedThe declaration of FilteredLogEventMapType using EMSCRIPTEN_DECLARE_VAL_TYPE is appropriate. This addition supports the new filtering functionality and the
get_filtered_log_event_map()method.
68-74: LGTM: New build() method added with clear documentationThe addition of the
build()method, which replaces the previousdeserialize_range()method, is well-documented and aligns with the changes described in the PR summary. The method's purpose and return value are clearly explained.
77-91: LGTM: decode_range() method updated to support filteringThe
decode_range()method has been appropriately updated to include theuse_filterparameter. The documentation clearly explains the new parameter and its effect on the method's behaviour. These changes align well with the new filtering functionality.
100-103: LGTM: Member variables updated to support new functionalityThe changes to the member variables are appropriate:
- The type of
m_encoded_log_eventshas been updated toLogViewerEvent, which is consistent with the new include statement and the changes described in the PR summary.- The addition of
m_filtered_log_event_mapsupports the new filtering functionality.Note: The previous review comment about initializing
m_is_filteredis no longer applicable as this variable is not present in the current version.src/clp_ffi_js/ir/StreamReader.cpp (9)
8-8: Include<optional>header is appropriate.The inclusion of the
<optional>header is necessary for the use ofstd::optionalin the code.
29-29: IncludeLogViewerEvent.hppheader is necessary.The inclusion of
LogViewerEvent.hppis required for theLogViewerEventclass used in the code.
103-109: Implementation ofget_filtered_log_event_mapis correct.The function correctly handles the case when no filter is applied by returning
nulland returns the filtered log event map appropriately.
111-154:buildfunction is implemented correctly.The function appropriately builds and stores the encoded log events, including parsing log levels and handling potential errors.
177-206:decode_rangefunction handles filtering correctly.The function correctly decodes log events within the specified range, taking into account whether filtering is applied.
223-223: Verify the correctness oflog_event_idx + 1.In the
EM_ASMblock,log_event_idx + 1is used. Please ensure that this offset is intentional and aligns with how indices are expected in the JavaScript environment.
230-245:filter_log_eventsfunction is implemented correctly.The function appropriately filters the log events based on the provided log levels and updates the filtered event map.
263-266: Type registration forFilteredLogEventMapTypeis appropriate.The emscripten type is correctly registered as
"number[]", adhering to TypeScript array conventions.
276-279: EMSCRIPTEN bindings updated for new methods.The functions
getFilteredLogEventMap,build,decodeRange, andfilterLogEventsare correctly bound for JavaScript interoperability.
src/clp_ffi_js/ir/LogViewerEvent.hpp
Outdated
| * @tparam encoded_variable_t The type of encoded variables in the event | ||
| */ | ||
| template <typename encoded_variable_t> | ||
| class LogViewerEvent : public clp::ir::LogEvent<encoded_variable_t> { |
There was a problem hiding this comment.
Would it make more sense to name this as LogEventExt?
There was a problem hiding this comment.
(As in we might want to be less specific about the Log Viewer which is a final product. ) If done correctly, in the future we can release clp-ffi-js as a Node.js library and the extended attributes like the log level are still useful, right?
There was a problem hiding this comment.
Can we call it LogEventWithLevel? I prefer to pick a strangely specific name because eventually, once we move to the next IR format, LogEvent (which will be a set of kv-pairs) should have a field which is its level. So by picking an odd name now, I think it will hint to the reader that this code has something odd about it and they will look more into the docstring (where we can explain the situation).
src/clp_ffi_js/ir/StreamReader.cpp
Outdated
| for (size_t index = 0; index < m_encoded_log_events.size(); ++index) { | ||
| const auto& logEvent = m_encoded_log_events[index]; | ||
| if (std::find(filter_levels.begin(), filter_levels.end(), logEvent.get_log_level()) != filter_levels.end()) { | ||
| m_filtered_log_event_map->push_back(index); | ||
| } | ||
| } |
There was a problem hiding this comment.
| for (size_t index = 0; index < m_encoded_log_events.size(); ++index) { | |
| const auto& logEvent = m_encoded_log_events[index]; | |
| if (std::find(filter_levels.begin(), filter_levels.end(), logEvent.get_log_level()) != filter_levels.end()) { | |
| m_filtered_log_event_map->push_back(index); | |
| } | |
| } | |
| for (const auto& [logEventIdx, logEvent] : std::views::enumerate(m_encoded_log_events)) { | |
| if (std::ranges::find(filter_levels, logEvent.get_log_level()) != filter_levels.end()) { | |
| m_filtered_log_event_map->push_back(logEventIdx); | |
| } | |
| } |
There was a problem hiding this comment.
Reverting back to previous. I dont think most people are using c++23 yet, so not sure we want to use std::views::enumerate
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
src/clp_ffi_js/ir/StreamReader.hpp (2)
61-70: Consider enhancing the comment for filter_log_events().The new methods for filtered log events look good. However, the comment for
filter_log_events()could be more descriptive. Consider updating it to provide more context about what the method does and how it affects the internal state of the StreamReader.Suggestion:
/** * Generates a filtered collection of log events based on the provided log levels. * This method updates the internal filtered log event map. * * @param log_level_filter Array of selected log levels to filter by */
81-96: Consider improving comment formatting for better readability.The updated
decode_range()method and its documentation look good. The newuse_filterparameter is well-explained. To improve readability, consider using a bullet list for the return value description:/** * Decodes log events in the range `[beginIdx, endIdx)` of the filtered or unfiltered * (depending on the value of `useFilter`) log events collection. * * @param begin_idx * @param end_idx * @param use_filter Whether to decode from the filtered or unfiltered log events collection. * @return An array where each element is a decoded log event represented by an array of: * - The log event's message * - The log event's timestamp as milliseconds since the Unix epoch * - The log event's log level as an integer that indexes into `cLogLevelNames` * - The log event's number (1-indexed) in the stream * @return null if any log event in the range doesn't exist (e.g. the range exceeds the number * of log events in the collection). */This format makes it easier to scan and understand the return value structure.
src/clp_ffi_js/ir/StreamReader.cpp (3)
Line range hint
126-184: LGTM with suggestions: Well-implementeddeserialize_streammethodThe
deserialize_streammethod is well-structured and handles error cases appropriately. However, consider the following improvements:
- Extract the log level parsing logic (lines 155-172) into a separate method for better readability and maintainability.
- Replace magic numbers like
cLogLevelPositionInMessageswith named constants or enum values.Consider refactoring the log level extraction logic:
LogLevel parse_log_level(std::string_view logtype) { constexpr size_t cLogLevelPositionInMessages = 1; if (logtype.length() <= cLogLevelPositionInMessages) { return LogLevel::NONE; } auto log_level_substr = logtype.substr(cLogLevelPositionInMessages); auto it = std::find_if( cLogLevelNames.begin() + static_cast<size_t>(cValidLogLevelsBeginIdx), cLogLevelNames.end(), [&](std::string_view level) { return log_level_substr.starts_with(level); } ); return (it != cLogLevelNames.end()) ? static_cast<LogLevel>(std::distance(cLogLevelNames.begin(), it)) : LogLevel::NONE; }Then use it in
deserialize_stream:auto log_level = parse_log_level(logtype);
Line range hint
186-231: LGTM with a minor fix: Updateddecode_rangemethodThe updated
decode_rangemethod now handles both filtered and unfiltered log event decoding, which is a good improvement. However, there's a minor issue with the index range check.The condition
0 > begin_idxon line 198 is unnecessary sincebegin_idxis of typesize_t(unsigned). Consider updating the condition to:- if (length < end_idx || 0 > begin_idx || begin_idx > end_idx) { + if (begin_idx > end_idx || end_idx > length) {This change ensures that:
- The begin index is less than or equal to the end index.
- The end index doesn't exceed the available length.
- We don't have an unnecessary check for a negative begin index.
Line range hint
1-274: Overall assessment: Well-implemented changes with minor suggestionsThe changes to
StreamReader.cppsuccessfully implement log level filtering and improve stream deserialization. The new methods and updates to existing ones are generally well-structured and consistent. A few minor improvements have been suggested:
- Verify C++20 compatibility for
std::ranges::findusage.- Refactor log level parsing logic in
deserialize_stream.- Fix the index range check in
decode_range.These changes enhance the functionality of the
StreamReaderclass while maintaining good code quality. After addressing the suggested improvements, the code will be more robust and maintainable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/clp_ffi_js/constants.hpp (1 hunks)
- src/clp_ffi_js/ir/StreamReader.cpp (7 hunks)
- src/clp_ffi_js/ir/StreamReader.hpp (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/clp_ffi_js/constants.hpp
🧰 Additional context used
🔇 Additional comments (8)
src/clp_ffi_js/ir/StreamReader.hpp (3)
20-32: LGTM: New type declarations for filtered log events.The addition of
FilteredLogEventMapTsTypeand theFilteredLogEventsMapalias are well-documented and consistent with the new filtering functionality. Good job on providing clear comments for the new type.
72-78: LGTM: Improved method name and documentation.The renaming of
deserialize_range()todeserialize_stream()and the updated comment provide a clear description of the method's purpose and behaviour. This change improves the overall clarity of the API.
104-104: LGTM: Updated log event type to include level information.The change from
LogEventtoLogEventWithLevelform_encoded_log_eventsis consistent with the new filtering functionality. This update allows for storing log level information directly with each event, which is crucial for efficient filtering.src/clp_ffi_js/ir/StreamReader.cpp (5)
8-8: LGTM: Addition of<optional>headerThe inclusion of the
<optional>header is appropriate for the new functionality, likely used for them_filtered_log_event_mapmember.
28-28: LGTM: Inclusion ofLogEventWithLevel.hppThe addition of the
LogEventWithLevel.hppheader is consistent with the new log level filtering functionality being implemented.
102-108: LGTM: Well-implementedget_filtered_log_event_mapmethodThe new
get_filtered_log_event_mapmethod is well-implemented. It correctly handles the case when no filtering has been applied and efficiently converts the C++ vector to a JavaScript array usingemscripten::val::array.
Line range hint
254-270: LGTM: Updated EMSCRIPTEN bindingsThe EMSCRIPTEN bindings have been correctly updated to reflect the changes in the
StreamReaderclass. The new methods are properly bound, and the removeddeserializeRangebinding has been appropriately deleted. The type registration forFilteredLogEventMapTsTypecorrectly uses a union type to represent the possibility of a null value.
110-124: LGTM with a note: Well-implementedfilter_log_eventsmethodThe
filter_log_eventsmethod is well-implemented, correctly handling null input and efficiently converting the JavaScript array to a C++ vector. However, the use ofstd::ranges::findmight require C++20 support.Please verify the project's C++ standard. If it's not C++20, consider using
std::findinstead:If C++20 is not used, apply this change:
- if (std::ranges::find(filter_levels, log_event.get_log_level()) != filter_levels.end()) { + if (std::find(filter_levels.begin(), filter_levels.end(), log_event.get_log_level()) != filter_levels.end()) {
| std::unique_ptr<StreamReaderDataContext<clp::ir::four_byte_encoded_variable_t>> | ||
| m_stream_reader_data_context; | ||
| FilteredLogEventsMap m_filtered_log_event_map; |
There was a problem hiding this comment.
Consider initializing m_filtered_log_event_map.
The addition of m_filtered_log_event_map is consistent with the new filtering functionality. However, to prevent potential undefined behaviour, consider initializing this member variable:
FilteredLogEventsMap m_filtered_log_event_map{};This ensures that the optional starts in a well-defined state (containing no value) when a StreamReader object is created.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
src/clp_ffi_js/ir/StreamReader.cpp (4)
102-108: LGTM: New method for retrieving filtered log event mapThe implementation of
get_filtered_log_event_map()looks good. It correctly handles the case when no filtering has been applied by returning null.Consider using
std::movewhen returning the array to potentially avoid unnecessary copying:return FilteredLogEventMapTsType{emscripten::val::array(std::move(m_filtered_log_event_map.value()))};This change might improve performance, especially for large filtered maps.
110-124: LGTM with suggestions: New method for filtering log eventsThe implementation of
filter_log_events()looks good overall. It correctly handles null input and implements the filtering logic as expected.Consider the following optimizations:
- Use
std::unordered_setinstead ofstd::vectorforfilter_levelsto improve lookup performance from O(n) to O(1):auto filter_levels = std::unordered_set<std::uint8_t>( emscripten::vecFromJSArray<std::uint8_t>(log_level_filter).begin(), emscripten::vecFromJSArray<std::uint8_t>(log_level_filter).end() );
- Replace
std::ranges::findwithfilter_levels.contains():if (filter_levels.contains(static_cast<std::uint8_t>(log_event.get_log_level()))) {These changes could significantly improve performance for large datasets.
Also, ensure that your project is set to use C++20, as
std::ranges::findrequires it.
Line range hint
126-184: LGTM with suggestions: New method for deserializing the streamThe implementation of
deserialize_stream()looks good overall. It correctly handles various error cases and implements the deserialization logic as expected.Consider the following improvements:
- Extract the log level parsing logic into a separate method for better readability and maintainability:
LogLevel parse_log_level(std::string_view logtype) { // ... (move the existing log level parsing logic here) }
- Define constants for magic numbers:
constexpr size_t cLogLevelPositionInMessages = 1; constexpr size_t cDefaultReservedMessageLength = 512;
- Consider using
std::optional<LogLevel>for thelog_levelvariable to make it clear that it might not have a valid value.These changes would improve the code's readability and maintainability.
265-270: LGTM with a suggestion: Added new function bindingsThe addition of new function bindings for
getFilteredLogEventMap,filterLogEvents, anddeserializeStreamis correct and consistent with the new methods added to the StreamReader class.Consider reordering the bindings to match the order of the methods in the class declaration. This can improve readability and make it easier to maintain consistency between the class declaration and the bindings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/clp_ffi_js/ir/StreamReader.cpp (7 hunks)
🧰 Additional context used
🔇 Additional comments (4)
src/clp_ffi_js/ir/StreamReader.cpp (4)
Line range hint
8-28: LGTM: New includes added to support enhanced functionalityThe addition of these new includes is appropriate for the implementation of log level filtering and the restructuring of the StreamReader class. The use of suggests good practices for handling nullable values.
206-214: LGTM: Updated loop logic in decode_rangeThe changes to the loop in
decode_range()look good. The code correctly handles both filtered and unfiltered cases based on theuse_filterparameter. The logic is clear and easy to understand, making it maintainable for future developers.
Line range hint
221-231: LGTM: Updated message handling and output in decode_rangeThe changes to the message handling and EM_ASM call in
decode_range()look good. The addition oflog_event.get_log_level()to the output is consistent with the new filtering functionality and provides valuable information to the JavaScript side.
254-255: LGTM: Added registration for FilteredLogEventMapTsTypeThe addition of the registration for
FilteredLogEventMapTsTypeis correct and necessary for the Emscripten bindings. The type is properly defined as "number[] | null", which accurately represents the possible return values of theget_filtered_log_event_map()method.
|
I had to fix a small thing (you can see change in commit 7564280 ), but then tested again and all looks good. I suggest updating the version and publishing to npm? I dont know if i need permissions or something to do this? |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
src/clp_ffi_js/ir/StreamReader.cpp (2)
210-215: Avoid Unnecessary Initialization oflog_event_idxYou can eliminate the redundant initialization of
log_event_idxby declaring it within each branch:size_t log_event_idx; if (use_filter) { log_event_idx = m_filtered_log_event_map->at(i); } else { log_event_idx = i; }This approach enhances code clarity by defining variables closer to their point of use.
184-184: Simplify Resetting the Unique PointerCalling
reset(nullptr)onstd::unique_ptris redundant. You can simplify it by using:m_stream_reader_data_context.reset();This change makes the code cleaner and more idiomatic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/clp_ffi_js/ir/StreamReader.cpp (7 hunks)
🧰 Additional context used
🔇 Additional comments (3)
src/clp_ffi_js/ir/StreamReader.cpp (3)
117-117: UseLogLevelfor Type Safety and ClarityConsider using
LogLevelinstead ofstd::uint8_twhen extracting filter levels to enhance type safety and code clarity:auto filter_levels{emscripten::vecFromJSArray<LogLevel>(log_level_filter)};This change ensures consistency in the usage of
LogLevelthroughout your code.
120-122: Ensure Compatibility with Project's C++ StandardThe use of
std::ranges::findrequires C++20 support. If your project is not compiled with C++20, consider replacing it withstd::find:if (std::find(filter_levels.begin(), filter_levels.end(), log_level.get_log_level()) != filter_levels.end()) { m_filtered_log_event_map->emplace_back(log_event_idx); }This ensures compatibility with earlier C++ standards.
232-234: Verify Data Types Passed to JavaScriptEnsure that the data types passed in the
EM_ASMblock match the expected JavaScript types. Specifically, confirm thatlog_event.get_log_level()returns a value compatible with JavaScript number types and that incrementinglog_event_idx + 1aligns with your indexing scheme.
| if (length < end_idx || begin_idx > end_idx) { | ||
| return DecodedResultsTsType{emscripten::val::null()}; | ||
| } |
There was a problem hiding this comment.
Correct the Index Range Check in decode_range
The condition if (length < end_idx || begin_idx > end_idx) may lead to unexpected behaviour. To ensure proper bounds checking, consider updating the condition:
if (begin_idx >= end_idx || end_idx > length) {
return DecodedResultsTsType{emscripten::val::null()};
}This change verifies that begin_idx is less than end_idx and that end_idx does not exceed the total length, preventing potential out-of-bounds errors.
| auto StreamReader::deserialize_stream() -> size_t { | ||
| if (nullptr == m_stream_reader_data_context) { | ||
| return m_encoded_log_events.size(); | ||
| } | ||
|
|
||
| constexpr size_t cDefaultNumReservedLogEvents{500'000}; | ||
| m_encoded_log_events.reserve(cDefaultNumReservedLogEvents); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Check for Potential Memory Allocation Issues
Reserving space for 500,000 log events with m_encoded_log_events.reserve(cDefaultNumReservedLogEvents); could lead to high memory usage. Consider assessing typical usage scenarios to determine if this number is appropriate or if a smaller initial capacity would suffice.
| auto const& logtype = message.get_logtype(); | ||
| constexpr size_t cLogLevelPositionInMessages{1}; | ||
| LogLevel log_level{LogLevel::NONE}; | ||
| if (logtype.length() > cLogLevelPositionInMessages) { | ||
| // NOLINTNEXTLINE(readability-qualified-auto) | ||
| auto const log_level_name_it{std::find_if( | ||
| cLogLevelNames.begin() + static_cast<size_t>(cValidLogLevelsBeginIdx), | ||
| cLogLevelNames.end(), | ||
| [&](std::string_view level) { | ||
| return logtype.substr(cLogLevelPositionInMessages).starts_with(level); | ||
| } | ||
| )}; | ||
| if (log_level_name_it != cLogLevelNames.end()) { | ||
| log_level = static_cast<LogLevel>( | ||
| std::distance(cLogLevelNames.begin(), log_level_name_it) | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle Empty or Short logtype Correctly
In the log level extraction logic, ensure that logtype with length equal to cLogLevelPositionInMessages is handled appropriately. Currently, if logtype is exactly one character long, logtype.substr(cLogLevelPositionInMessages) results in an empty string, which may not be intended. Consider adjusting the condition:
if (logtype.length() >= cLogLevelPositionInMessages) {
// Existing code...
}This change ensures that even log types of minimal valid length are processed correctly.
davemarco
changed the title
davemarco
changed the title
Co-authored-by: kirkrodrigues <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
src/clp_ffi_js/ir/StreamReader.cpp (4)
102-108: LGTM: Newget_filtered_log_event_mapmethodThe method correctly handles both filtered and unfiltered cases, returning null when no filtering has been applied. The use of
emscripten::val::arrayis appropriate for JavaScript interoperability.Consider using
std::movewhen returningm_filtered_log_event_mapto avoid unnecessary copying:- return FilteredLogEventMapTsType{emscripten::val::array(m_filtered_log_event_map.value())}; + return FilteredLogEventMapTsType{emscripten::val::array(std::move(m_filtered_log_event_map.value()))};This change could potentially improve performance, especially for large filtered maps.
110-130: LGTM: Newfilter_log_eventsmethod with a suggestionThe method effectively implements log level filtering. The use of
std::ranges::findis a nice modern C++ feature. However, consider usingstd::unordered_setinstead ofstd::vectorforfilter_levelsto improve lookup performance:- auto filter_levels{emscripten::vecFromJSArray<std::underlying_type_t<LogLevel>>(log_level_filter - )}; + auto filter_levels{std::unordered_set<std::underlying_type_t<LogLevel>>( + emscripten::vecFromJSArray<std::underlying_type_t<LogLevel>>(log_level_filter).begin(), + emscripten::vecFromJSArray<std::underlying_type_t<LogLevel>>(log_level_filter).end() + )}; - if (std::ranges::find( - filter_levels, - clp::enum_to_underlying_type(log_event.get_log_level()) - ) - != filter_levels.end()) + if (filter_levels.count(clp::enum_to_underlying_type(log_event.get_log_level())) > 0)This change would reduce the time complexity of the level lookup from O(n) to O(1), where n is the number of filter levels.
Line range hint
132-190: LGTM: Newdeserialize_streammethod with a suggestionThe method effectively deserializes the stream and extracts log levels. Error handling is comprehensive. Consider using a lookup table for log level names to improve performance:
static const std::unordered_map<std::string_view, LogLevel> logLevelMap = { {"TRACE", LogLevel::TRACE}, {"DEBUG", LogLevel::DEBUG}, // ... other levels ... }; // In the log level extraction logic: auto it = logLevelMap.find(logtype.substr(cLogLevelPositionInMessages)); if (it != logLevelMap.end()) { log_level = it->second; }This change could potentially improve the performance of log level extraction, especially for large numbers of log events.
192-220: LGTM: Updateddecode_rangemethod with a suggestionThe changes to
decode_rangeeffectively handle both filtered and unfiltered cases. The index range check is correct. Consider using a ternary operator to simplify the log event index selection:- size_t log_event_idx{0}; - if (use_filter) { - log_event_idx = m_filtered_log_event_map->at(i); - } else { - log_event_idx = i; - } + size_t log_event_idx = use_filter ? m_filtered_log_event_map->at(i) : i;This change would make the code more concise without sacrificing readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/clp_ffi_js/ir/StreamReader.cpp (7 hunks)
🧰 Additional context used
🔇 Additional comments (3)
src/clp_ffi_js/ir/StreamReader.cpp (3)
8-8: LGTM: Addition of<optional>headerThe inclusion of the
<optional>header is appropriate, likely to support the new filtering functionality in theStreamReaderclass.
28-28: LGTM: Inclusion ofLogEventWithLevel.hppThe addition of this header is consistent with the new functionality for handling log levels in the
StreamReaderclass.
Line range hint
260-276: LGTM: Updated EMSCRIPTEN bindingsThe new bindings correctly expose the added methods to JavaScript. The type registration for
FilteredLogEventMapTsTypeas "number[] | null" is appropriate, allowing for the case when no filtering has been applied.
Can you submit a PR after this one to bump the version to 0.2.0? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/clp_ffi_js/ir/StreamReader.cpp (3)
112-132: LGTM with a suggestion: Well-implemented filtering method with room for optimization.The
filter_log_eventsmethod correctly implements the filtering logic. It properly handles the case of clearing the filter when the input is null. The use ofemscripten::vecFromJSArrayfor converting the JavaScript array to a C++ vector is appropriate.However, there's room for performance improvement:
Consider using
std::unordered_setinstead ofstd::vectorforfilter_levels. This would change the time complexity of the level lookup from O(n) to O(1), where n is the number of filter levels. Here's a suggested implementation:void StreamReader::filter_log_events(emscripten::val const& log_level_filter) { if (log_level_filter.isNull()) { m_filtered_log_event_map.reset(); return; } m_filtered_log_event_map.emplace(); auto filter_levels_vec = emscripten::vecFromJSArray<std::underlying_type_t<LogLevel>>(log_level_filter); std::unordered_set<std::underlying_type_t<LogLevel>> filter_levels(filter_levels_vec.begin(), filter_levels_vec.end()); for (size_t log_event_idx = 0; log_event_idx < m_encoded_log_events.size(); ++log_event_idx) { auto const& log_event = m_encoded_log_events[log_event_idx]; if (filter_levels.count(clp::enum_to_underlying_type(log_event.get_log_level())) > 0) { m_filtered_log_event_map->emplace_back(log_event_idx); } } }This change would be particularly beneficial for larger datasets or when there are many filter levels.
Line range hint
134-191: LGTM with a suggestion: Well-implemented deserialization method.The
deserialize_streammethod correctly implements the deserialization logic for log events. The extraction of log levels from the logtype is done carefully, considering potential edge cases.However, there's a point to consider:
The method reserves space for 500,000 log events initially with
m_encoded_log_events.reserve(cDefaultNumReservedLogEvents). While this can improve performance by reducing reallocations, it might be excessive for some use cases and could lead to unnecessary memory usage.Consider making this value configurable or adjustable based on expected input size. Alternatively, you could implement a more dynamic allocation strategy, starting with a smaller reservation and growing as needed.
For example:
constexpr size_t cInitialReservedLogEvents{10'000}; constexpr size_t cGrowthFactor{2}; size_t current_capacity = cInitialReservedLogEvents; m_encoded_log_events.reserve(current_capacity); // In the deserialization loop: if (m_encoded_log_events.size() == current_capacity) { current_capacity *= cGrowthFactor; m_encoded_log_events.reserve(current_capacity); }This approach would start with a smaller memory footprint and grow as needed, potentially being more memory-efficient for smaller log files while still providing good performance for larger ones.
194-222: LGTM with a suggestion: Good implementation of filtered decoding.The changes to the
decode_rangemethod correctly implement support for both filtered and unfiltered log events. The addition of theuse_filterparameter and the logic to handle both cases are well done.However, there's a small improvement that could be made:
The index range check could be more explicit. Consider updating the condition:
if (begin_idx >= end_idx || end_idx > length) { return DecodedResultsTsType{emscripten::val::null()}; }This change makes it clearer that:
- The begin index should be less than the end index.
- The end index should not exceed the available length.
This slight modification improves readability and makes the bounds checking more explicit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/clp_ffi_js/ir/StreamReader.cpp (6 hunks)
🧰 Additional context used
🔇 Additional comments (5)
src/clp_ffi_js/ir/StreamReader.cpp (5)
8-8: LGTM: New includes are appropriate for the added functionality.The addition of
<optional>,<type_traits>, and<clp/type_utils.hpp>headers is consistent with the new features implemented in this file. These includes support the use ofstd::optional,std::underlying_type_t, andclp::enum_to_underlying_typerespectively.Also applies to: 12-12, 23-23
104-110: LGTM: Well-implemented getter for filtered log event map.The
get_filtered_log_event_mapmethod correctly handles both cases: when a filtered map exists and when it doesn't. The use ofemscripten::val::arrayfor returning the result to JavaScript is appropriate. The method provides a clean interface for accessing the filtered log events.
Line range hint
215-241: LGTM: Excellent implementation of filtered decoding loop.The changes to the decoding loop in
decode_rangeare well-implemented. The method now correctly handles both filtered and unfiltered cases, using the appropriate index based on whether filtering is enabled. The inclusion of the log level in the output array is consistent with the new filtering functionality and provides valuable information to the caller.The logic for selecting the correct log event index is clear and concise:
size_t log_event_idx{0}; if (use_filter) { log_event_idx = m_filtered_log_event_map->at(i); } else { log_event_idx = i; }This implementation ensures that the correct log events are decoded, whether filtering is used or not.
Line range hint
262-278: LGTM: Appropriate updates to EMSCRIPTEN bindings.The changes to the EMSCRIPTEN bindings correctly expose the new functionality to JavaScript:
The registration of
FilteredLogEventMapTsTypeas "number[] | null" is appropriate, accurately representing the possible return types of theget_filtered_log_event_mapmethod.The addition of new functions to the bindings (
getFilteredLogEventMap,filterLogEvents, anddeserializeStream) correctly exposes the new methods of theStreamReaderclass to JavaScript.These updates ensure that the new filtering and deserialization capabilities are accessible from the JavaScript side, maintaining consistency between the C++ implementation and its JavaScript interface.
Line range hint
1-281: Overall assessment: Well-implemented filtering functionality with minor suggestions for improvement.The changes to
StreamReader.cppsuccessfully implement log level filtering functionality and update existing methods to support it. The new methodsget_filtered_log_event_map,filter_log_events, anddeserialize_streamare well-designed and correctly implemented. The modifications todecode_rangeappropriately handle both filtered and unfiltered cases.Key strengths:
- Correct implementation of filtering logic.
- Proper handling of edge cases (e.g., null filter input).
- Consistent updates to EMSCRIPTEN bindings.
Suggestions for improvement:
- Consider using
std::unordered_setinfilter_log_eventsfor better performance.- Evaluate the initial reservation size in
deserialize_streamfor potential memory optimization.- Make the index range check in
decode_rangemore explicit.These changes significantly enhance the functionality of the
StreamReaderclass while maintaining good code quality. The suggested improvements are minor and aimed at optimizing performance and clarity.
|
Sure I can do that. I just made your changes and tested again. All looks good |
davemarco
changed the title
Description
Adds support for log level filtering.
Validation performed
Performed some basic testing by calling functions in a test log-viewer branch. More testing will be done once integrated into log viewer.
Summary by CodeRabbit
New Features
LogViewerEventclass for enhanced log event handling, including log level information.StreamReaderclass to filter log events based on user-selected log levels.StreamReaderDataContextclass for managing stream reading and deserialization resources.Bug Fixes
StreamReaderclass.Documentation
Summary by CodeRabbit
New Features
LogLevelenumeration for defining various log levels.StreamReaderclass to filter log events based on user-selected log levels.StreamReaderclass for retrieving filtered log events and deserializing log data.Bug Fixes
StreamReaderclass.Documentation