-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Support search logs by timestamp for structured and unstructured logs. #42
base: main
Are you sure you want to change the base?
Changes from all commits
ea23947
7edaa48
7481573
3ee05a2
f1a71a1
5263385
c423ec5
467e998
c66cb80
89338f7
a99ec2a
9ec039f
4f125b8
5221588
abeb4f8
6647fbd
046191e
4c7386d
c400361
ca4a616
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Henry8192 marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,7 @@ | |
#include <type_traits> | ||
#include <vector> | ||
|
||
#include <clp/ir/types.hpp> | ||
#include <clp/streaming_compression/zstd/Decompressor.hpp> | ||
#include <clp/type_utils.hpp> | ||
#include <emscripten/em_asm.h> | ||
|
@@ -29,6 +30,7 @@ EMSCRIPTEN_DECLARE_VAL_TYPE(ReaderOptions); | |
// JS types used as outputs | ||
EMSCRIPTEN_DECLARE_VAL_TYPE(DecodedResultsTsType); | ||
EMSCRIPTEN_DECLARE_VAL_TYPE(FilteredLogEventMapTsType); | ||
EMSCRIPTEN_DECLARE_VAL_TYPE(LogEventIdxTsType); | ||
|
||
enum class StreamType : uint8_t { | ||
Structured, | ||
|
@@ -44,6 +46,23 @@ using LogEvents = std::vector<LogEventWithFilterData<LogEvent>>; | |
*/ | ||
using FilteredLogEventsMap = std::optional<std::vector<size_t>>; | ||
|
||
template <typename LogEvent> | ||
concept GetLogEventIdxInterface = requires( | ||
LogEventWithFilterData<LogEvent> const& event, | ||
clp::ir::epoch_time_ms_t timestamp | ||
) { | ||
{ | ||
event.get_timestamp() | ||
} -> std::convertible_to<clp::ir::epoch_time_ms_t>; | ||
}; | ||
|
||
template <typename LogEvent, typename ToStringFunc> | ||
concept DecodeRangeInterface = requires(ToStringFunc func, LogEvent const& log_event) { | ||
{ | ||
func(log_event) | ||
} -> std::convertible_to<std::string>; | ||
}; | ||
|
||
/** | ||
* Class to deserialize and decode Zstandard-compressed CLP IR streams as well as format decoded | ||
* log events. | ||
|
@@ -123,6 +142,18 @@ class StreamReader { | |
*/ | ||
[[nodiscard]] virtual auto decode_range(size_t begin_idx, size_t end_idx, bool use_filter) const | ||
-> DecodedResultsTsType = 0; | ||
/** | ||
* Finds the index of the last log event that matches or next to the given timestamp. | ||
* | ||
* @tparam LogEvent | ||
* @param timestamp The timestamp to search for, in milliseconds since the Unix epoch. | ||
* @return The last index of the log event whose timestamp is smaller than or equal to the | ||
* `timestamp`. | ||
* @return `0` if all log event timestamps are larger than the target. | ||
* @return null if no log event exists in the stream. | ||
*/ | ||
[[nodiscard]] virtual auto get_log_event_idx_by_timestamp(clp::ir::epoch_time_ms_t timestamp | ||
) -> LogEventIdxTsType = 0; | ||
|
||
protected: | ||
explicit StreamReader() = default; | ||
|
@@ -143,11 +174,7 @@ class StreamReader { | |
* @throws Propagates `ToStringFunc`'s exceptions. | ||
*/ | ||
template <typename LogEvent, typename ToStringFunc> | ||
requires requires(ToStringFunc func, LogEvent const& log_event) { | ||
{ | ||
func(log_event) | ||
} -> std::convertible_to<std::string>; | ||
} | ||
requires DecodeRangeInterface<LogEvent, ToStringFunc> | ||
static auto generic_decode_range( | ||
size_t begin_idx, | ||
size_t end_idx, | ||
|
@@ -163,7 +190,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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. was it a mistake? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. isn't it duplicated? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's revert the change and fix it in another PR. I think we should also add docs for |
||
* @param[out] filtered_log_event_map Returns the filtered log events. | ||
*/ | ||
template <typename LogEvent> | ||
|
@@ -172,14 +198,23 @@ class StreamReader { | |
LogLevelFilterTsType const& log_level_filter, | ||
LogEvents<LogEvent> const& log_events | ||
) -> void; | ||
|
||
/** | ||
* Templated implementation of `get_log_event_idx_by_timestamp`. | ||
* | ||
* @tparam LogEvent | ||
* @param timestamp | ||
* event timestamps are larger than the target. In that case, return the first log event index. | ||
*/ | ||
template <GetLogEventIdxInterface LogEvent> | ||
auto generic_get_log_event_idx_by_timestamp( | ||
std::vector<LogEventWithFilterData<LogEvent>> const& log_events, | ||
clp::ir::epoch_time_ms_t timestamp | ||
) -> LogEventIdxTsType; | ||
}; | ||
|
||
template <typename LogEvent, typename ToStringFunc> | ||
requires requires(ToStringFunc func, LogEvent const& log_event) { | ||
{ | ||
func(log_event) | ||
} -> std::convertible_to<std::string>; | ||
} | ||
requires DecodeRangeInterface<LogEvent, ToStringFunc> | ||
auto StreamReader::generic_decode_range( | ||
size_t begin_idx, | ||
size_t end_idx, | ||
|
@@ -258,6 +293,34 @@ auto StreamReader::generic_filter_log_events( | |
} | ||
} | ||
} | ||
|
||
template <GetLogEventIdxInterface LogEvent> | ||
auto StreamReader::generic_get_log_event_idx_by_timestamp( | ||
LogEvents<LogEvent> const& log_events, | ||
clp::ir::epoch_time_ms_t timestamp | ||
) -> LogEventIdxTsType { | ||
if (log_events.empty()) { | ||
return LogEventIdxTsType{emscripten::val::null()}; | ||
} | ||
Henry8192 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
auto upper{std::upper_bound( | ||
log_events.begin(), | ||
log_events.end(), | ||
timestamp, | ||
[](clp::ir::epoch_time_ms_t ts, LogEventWithFilterData<LogEvent> const& log_event) { | ||
return ts < log_event.get_timestamp(); | ||
} | ||
)}; | ||
|
||
if (upper == log_events.begin()) { | ||
return LogEventIdxTsType{emscripten::val(0)}; | ||
} | ||
|
||
auto const upper_index{std::distance(log_events.begin(), upper)}; | ||
auto const index{upper_index - 1}; | ||
|
||
return LogEventIdxTsType{emscripten::val(index)}; | ||
} | ||
} // namespace clp_ffi_js::ir | ||
|
||
#endif // CLP_FFI_JS_IR_STREAMREADER_HPP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 returningstd::optional<>
types.https://emscripten.org/docs/porting/connecting_cpp_and_javascript/embind.html#:~:text=For%20convenience%2C%20embind%20provides%20factory%20functions%20to%20register%20std%3A%3Avector%3CT%3E%20(register_vector())%2C%20std%3A%3Amap%3CK%2C%20V%3E%20(register_map())%2C%20and%20std%3A%3Aoptional%3CT%3E%20(register_optional())%20types%3A
@coderabbitai create an issue to track this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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:
Length of output: 792
✏️ Learnings added
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in a separate PR i meant, so we should create an issue to track the effort
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.