feat: Support search logs by timestamp for structured and unstructured logs. #42
Conversation
# Conflicts: # src/clp_ffi_js/ir/StreamReader.cpp # src/clp_ffi_js/ir/StreamReader.hpp
WalkthroughThe pull request introduces a new method Changes
Sequence DiagramsequenceDiagram
participant Client
participant StreamReader
participant LogEvents
Client->>StreamReader: get_log_event_idx_by_timestamp(timestamp)
StreamReader->>LogEvents: generic_get_log_event_idx_by_timestamp(log_events, timestamp)
LogEvents-->>StreamReader: return log event index
StreamReader-->>Client: return index or null
Finishing Touches
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 (
|
There was a problem hiding this comment.
For future commits, you may follow https://github.com/y-scope/clp-ffi-js?tab=readme-ov-file#linting to run the linter
|
This PR is blocking y-scope/yscope-log-viewer#152 |
…x_by_timestamp, use std::ranges::upper_bound instead of std::upper_bound
…s null when log events are empty, and return index with "best effort"
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/clp_ffi_js/ir/StreamReader.cpp(2 hunks)src/clp_ffi_js/ir/StreamReader.hpp(6 hunks)src/clp_ffi_js/ir/StructuredIrStreamReader.cpp(2 hunks)src/clp_ffi_js/ir/StructuredIrStreamReader.hpp(2 hunks)src/clp_ffi_js/ir/UnstructuredIrStreamReader.cpp(1 hunks)src/clp_ffi_js/ir/UnstructuredIrStreamReader.hpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
src/clp_ffi_js/ir/UnstructuredIrStreamReader.hpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
src/clp_ffi_js/ir/StructuredIrStreamReader.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
src/clp_ffi_js/ir/StructuredIrStreamReader.hpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
src/clp_ffi_js/ir/UnstructuredIrStreamReader.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
src/clp_ffi_js/ir/StreamReader.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
src/clp_ffi_js/ir/StreamReader.hpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
🪛 GitHub Actions: lint
src/clp_ffi_js/ir/StreamReader.hpp
[error] 65-66: Code formatting violation: Static create method declaration is not properly formatted
[error] 126-126: Code formatting violation: DecodedResultsTsType return type declaration is not properly formatted
[error] 138-139: Code formatting violation: get_log_event_index_by_timestamp method declaration is not properly formatted
[error] 201-203: Code formatting violation: requires clause and its parameters are not properly formatted
[error] 301-303: Code formatting violation: requires clause and its parameters are not properly formatted
🔇 Additional comments (10)
src/clp_ffi_js/ir/UnstructuredIrStreamReader.hpp (1)
74-75: LGTM! Method declaration is well-defined.The method signature is consistent with the base class and follows proper C++ practices with the [[nodiscard]] attribute.
src/clp_ffi_js/ir/StructuredIrStreamReader.hpp (2)
11-11: LGTM! Include directive is appropriately placed.The addition of
<clp/ir/types.hpp>provides the necessary type definitions.
78-79: LGTM! Method declaration is consistent.The method signature matches the base class and follows the same pattern as UnstructuredIrStreamReader.
src/clp_ffi_js/ir/StructuredIrStreamReader.cpp (2)
14-14: LGTM! Include directive is appropriately placed.The addition of
<clp/ir/types.hpp>provides the necessary type definitions.
151-158: LGTM! Implementation is clean and efficient.The implementation properly delegates to the generic function, promoting code reuse between different reader types.
src/clp_ffi_js/ir/UnstructuredIrStreamReader.cpp (1)
161-168: LGTM! Implementation is consistent with StructuredIrStreamReader.The implementation properly uses the generic function, ensuring consistent behaviour across different reader types.
src/clp_ffi_js/ir/StreamReader.cpp (1)
132-132: LGTM! Type registration and function binding are properly implemented.The new type registration and function binding follow the established patterns in the codebase.
Also applies to: 149-153
src/clp_ffi_js/ir/StreamReader.hpp (3)
14-14: LGTM! Include and type declaration are properly placed.The additions follow the established patterns in the codebase.
Also applies to: 33-33
128-137: LGTM! Documentation is clear and comprehensive.The documentation clearly explains the method's purpose, parameters, and return values.
313-331: LGTM! Implementation is efficient and handles edge cases properly.The implementation:
- Correctly handles empty log events
- Uses std::upper_bound for efficient binary search
- Properly handles the case when all timestamps are larger than the target
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/clp_ffi_js/ir/StreamReader.hpp (1)
312-312: Use Yoda condition for null check.According to the coding guidelines, prefer
false == <expression>over!<expression>.Apply this diff:
- if (log_events.empty()) { + if (true == log_events.empty()) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/clp_ffi_js/ir/StreamReader.hpp(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/clp_ffi_js/ir/StreamReader.hpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: lint (ubuntu-latest)
- GitHub Check: lint (macos-latest)
🔇 Additional comments (5)
src/clp_ffi_js/ir/StreamReader.hpp (5)
33-33: LGTM! Type declaration follows established pattern.The declaration of
LogEventIdxTsTypealigns with other type declarations in the file and properly integrates with Emscripten's type system.
128-137: LGTM! Well-documented method declaration.The documentation thoroughly explains all possible return scenarios and the method signature follows C++ best practices with appropriate use of [[nodiscard]].
138-139: Fix method declaration formatting.The method declaration needs to be reformatted according to the linting rules.
Apply this diff:
- [[nodiscard]] virtual auto get_log_event_index_by_timestamp(clp::ir::epoch_time_ms_t timestamp - ) -> LogEventIdxTsType = 0; + [[nodiscard]] virtual auto get_log_event_index_by_timestamp( + clp::ir::epoch_time_ms_t timestamp + ) -> LogEventIdxTsType = 0;
312-314: LGTM! Efficient implementation with proper edge case handling.The implementation efficiently uses binary search via std::upper_bound and properly handles all edge cases:
- Empty log events return null
- No exact match returns the last smaller index
- All larger timestamps return the first index
Also applies to: 315-322, 323-329
300-307: Fix requires clause formatting.The requires clause needs to be reformatted according to the linting rules.
Apply this diff:
-requires requires( - LogEventWithFilterData<LogEvent> const& event, - clp::ir::epoch_time_ms_t timestamp - ) { + requires requires(LogEventWithFilterData<LogEvent> const& event, + clp::ir::epoch_time_ms_t timestamp) {
| @@ -129,6 +129,7 @@ EMSCRIPTEN_BINDINGS(ClpStreamReader) { | |||
| "Array<[string, bigint, number, number]>" | |||
| ); | |||
| emscripten::register_type<clp_ffi_js::ir::FilteredLogEventMapTsType>("number[] | null"); | |||
| emscripten::register_type<clp_ffi_js::ir::LogEventIdxTsType>("number | null"); | |||
There was a problem hiding this comment.
Registering a type that is nullable is good for consistency with existing code like FilteredLogEventMapTsType.
That said, Embind seems to have added support for std::optional<> so we should look into converting functions that return nullable data into returning std::optional<> types.
For convenience, embind provides factory functions to register std::vector (register_vector()), std::map<K, V> (register_map()), and std::optional (register_optional()) types:
@coderabbitai create an issue to track this
There was a problem hiding this comment.
@junhaoliao I've created issue #51 to track this enhancement: #51
🔗 Analysis chain
@junhaoliao I've created a GitHub issue to track this enhancement. The issue has been assigned to you and includes details about the current state and the proposed changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Create a GitHub issue to track the conversion of nullable types to std::optional
gh issue create \
--title "refactor: Convert nullable Embind types to use std::optional" \
--body "## Background
Currently, we register nullable types in Embind using custom type definitions that map to 'T | null' in TypeScript. For example:
- \`FilteredLogEventMapTsType\` -> \`number[] | null\`
- \`LogEventIdxTsType\` -> \`number | null\`
Embind now provides built-in support for \`std::optional<T>\` through the \`register_optional()\` function, as documented [here](https://emscripten.org/docs/porting/connecting_cpp_and_javascript/embind.html).
## Proposed Changes
Convert all nullable type registrations to use \`std::optional<T>\` with \`register_optional()\`.
## References
- Discussion: ${GITHUB_SERVER_URL}/${GITHUB_REPOSITORY}/pull/42#discussion_r1905740583" \
--assignee junhaoliao
Length of output: 792
✏️ Learnings added
Learnt from: junhaoliao
PR: y-scope/clp-ffi-js#42
File: src/clp_ffi_js/ir/StreamReader.cpp:132-132
Timestamp: 2025-01-07T16:26:06.449Z
Learning: Embind supports std::optional<T> through register_optional() function, which should be preferred over custom nullable type definitions (T | null) for better type safety and consistency.
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.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
we should look into converting functions...
in a separate PR i meant, so we should create an issue to track the effort
There was a problem hiding this comment.
@junhaoliao I had already created issue #51 to track this effort in response to your previous comment. The issue will help track the conversion of nullable types to std::optional in a separate PR.
| @@ -163,7 +177,6 @@ class StreamReader { | |||
| * @tparam LogEvent | |||
| * @param log_level_filter | |||
| * @param log_events Derived class's log events. | |||
| * @param log_events | |||
There was a problem hiding this comment.
isn't it duplicated?
There was a problem hiding this comment.
Let's revert the change and fix it in another PR. I think we should also add docs for filtered_log_event_map.
Co-authored-by: Junhao Liao <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/clp_ffi_js/ir/StreamReader.hpp (1)
196-203: Move requires clause to a concept.The requires clause is duplicated. Consider moving it to a concept for better maintainability and reusability.
Create a new concept before the class definition:
template <typename T> concept HasTimestamp = requires(T const& event, clp::ir::epoch_time_ms_t timestamp) { { event.get_timestamp() } -> std::convertible_to<clp::ir::epoch_time_ms_t>; };Then simplify the template declarations:
template <typename LogEvent> requires HasTimestamp<LogEventWithFilterData<LogEvent>>Also applies to: 296-303
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/clp_ffi_js/ir/StreamReader.hpp(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/clp_ffi_js/ir/StreamReader.hpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
🪛 GitHub Actions: lint
src/clp_ffi_js/ir/StreamReader.hpp
[error] 133-133: Code formatting violation: incorrect formatting around timestamp parameter documentation
[error] 326-326: Code formatting violation: incorrect formatting in variable declaration and initialization
🔇 Additional comments (3)
src/clp_ffi_js/ir/StreamReader.hpp (3)
33-33: LGTM! Type declaration is well-placed and consistent.The new type
LogEventIdxTsTypefollows the established naming convention and is appropriately grouped with other output types.
137-138: LGTM! Method signature is well-defined.The virtual method declaration follows the class's pattern and correctly specifies its pure virtual nature.
133-133:⚠️ Potential issueFix documentation formatting.
The parameter documentation needs proper formatting according to the linting rules.
Apply this diff to fix the formatting:
- * @param timestamp The timestamp to search for, in milliseconds since the Unix epoch. + * @param timestamp + * The timestamp to search for, in milliseconds since the Unix epoch.Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Actions: lint
[error] 133-133: Code formatting violation: incorrect formatting around timestamp parameter documentation
…et_log_event_index_by_timestamp
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/clp_ffi_js/ir/StreamReader.hpp (2)
145-156: Consider adding a usage example in the documentation.The documentation is comprehensive but would benefit from a concrete example demonstrating the return values for different scenarios (exact match, approximate match, no match).
* @return null if no log event exists in the stream. + * + * @example + * For a stream with events at timestamps [100, 200, 300]: + * get_log_event_index_by_timestamp(150) returns 0 (index of timestamp 100) + * get_log_event_index_by_timestamp(200) returns 1 (exact match) + * get_log_event_index_by_timestamp(400) returns 2 (index of timestamp 300) */
297-323: Add defensive checks for index calculations.While the implementation is efficient using binary search, consider adding safety checks:
auto const upper_index{std::distance(log_events.begin(), upper)}; + // Check for potential overflow + if (upper_index == 0) { + return LogEventIdxTsType{emscripten::val::null()}; + } auto const index{upper_index - 1}; + // Validate index is within bounds + if (index >= log_events.size()) { + return LogEventIdxTsType{emscripten::val::null()}; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/clp_ffi_js/ir/StreamReader.hpp(7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/clp_ffi_js/ir/StreamReader.hpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: lint (ubuntu-latest)
- GitHub Check: lint (macos-latest)
🔇 Additional comments (3)
src/clp_ffi_js/ir/StreamReader.hpp (3)
14-14: LGTM! The new include and type declaration are well-placed.The addition of
<clp/ir/types.hpp>andLogEventIdxTsTypedeclaration aligns with the PR's objective of supporting timestamp-based search functionality.Also applies to: 33-33
49-64: Well-structured concept definitions!The introduction of
GetLogEventIdxInterfaceandDecodeRangeInterfaceconcepts improves code clarity and maintainability. The concepts effectively encapsulate the requirements for timestamp operations and string conversion respectively.
310-312: Verify timestamp comparison edge cases.The current comparison assumes timestamps are always valid. Consider verifying how the code handles:
- Negative timestamps
- Maximum timestamp values
- Timestamp overflow scenarios
✅ Verification successful
Timestamp comparison implementation is verified safe
The current implementation correctly handles all edge cases:
- Uses consistent type system across the codebase
- Preserves ordering for all possible values including zero, negative, and maximum values
- Handles missing timestamps gracefully by defaulting to zero
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for timestamp validation or handling in the codebase rg -l "epoch_time_ms_t" | xargs rg -B2 -A2 "(numeric_limits|MAX|MIN|overflow|underflow)" # Check if there are any existing test cases for timestamp edge cases fd -e cpp -e hpp test_.*timestamp | xargs rg -A 5 "TEST.*timestamp"Length of output: 160
Script:
#!/bin/bash # Find timestamp type definition and usage ast-grep --pattern 'using epoch_time_ms_t = $_' # Find timestamp comparisons ast-grep --pattern 'get_timestamp()' # Look for test files that might contain timestamp tests fd -e cpp -e hpp test_ | xargs rg -B2 -A2 "epoch_time_ms_t"Length of output: 6520
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/clp_ffi_js/ir/StreamReader.hpp (1)
202-213: Consider enhancing the documentation.The documentation could be more comprehensive. Consider adding details about:
- The algorithm used (binary search)
- Edge cases handling
- Performance characteristics
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/clp_ffi_js/ir/StreamReader.cpp(2 hunks)src/clp_ffi_js/ir/StreamReader.hpp(7 hunks)src/clp_ffi_js/ir/StructuredIrStreamReader.cpp(2 hunks)src/clp_ffi_js/ir/StructuredIrStreamReader.hpp(2 hunks)src/clp_ffi_js/ir/UnstructuredIrStreamReader.cpp(1 hunks)src/clp_ffi_js/ir/UnstructuredIrStreamReader.hpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/clp_ffi_js/ir/StructuredIrStreamReader.hpp
- src/clp_ffi_js/ir/StreamReader.cpp
- src/clp_ffi_js/ir/UnstructuredIrStreamReader.hpp
- src/clp_ffi_js/ir/UnstructuredIrStreamReader.cpp
- src/clp_ffi_js/ir/StructuredIrStreamReader.cpp
🧰 Additional context used
📓 Path-based instructions (1)
src/clp_ffi_js/ir/StreamReader.hpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: lint (ubuntu-latest)
- GitHub Check: lint (macos-latest)
🔇 Additional comments (6)
src/clp_ffi_js/ir/StreamReader.hpp (6)
14-14: LGTM! Include statement added as suggested.The addition of
clp/ir/types.hppis appropriate for using theclp::ir::epoch_time_ms_ttype.
33-33: LGTM! Type declaration follows established pattern.The declaration of
LogEventIdxTsTypealigns with the existing TypeScript type declarations in the file.
49-57: LGTM! Well-defined concept for timestamp retrieval.The concept properly constrains the LogEvent type to ensure it provides a timestamp that's convertible to
clp::ir::epoch_time_ms_t.
59-64: LGTM! Good refactoring to use a concept.Moving the requirements to a named concept improves code organization and reusability.
145-156: LGTM! Well-documented method declaration.The method declaration is properly marked as
[[nodiscard]]and includes comprehensive documentation of parameters and return values.
297-323: LGTM! Efficient implementation with proper edge case handling.The implementation uses
std::upper_boundfor efficient binary search and correctly handles all edge cases as documented.
Description
Adds
getLogEventIndexByTimestampAPI inclp_ffi_js/ir/StreamReader.cppfor ClpStreamReader. It returnsnullonly if there are no log events. Otherwise, it would search logs in "best effort", which means it returns the last index smaller than the timestamp if no exact timestamp match, unless all log event timestamps are larger than the target. In that case, return the first log event index.In
clp_ffi_js/ir/StreamReader.hpp, implementsgeneric_get_log_event_index_by_timestamp(), the core logic (basically just binary search) for finding log events by timestamp.This function is called both by
StructuredIrStreamReaderandUnstructuredIrStreamReader'sget_log_event_index_by_timestamp.Validation performed
Use the following javascript for testing structured and unstructured logs:
Summary by CodeRabbit
New Features
Technical Improvements