Add LogEventWithFilterData template class to extend log event classes with processed fields for filtering.
#32
Conversation
WalkthroughThis pull request introduces a new file Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 1
🧹 Outside diff range and nitpick comments (4)
src/clp_ffi_js/ir/LogEventWithFilterData.hpp (2)
11-15: Consider enhancing class documentation.The documentation could be more comprehensive by:
- Explaining how log level filtering is facilitated
- Documenting the relationship with IR file deserialization
- Adding
@seereferences to related classes
40-44: Consider adding noexcept specifications to accessor methods.Since these accessor methods cannot throw exceptions, they should be marked as noexcept:
- [[nodiscard]] auto get_log_event() const -> LogEvent const& { return m_log_event; } + [[nodiscard]] auto get_log_event() const noexcept -> LogEvent const& { return m_log_event; } - [[nodiscard]] auto get_log_level() const -> LogLevel { return m_log_level; } + [[nodiscard]] auto get_log_level() const noexcept -> LogLevel { return m_log_level; } - [[nodiscard]] auto get_timestamp() const -> clp::ir::epoch_time_ms_t { return m_timestamp; } + [[nodiscard]] auto get_timestamp() const noexcept -> clp::ir::epoch_time_ms_t { return m_timestamp; }src/clp_ffi_js/ir/UnstructuredIrStreamReader.hpp (1)
Line range hint
15-90: Well-designed transition to template-based filteringThe shift from inheritance-based
LogEventWithLevelto template-basedLogEventWithFilterDatais a solid architectural improvement that:
- Provides better type safety through templates
- Supports both unstructured and structured log events uniformly
- Enables efficient log level filtering by parsing during deserialization
- Reduces code duplication through template specialization
Consider documenting these architectural decisions in the class documentation to help future maintainers understand the design rationale.
src/clp_ffi_js/ir/UnstructuredIrStreamReader.cpp (1)
150-155: Consider constructingLogEventWithFilterDatadirectly withinemplace_back.To improve efficiency and reduce unnecessary copies or moves, you could construct the
LogEventWithFilterDataobject directly within theemplace_backcall.Apply this diff to streamline the code:
-auto log_event_with_filter_data{LogEventWithFilterData<UnstructuredLogEvent>( - log_event, - log_level, - log_event.get_timestamp() -)}; -m_encoded_log_events.emplace_back(std::move(log_event_with_filter_data)); +m_encoded_log_events.emplace_back( + log_event, + log_level, + log_event.get_timestamp() +);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/clp_ffi_js/ir/LogEventWithFilterData.hpp(1 hunks)src/clp_ffi_js/ir/LogEventWithLevel.hpp(0 hunks)src/clp_ffi_js/ir/UnstructuredIrStreamReader.cpp(4 hunks)src/clp_ffi_js/ir/UnstructuredIrStreamReader.hpp(2 hunks)
💤 Files with no reviewable changes (1)
- src/clp_ffi_js/ir/LogEventWithLevel.hpp
🧰 Additional context used
📓 Learnings (1)
src/clp_ffi_js/ir/UnstructuredIrStreamReader.cpp (1)
Learnt from: junhaoliao
PR: y-scope/clp-ffi-js#26
File: src/clp_ffi_js/ir/IrStreamReader.cpp:58-59
Timestamp: 2024-11-04T19:35:32.602Z
Learning: In the `IrStreamReader::filter_log_events` method in `src/clp_ffi_js/ir/IrStreamReader.cpp`, the wrapper code is typed with TypeScript, ensuring that non-array inputs for `log_level_filter` would fail static checks, so additional runtime error handling is unnecessary.
🔇 Additional comments (12)
src/clp_ffi_js/ir/LogEventWithFilterData.hpp (3)
16-17: Well-designed template approach!
The template design effectively supports both unstructured and structured log events, aligning with the PR objectives.
29-38: Excellent implementation of special member functions!
The class correctly follows the Rule of Five and modern C++ best practices by:
- Explicitly deleting copy operations
- Defaulting move operations
- Declaring the destructor as default
46-50: Well-organized member variables!
The private members are logically ordered and use appropriate types.
src/clp_ffi_js/ir/UnstructuredIrStreamReader.hpp (3)
15-16: Header changes look good!
The inclusion of constants.hpp and LogEventWithFilterData.hpp properly supports the transition from LogEventWithLevel to the new templated approach.
23-23: Type alias enhances code clarity!
The UnstructuredLogEvent alias provides clear semantic meaning and aligns well with the new templated LogEventWithFilterData approach.
90-90: Verify implementation of affected methods
The change from LogEventWithLevel to LogEventWithFilterData looks correct. However, please ensure that the following methods have been properly updated in the implementation file to handle the new type:
deserialize_stream()get_filtered_log_event_map()filter_log_events()
✅ Verification successful
Let me gather more context about the implementation details.
Let me gather more context about the implementation of the filtering methods.
Based on the code analysis, I can now provide the final response.
The type change is properly handled in all affected methods
The implementation correctly handles the new LogEventWithFilterData type in all relevant methods:
deserialize_stream(): Properly constructsLogEventWithFilterDatawith log event, level and timestampfilter_log_events(): Correctly accesses log level viaget_log_level()for filteringget_filtered_log_event_map(): Returns filtered indices as expected
The change from LogEventWithLevel to LogEventWithFilterData has been consistently implemented throughout the codebase with proper handling of both log level and timestamp data.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of affected methods
# Test: Search for method implementations to ensure they handle LogEventWithFilterData
rg -A 10 "UnstructuredIrStreamReader::(deserialize_stream|get_filtered_log_event_map|filter_log_events)"
Length of output: 2867
Script:
#!/bin/bash
# Search for the implementation of deserialize_log_event to see how it handles LogEventWithFilterData
rg -A 15 "deserialize_log_event\(\)"
# Search for LogEventWithFilterData usage
rg -A 5 "LogEventWithFilterData"
# Search for any references to the old type LogEventWithLevel
rg "LogEventWithLevel"
Length of output: 5609
Script:
#!/bin/bash
# Search for the complete implementation of filter_log_events to see how it handles the new type
rg -A 30 "void UnstructuredIrStreamReader::filter_log_events"
# Search for the complete implementation of get_filtered_log_event_map
rg -A 30 "UnstructuredIrStreamReader::get_filtered_log_event_map"
# Look for any timestamp filtering related code
rg -A 10 "timestamp" src/clp_ffi_js/ir/
Length of output: 24833
src/clp_ffi_js/ir/UnstructuredIrStreamReader.cpp (6)
29-29: Included necessary header for LogEventWithFilterData.
The inclusion of LogEventWithFilterData.hpp ensures that the LogEventWithFilterData template class is available for use in this file.
189-191: Accessing log_level and timestamp appropriately.
Retrieving log_level and timestamp from log_event_with_filter_data maintains clarity and ensures that the data associated with each log event is accurately accessed.
193-195: Decoding the unstructured log event message correctly.
By obtaining the unstructured_log_event from log_event_with_filter_data and decoding its message, the implementation correctly processes the unstructured log data.
206-206: Inserting formatted timestamp into the timestamp pattern.
The call to m_ts_pattern.insert_formatted_timestamp(timestamp, message); ensures that the timestamp is correctly formatted and inserted into the log message.
Line range hint 72-78: Consider removing unnecessary runtime null check for log_level_filter.
Given that the wrapper code is typed with TypeScript, which enforces that log_level_filter is not null, the runtime null check might be redundant.
As per the learning from PR y-scope/clp-ffi-js#26, you can safely remove the null check for log_level_filter to simplify the code:
-void UnstructuredIrStreamReader::filter_log_events(LogLevelFilterTsType const& log_level_filter) {
- if (log_level_filter.isNull()) {
- m_filtered_log_event_map.reset();
- return;
- }
+void UnstructuredIrStreamReader::filter_log_events(LogLevelFilterTsType const& log_level_filter) {
m_filtered_log_event_map.emplace();
// Rest of the code...212-213: Verify the types when passing timestamp and log_level to JavaScript.
When interfacing with JavaScript through EM_ASM, it's important to ensure that timestamp and log_level are of types compatible with the JavaScript environment to prevent potential runtime issues.
Run the following script to confirm that timestamp and log_level are of appropriate integral types:
This script checks for the declarations of timestamp and log_level to ensure they match expected integral types compatible with JavaScript number type.
✅ Verification successful
Types are correctly handled when passing to JavaScript
The verification shows that:
timestampis of typeclp::ir::epoch_time_ms_twhich is compatible with JavaScript's number type for timestampslog_levelis an enum class with underlying typestd::uint8_twhich safely converts to JavaScript's number type- The values are passed correctly through
EM_ASMmacro in thedecode_rangemethod
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the types of `timestamp` and `log_level` in the codebase.
# Expected: `timestamp` should be of type `int64_t` or similar.
# `log_level` should be of an integral type representing log levels.
ast-grep --lang cpp --pattern '$T timestamp = $_;' src/ \
| grep -E 'int64_t|std::int64_t|long long'
ast-grep --lang cpp --pattern '$T log_level = $_;' src/ \
| grep -E 'int|LogLevel|std::underlying_type_t<LogLevel>'
Length of output: 216
Script:
#!/bin/bash
# Let's try a different approach to find the type declarations
# First, let's find the relevant file content to see the context
cat src/clp_ffi_js/ir/UnstructuredIrStreamReader.cpp
# Also search for any enum or type definitions related to log level
rg -A 5 "enum.*LogLevel|typedef.*LogLevel" --type cpp
# Look for timestamp related declarations
rg "timestamp" --type cpp -B 2 -A 2
Length of output: 20772
| explicit LogEventWithFilterData( | ||
| LogEvent log_event, | ||
| LogLevel log_level, | ||
| clp::ir::epoch_time_ms_t timestamp | ||
| ) | ||
| : m_log_event{std::move(log_event)}, | ||
| m_log_level{log_level}, | ||
| m_timestamp{timestamp} {} |
There was a problem hiding this comment.
💡 Codebase verification
Parameter validation is needed for LogLevel
The LogLevel enum shows that NONE (0) is explicitly marked as not being a valid level. Since the constructor accepts any LogLevel value, it should validate against this invalid case.
- Add validation in
LogEventWithFilterDataconstructor to ensurelog_level != LogLevel::NONE - Consider adding a reasonable minimum bound check for the timestamp (e.g., after Unix epoch start)
Example:
if (log_level == LogLevel::NONE) {
throw std::invalid_argument("Invalid log level: NONE");
}
if (timestamp < 0) {
throw std::invalid_argument("Timestamp must be non-negative");
}🔗 Analysis chain
Consider adding parameter validation.
While the constructor implementation is efficient, consider validating:
- The log level value for validity
- The timestamp for reasonable bounds
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if LogLevel validation exists elsewhere
rg -A 5 "LogLevel.*valid|valid.*LogLevel"
Length of output: 43
Script:
#!/bin/bash
# Let's check the LogLevel enum definition and usage
rg -A 5 "enum.*LogLevel"
# Also check for any validation functions related to logging
rg -A 5 "isValid.*Log|validate.*Log"
# Look for timestamp validation patterns
rg -A 5 "timestamp.*valid|valid.*timestamp"
# Check the header file for LogLevel definition
fd -e hpp -e h "LogLevel" --exec cat {}
Length of output: 454
There was a problem hiding this comment.
No this is not true for log level
There was a problem hiding this comment.
@davemarco, understood. Thank you for the clarification.
🔗 Analysis chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the definition of LogLevel enum
fd -e hpp -e h "LogLevel" --exec cat {}
# Search for usage of LogLevel::NONE in the codebase
rg -A 5 "LogLevel::NONE"
# Check if there are any validation functions for LogLevel
rg -A 5 "isValidLogLevel|validateLogLevel"
# Look for default assignments of LogLevel::NONE
rg -A 5 "LogLevel.*=.*LogLevel::NONE"
Length of output: 1208
✏️ Learnings added
Learnt from: davemarco
PR: y-scope/clp-ffi-js#32
File: src/clp_ffi_js/ir/LogEventWithFilterData.hpp:0-0
Timestamp: 2024-11-10T02:14:19.107Z
Learning: In the context of `LogEventWithFilterData`, it is acceptable for `log_level` to be `LogLevel::NONE`; additional validation against `LogLevel::NONE` is not necessary.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| * for the log level and timestamp. The additional members facilitate log level filtering. | ||
| * @tparam LogEvent The type of the log event. | ||
| */ | ||
| template <typename LogEvent> |
There was a problem hiding this comment.
If I'm not wrong, all template parameters should following naming convention type_name_t.
cc @LinZhihao-723 / @kirkrodrigues please help confirm.
There was a problem hiding this comment.
Only primitive template parameters are named following the format type_name_t. Non-primitives are named how Marco has them.
There was a problem hiding this comment.
A little adding on:
Since we're using C++20, we should use requires or define a concept about the accepted LogEvent type
There was a problem hiding this comment.
@davemarco caution: i could be mistaken but i remember having issues with Emscripten + Ninja + C++20 features when we initialize this project, and not finding the concepts headers were one of the symptoms. The mitigation to use Unix Makefiles was removed in #19 . You may try adding the mitigation back if you see odd issues compiling the code.
There was a problem hiding this comment.
I dont know much about concepts but if similiar to golang interface, im not sure we want here. The log events types that this class is accepting do not conform to an any interface. This class is really just to stick extra members on a random type, without using inheritance.
There was a problem hiding this comment.
There was a problem hiding this comment.
Although the types that this class will accept don't need to conform to any interface, the description I suggested does indicate that they need to be log events with fields. Not all types (e.g. int64_t) would fit that description (unless we're using a very loose definition), so in that case, using a concept/requires statement to restrict the types to the existing LogEvent types would be beneficial.
Let me know if you disagree.
There was a problem hiding this comment.
I added something to do this.
Co-authored-by: Junhao Liao <[email protected]>
| namespace clp_ffi_js::ir { | ||
| /** | ||
| * A class which accepts a log event type as a template parameter and provides additional members | ||
| * for the log level and timestamp. The additional members facilitate log level filtering. |
There was a problem hiding this comment.
I understand we are including the timestamp in this class because we want to enable timestamp parsing for the StructuredStreamReader; however, the intention of such is not very explicit in this PR.
Shall we say
A templated class that accepts a log event type and extends it with additional members for log level and timestamp. These members enable efficient filtering by log level and timestamp, enhancing flexibility in log processing.
There was a problem hiding this comment.
how about
A templated class that accepts a log event type and extends it with additional members for log level and timestamp. These members allow the
streamReaderto be more efficient since there is no need to "reparse" these values on every call tofilterLogEvents()anddecodeRange().
There was a problem hiding this comment.
All good except I think you meant to capitalize StreamReader.
There was a problem hiding this comment.
Sorry I just read the latest code. I think that's good enough. (Kirk might have better ideas about improving that though
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/clp_ffi_js/ir/UnstructuredIrStreamReader.cpp (1)
188-194: Consider enhancing error message detailsThe access pattern for LogEventWithFilterData is correct and well-structured. However, the error message for failed message decoding could be more informative.
Consider enhancing the error message to include more context:
- "Failed to decode message" + std::format("Failed to decode message at index {}", log_event_idx)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/clp_ffi_js/ir/LogEventWithFilterData.hpp(1 hunks)src/clp_ffi_js/ir/UnstructuredIrStreamReader.cpp(4 hunks)src/clp_ffi_js/ir/UnstructuredIrStreamReader.hpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/clp_ffi_js/ir/LogEventWithFilterData.hpp
🧰 Additional context used
📓 Learnings (1)
src/clp_ffi_js/ir/UnstructuredIrStreamReader.cpp (1)
Learnt from: junhaoliao
PR: y-scope/clp-ffi-js#26
File: src/clp_ffi_js/ir/IrStreamReader.cpp:58-59
Timestamp: 2024-11-04T19:35:32.602Z
Learning: In the `IrStreamReader::filter_log_events` method in `src/clp_ffi_js/ir/IrStreamReader.cpp`, the wrapper code is typed with TypeScript, ensuring that non-array inputs for `log_level_filter` would fail static checks, so additional runtime error handling is unnecessary.
🔇 Additional comments (7)
src/clp_ffi_js/ir/UnstructuredIrStreamReader.cpp (3)
28-28: LGTM: Include statement properly added
The include statement for LogEventWithFilterData.hpp is correctly placed with other project includes.
211-212: LGTM: Parameter handling is correct
The timestamp and log level parameters are properly extracted from LogEventWithFilterData and passed to the JavaScript layer in the correct order.
149-154: Verify log level parsing coverage
The construction of LogEventWithFilterData properly encapsulates the log event, level, and timestamp. The approach of parsing log levels during deserialization is efficient and reduces redundancy.
Let's verify the log level parsing coverage:
✅ Verification successful
Let me gather more information about the log level parsing implementation to ensure complete coverage.
Let me gather one final piece of information about the log level names and enum definition.
Let me correct the file type flags and try again.
Log level parsing implementation is consistent and complete
The log level parsing implementation in UnstructuredIrStreamReader.cpp is properly synchronized with the centralized LogLevel enum and cLogLevelNames array defined in constants.hpp. The implementation:
- Correctly handles all defined log levels (NONE, TRACE, DEBUG, INFO, WARN)
- Uses a consistent parsing approach with proper validation
- Maintains synchronization between enum values and their string representations
- Defaults to LogLevel::NONE for unrecognized levels
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent log level parsing across the codebase
# Look for other log level parsing implementations that might need consolidation
# Search for log level parsing patterns
rg -A 3 "LogLevel::" --type cpp
Length of output: 945
Script:
#!/bin/bash
# Check the LogLevel enum definition and its usage
ast-grep --pattern 'enum $_ LogLevel {
$$$
}'
# Search for log level string mappings and parsing logic
rg -B 2 -A 5 "log_level" --type cpp
Length of output: 10724
Script:
#!/bin/bash
# Check the log level names and enum definitions
rg -B 2 -A 5 "cLogLevelNames" --type cpp --type hpp
# Look for any log level validation or conversion utilities
rg -B 2 -A 5 "enum.*LogLevel" --type hpp
Length of output: 150
Script:
#!/bin/bash
# Check the log level names and enum definitions
rg -B 2 -A 5 "cLogLevelNames" --type cpp
# Look for any log level validation or conversion utilities
rg -B 2 -A 5 "enum.*LogLevel"
Length of output: 3238
src/clp_ffi_js/ir/UnstructuredIrStreamReader.hpp (4)
10-14: Headers added appropriately
The inclusion of necessary header files ensures that all dependencies are correctly met for the updated implementation.
16-18: Include new headers for LogEventWithFilterData and associated classes
The added includes are appropriate and necessary for the use of LogEventWithFilterData, StreamReader, and StreamReaderDataContext.
23-23: Introduction of type alias UnstructuredLogEvent
Defining UnstructuredLogEvent enhances code readability and maintainability by providing a clear alias for the complex template type.
90-93: Update member variable m_encoded_log_events to use LogEventWithFilterData
The change in the type of m_encoded_log_events to std::vector<LogEventWithFilterData<UnstructuredLogEvent>> correctly reflects the new data structure for log events with filter data.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/clp_ffi_js/ir/UnstructuredIrStreamReader.cpp (1)
150-155: Consider future filtering optimizations.While the current implementation efficiently stores parsed log levels, consider these future optimizations:
- Pre-sorting log events by level during deserialization to optimize filtering operations
- Using a bitmap or similar data structure for faster level matching in
filter_log_eventsAlso applies to: 189-194
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/clp_ffi_js/ir/UnstructuredIrStreamReader.cpp(4 hunks)
🧰 Additional context used
📓 Learnings (1)
src/clp_ffi_js/ir/UnstructuredIrStreamReader.cpp (1)
Learnt from: junhaoliao
PR: y-scope/clp-ffi-js#26
File: src/clp_ffi_js/ir/IrStreamReader.cpp:58-59
Timestamp: 2024-11-04T19:35:32.602Z
Learning: In the `IrStreamReader::filter_log_events` method in `src/clp_ffi_js/ir/IrStreamReader.cpp`, the wrapper code is typed with TypeScript, ensuring that non-array inputs for `log_level_filter` would fail static checks, so additional runtime error handling is unnecessary.
🔇 Additional comments (3)
src/clp_ffi_js/ir/UnstructuredIrStreamReader.cpp (3)
29-29: LGTM: Header changes are appropriate.
The inclusion of LogEventWithFilterData.hpp aligns with the PR objectives to replace LogEventWithLevel with the new templated class.
189-194: LGTM: Clean separation of log event data and filtering metadata.
The changes properly utilize the new LogEventWithFilterData structure, maintaining type safety while providing better organization of log event data and its associated filtering metadata.
Also applies to: 212-213
150-155: LGTM: Efficient log event construction with parsed data.
The construction of LogEventWithFilterData properly stores the parsed log level and timestamp during deserialization, which avoids redundant parsing later. This aligns with the PR objectives.
Let's verify the log level parsing logic is consistent across the codebase:
✅ Verification successful
The search results show that the log level parsing logic is centralized in UnstructuredIrStreamReader.cpp and uses the cLogLevelNames array defined in constants.hpp. Let me verify the complete log level parsing implementation to ensure it's consistent.
Log level parsing implementation is consistent and properly centralized
The verification confirms that:
- Log level parsing is exclusively implemented in
UnstructuredIrStreamReader.cpp - The
cLogLevelNamesarray inconstants.hppis the single source of truth for log level strings - Other components like
StructuredIrStreamReader.cppuse theLogLevelenum directly without parsing - The implementation properly uses the
cValidLogLevelsBeginIdxconstant to skip the "NONE" level during parsing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent log level parsing patterns
# Look for other files that might be parsing log levels to ensure consistency
# Search for log level parsing patterns
rg -A 5 "logtype\.substr.*starts_with"
# Search for other uses of cLogLevelNames
rg -A 5 "cLogLevelNames"
Length of output: 2965
Script:
#!/bin/bash
# Check for any other log level parsing implementations
ast-grep --pattern 'LogLevel $name($$$) {
$$$
}'
# Look for other uses of LogLevel enum
rg -A 3 "LogLevel::"
# Check if there are any other files parsing log levels
rg -l "logtype.*level" --type cpp
Length of output: 1072
| m_log_level{log_level}, | ||
| m_timestamp{timestamp} {} | ||
|
|
||
| // Disable copy constructor and assignment operator |
There was a problem hiding this comment.
The key value pair log event has it. So felt it made sense to have it here. It also warns people if they are trying to copy it when it can be moved.
There was a problem hiding this comment.
The key value pair log event has it.
Yeah, I disagree with that decision, lol. I will follow up on that separately.
So felt it made sense to have it here.
Fair enough. I will come back to this once we resolve the issue with KeyValuePairLogEvent.
It also warns people if they are trying to copy it when it can be moved.
I think that argument is a bit of a slippery slope unless you have a type that is truly too massive to be copied. Yes, we want users to write high-performance code, but trying to force them to do that by disabling copying just hurts users who really want a copy of an object. That user may then want to add a clone member to your class at which point we're just adding syntax sugar for the sake of preventing code that might perform poorly if the compiler doesn't already optimize it.
There was a problem hiding this comment.
Okay makes sense, but left it as is for now until we update KeyValuePairLogEvent
| * for the log level and timestamp. The additional members facilitate log level filtering. | ||
| * @tparam LogEvent The type of the log event. | ||
| */ | ||
| template <typename LogEvent> |
There was a problem hiding this comment.
Although the types that this class will accept don't need to conform to any interface, the description I suggested does indicate that they need to be log events with fields. Not all types (e.g. int64_t) would fit that description (unless we're using a very loose definition), so in that case, using a concept/requires statement to restrict the types to the existing LogEvent types would be beneficial.
Let me know if you disagree.
| auto log_event_with_filter_data{LogEventWithFilterData<UnstructuredLogEvent>( | ||
| log_event, | ||
| log_level, | ||
| log_event.get_timestamp() | ||
| )}; | ||
| m_encoded_log_events.emplace_back(std::move(log_viewer_event)); | ||
| m_encoded_log_events.emplace_back(std::move(log_event_with_filter_data)); |
There was a problem hiding this comment.
As the rabbit suggested, we could emplace directly?
There was a problem hiding this comment.
I am not an expert here, but this guy (and maybe he isnt either) warns against. I also removed and it looked like clangd was trying to warn me it was using a non-existant constructor (maybe the one i deleted)
There was a problem hiding this comment.
If I'm not getting Kirk's comment wrong, I think he meant we could get rid of the separated variable declaration and directly write:
m_encoded_log_events.emplace_back(
log_event,
log_level,
log_event.get_timestamp()
);There was a problem hiding this comment.
Yeah, that's what I mean.
There was a problem hiding this comment.
okay oops changed!
Co-authored-by: kirkrodrigues <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/clp_ffi_js/ir/LogEventWithFilterData.hpp (1)
32-58: Consider adding parameter documentation to the constructor.The implementation is solid with good use of move semantics and proper memory management. Consider adding parameter documentation to clarify the expected values and units for
timestamp.Add documentation like this:
/** * Constructs a LogEventWithFilterData object. * @param log_event The log event to store * @param log_level The log level of the event * @param timestamp The timestamp in milliseconds since epoch */src/clp_ffi_js/ir/UnstructuredIrStreamReader.cpp (1)
150-150: Consider using direct emplacement for better readability.As discussed in previous reviews, consider using direct emplacement to make the code more concise:
- m_encoded_log_events.emplace_back(log_event, log_level, log_event.get_timestamp()); + m_encoded_log_events.emplace_back( + log_event, + log_level, + log_event.get_timestamp() + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/clp_ffi_js/ir/LogEventWithFilterData.hpp(1 hunks)src/clp_ffi_js/ir/UnstructuredIrStreamReader.cpp(4 hunks)src/clp_ffi_js/ir/UnstructuredIrStreamReader.hpp(2 hunks)
🧰 Additional context used
📓 Learnings (2)
src/clp_ffi_js/ir/LogEventWithFilterData.hpp (1)
Learnt from: davemarco
PR: y-scope/clp-ffi-js#32
File: src/clp_ffi_js/ir/LogEventWithFilterData.hpp:0-0
Timestamp: 2024-11-10T02:14:19.411Z
Learning: In the context of `LogEventWithFilterData`, it is acceptable for `log_level` to be `LogLevel::NONE`; additional validation against `LogLevel::NONE` is not necessary.
src/clp_ffi_js/ir/UnstructuredIrStreamReader.cpp (1)
Learnt from: junhaoliao
PR: y-scope/clp-ffi-js#26
File: src/clp_ffi_js/ir/IrStreamReader.cpp:58-59
Timestamp: 2024-11-04T19:35:32.602Z
Learning: In the `IrStreamReader::filter_log_events` method in `src/clp_ffi_js/ir/IrStreamReader.cpp`, the wrapper code is typed with TypeScript, ensuring that non-array inputs for `log_level_filter` would fail static checks, so additional runtime error handling is unnecessary.
🔇 Additional comments (11)
src/clp_ffi_js/ir/LogEventWithFilterData.hpp (6)
1-11: LGTM! File structure and includes are well-organized.
The include guards are properly defined and the headers are organized logically (standard library, external dependencies, and local includes).
12-15: LGTM! Namespace and type aliases are well-defined.
21-27: LGTM! Documentation is clear and comprehensive.
28-30: LGTM! Template constraints are properly implemented.
The use of concepts to constrain the template parameter is a good practice for type safety.
59-63: LGTM! Private members are well-organized.
16-20: Consider tracking the TODO for structured log support.
The concept currently only accepts UnstructuredLogEvent, but there's a TODO indicating future extension for structured logs.
Would you like me to create a GitHub issue to track this TODO?
Let's check for any structured log types that might need to be included:
✅ Verification successful
Let me search for any structured log related types or implementations to ensure we have a complete picture.
TODO for structured log support is valid and should be tracked
The codebase currently only implements unstructured log handling, with no existing structured log types. The TODO accurately reflects a future enhancement to extend the ValidLogEventTypes concept when structured log support is added.
- The TODO should be tracked as it represents a planned feature extension
- Current implementation in
UnstructuredIrStreamReaderis focused solely on unstructured logs - No structured log types are currently defined in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for structured log types in the codebase
rg -A 5 "StructuredLog|structured.*log"
Length of output: 3326
Script:
#!/bin/bash
# Search for any structured log related types or implementations
ast-grep --pattern 'struct $_StructuredLog$_ {
$$$
}'
# Also check for any other files that might contain structured log related code
rg -l "struct.*log|class.*log" --type cpp
Length of output: 244
src/clp_ffi_js/ir/UnstructuredIrStreamReader.hpp (2)
15-15: LGTM: Header inclusion changes align with the new architecture.
The replacement of LogEventWithLevel.hpp with LogEventWithFilterData.hpp is consistent with the architectural changes described in the PR objectives.
88-88: Verify the impact of the type change for m_encoded_log_events.
The change from LogEventWithLevel<four_byte_encoded_variable_t> to LogEventWithFilterData<UnstructuredLogEvent> looks correct, but let's verify all usage points to ensure compatibility.
✅ Verification successful
Type change for m_encoded_log_events is properly implemented and compatible
The type change from LogEventWithLevel to LogEventWithFilterData is verified to be safe and consistent throughout the codebase:
- All usages of
m_encoded_log_eventsin UnstructuredIrStreamReader.cpp are compatible with the new type - The new
LogEventWithFilterDataclass provides the same essential interfaces (get_log_level, get_timestamp) that were available in the old type - No other components directly access this member variable outside of UnstructuredIrStreamReader
- All access patterns (size(), emplace_back, indexing) remain unchanged and type-safe
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for all usages of m_encoded_log_events to verify compatibility
# with the new type LogEventWithFilterData<UnstructuredLogEvent>
echo "Searching for m_encoded_log_events usage..."
rg -p "m_encoded_log_events" --type cpp
# Search for potential method calls that might be affected by the type change
echo "Searching for methods that might need updates..."
ast-grep --pattern 'LogEventWithLevel<$_>' --type cpp
Length of output: 1277
Script:
#!/bin/bash
# Let's search for both old and new type usages with proper context
echo "Searching for LogEventWithLevel usage..."
rg -p "LogEventWithLevel" -B 2 -A 2 --type cpp
echo "Searching for LogEventWithFilterData usage..."
rg -p "LogEventWithFilterData" -B 2 -A 2 --type cpp
# Search for any potential consumers of m_encoded_log_events
echo "Searching for methods accessing m_encoded_log_events..."
rg -p "m_encoded_log_events\[|m_encoded_log_events\." -B 5 -A 5 --type cpp
Length of output: 5272
src/clp_ffi_js/ir/UnstructuredIrStreamReader.cpp (3)
29-29: LGTM! Include statement properly added.
The addition of LogEventWithFilterData.hpp and removal of LogEventWithLevel.hpp aligns with the PR objective.
184-187: LGTM! Clear and well-structured data extraction.
The code effectively extracts components from LogEventWithFilterData with clear variable names, improving readability.
206-207: LGTM! Proper usage of extracted values.
The timestamp and log level are correctly used in the EM_ASM call, maintaining consistency with the JavaScript array construction.
| // Concept defining valid log event types. | ||
| // TODO: Extend valid log event types when filtering support is added for structured logs. | ||
| template <typename T> | ||
| concept ValidLogEventTypes = std::same_as<T, UnstructuredLogEvent>; |
There was a problem hiding this comment.
Why not inline the concept into the requires statement? We don't have formal guidelines on using concepts yet, but I feel like we can avoid creating a concept for cases like this.
There was a problem hiding this comment.
Okay I made it inline
| m_log_level{log_level}, | ||
| m_timestamp{timestamp} {} | ||
|
|
||
| // Disable copy constructor and assignment operator |
There was a problem hiding this comment.
The key value pair log event has it.
Yeah, I disagree with that decision, lol. I will follow up on that separately.
So felt it made sense to have it here.
Fair enough. I will come back to this once we resolve the issue with KeyValuePairLogEvent.
It also warns people if they are trying to copy it when it can be moved.
I think that argument is a bit of a slippery slope unless you have a type that is truly too massive to be copied. Yes, we want users to write high-performance code, but trying to force them to do that by disabling copying just hurts users who really want a copy of an object. That user may then want to add a clone member to your class at which point we're just adding syntax sugar for the sake of preventing code that might perform poorly if the compiler doesn't already optimize it.
davemarco
changed the title
LogEventWithFilterData which accepts a log event type as a template parameter to prepare for log level filtering for structured log events.LogEventWithFilterData template class to extend log event classes with processed fields for filtering.
Description
Replace
LogEventWithLevelwithLogEventWithFilterData.LogEventWithFilterDatais similar toLogEventWithLevel, however it uses a template instead of inheritance; therefore, it can be used for both unstructured and structured log events. The idea for this class is that it will store the parsed log level and timestamp while the IR file is being deserialized for both unstructured and structured IR streams.The log level must be parsed for structured streams since the log level value may be invalid (i.e. the value is not a known log level string).
While we could parse and validate the structured log level during filtering and decoding, it is less repetitive to do it only once when the stream is being deserialized.
Validation performed
Tested with modified log viewer. Unstructured still works
Summary by CodeRabbit
Summary by CodeRabbit
New Features
LogEventWithFilterData, enhancing log level filtering capabilities.Bug Fixes
UnstructuredIrStreamReaderto ensure compatibility with the new log event structure.Chores
LogEventWithLevelclass to streamline the codebase.