Skip to content
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

Add log-level filtering methods and associated refactoring: #12

Merged
merged 50 commits into from
Oct 10, 2024
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
50 commits
Select commit Hold shift + click to select a range
398c23f
my changes for old interface
davemarco Sep 19, 2024
2292781
some work on changing to new interface, not yet complete
Sep 19, 2024
f69ee31
saving changes
davemarco Oct 2, 2024
5b74422
Merge branch 'main' into d-dev2
davemarco Oct 2, 2024
8749844
latest
davemarco Oct 2, 2024
782dd13
latest
davemarco Oct 2, 2024
856ce4a
small nit
davemarco Oct 2, 2024
2937d8f
junhao review
davemarco Oct 2, 2024
95379d6
use std optional
davemarco Oct 2, 2024
773020f
more changes
davemarco Oct 2, 2024
b965d98
more changes
davemarco Oct 2, 2024
5b31d01
small change
davemarco Oct 2, 2024
fbb3f7b
more changes
davemarco Oct 2, 2024
30e76a0
small changes
davemarco Oct 2, 2024
d878399
junhao review
davemarco Oct 2, 2024
d2e3d1d
more changes
davemarco Oct 2, 2024
b1f1afd
change log viewer event name
davemarco Oct 5, 2024
0b3dd23
Apply suggestions from code review
davemarco Oct 5, 2024
a1df31c
Merge branch 'main' into d-dev2
davemarco Oct 5, 2024
c3d13be
run linter
davemarco Oct 5, 2024
14d4977
change error logic for short logs, as there is n reason to actually b…
davemarco Oct 5, 2024
f4d2fd8
Apply suggestions from code review
davemarco Oct 5, 2024
cf093ee
Apply suggestions from code review
davemarco Oct 5, 2024
09656b4
Apply suggestions from code review
davemarco Oct 5, 2024
65a788f
junhao review
davemarco Oct 5, 2024
ca2a0eb
small change
davemarco Oct 5, 2024
a912eeb
fix braces
davemarco Oct 5, 2024
b7c28c9
line issue
davemarco Oct 5, 2024
8f81a51
add line back
davemarco Oct 5, 2024
5be8974
submodule fix?
davemarco Oct 5, 2024
fce620e
fix new name
davemarco Oct 5, 2024
47b9fed
fix lint
davemarco Oct 5, 2024
cf8c036
Merge branch 'main' into d-dev2
davemarco Oct 7, 2024
e8b732c
small change to trigger workflow
davemarco Oct 7, 2024
a273e33
fix clang
davemarco Oct 7, 2024
0a09a45
fix previous mistake? fixing linting error
davemarco Oct 8, 2024
ed140f1
Apply suggestions from code review
davemarco Oct 9, 2024
81e7739
fix lint
davemarco Oct 9, 2024
e967005
rename member function
davemarco Oct 9, 2024
55dabd4
remove clear
davemarco Oct 9, 2024
bd0371a
inline
davemarco Oct 9, 2024
1c4ab88
fix lint
davemarco Oct 9, 2024
2b946ba
Apply suggestions from code review
davemarco Oct 10, 2024
c04d887
Apply suggestions from code review
davemarco Oct 10, 2024
bab5036
kirk review
davemarco Oct 10, 2024
b3fde47
fix lint
davemarco Oct 10, 2024
7564280
fix bug with log level in enscripten
davemarco Oct 10, 2024
7d01cf2
fix lint
davemarco Oct 10, 2024
bd29b8f
Apply suggestions from code review
davemarco Oct 10, 2024
a244f5e
kirk review
davemarco Oct 10, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions src/clp_ffi_js/ir/LogViewerEvent.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
#ifndef CLP_FFI_JS_IR_LOG_VIEWER_EVENT_HPP
#define CLP_FFI_JS_IR_LOG_VIEWER_EVENT_HPP

#include <clp/ir/LogEvent.hpp>
#include <utility>

namespace clp_ffi_js::ir {

/**
* A class derived from LogEvent with an additional member for log level.
* @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> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make more sense to name this as LogEventExt?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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?

Copy link
Member

@kirkrodrigues kirkrodrigues Oct 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

public:
// Constructors
LogViewerEvent(
clp::ir::epoch_time_ms_t timestamp,
clp::UtcOffset utc_offset,
clp::ir::EncodedTextAst<encoded_variable_t> message,
size_t log_level
davemarco marked this conversation as resolved.
Show resolved Hide resolved
)
: clp::ir::LogEvent<encoded_variable_t>(timestamp, utc_offset, std::move(message)),
m_log_level{log_level} {}

// Methods
[[nodiscard]] auto get_log_level() const -> size_t;

private:
size_t m_log_level;
};

template <typename encoded_variable_t>
auto LogViewerEvent<encoded_variable_t>::get_log_level() const -> size_t {
return m_log_level;
}
} // namespace clp_ffi_js::ir

#endif // CLP_FFI_JS_IR_LOG_VIEWER_EVENT_HPP
137 changes: 96 additions & 41 deletions src/clp_ffi_js/ir/StreamReader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include <cstdint>
#include <iterator>
#include <memory>
#include <optional>
#include <span>
#include <string>
#include <string_view>
Expand All @@ -15,7 +16,6 @@
#include <clp/Array.hpp>
#include <clp/ErrorCode.hpp>
#include <clp/ffi/ir_stream/decoding_methods.hpp>
#include <clp/ir/LogEvent.hpp>
#include <clp/ir/LogEventDeserializer.hpp>
#include <clp/ir/types.hpp>
#include <clp/streaming_compression/zstd/Decompressor.hpp>
Expand All @@ -26,6 +26,7 @@

#include <clp_ffi_js/ClpFfiJsException.hpp>
#include <clp_ffi_js/constants.hpp>
#include <clp_ffi_js/ir/LogViewerEvent.hpp>
#include <clp_ffi_js/ir/StreamReaderDataContext.hpp>

using namespace std::literals::string_literals;
Expand Down Expand Up @@ -99,23 +100,58 @@ auto StreamReader::get_num_events_buffered() const -> size_t {
return m_encoded_log_events.size();
}

auto StreamReader::deserialize_range(size_t begin_idx, size_t end_idx) -> size_t {
constexpr size_t cFullRangeEndIdx{0};
if (0 != begin_idx || cFullRangeEndIdx != end_idx) {
throw ClpFfiJsException{
clp::ErrorCode::ErrorCode_Unsupported,
__FILENAME__,
__LINE__,
"Partial range deserialization is not yet supported."
};
auto StreamReader::get_filtered_log_event_map() const -> FilteredLogEventMapType {
if (std::nullopt == m_filtered_log_event_map) {
davemarco marked this conversation as resolved.
Show resolved Hide resolved
return FilteredLogEventMapType(emscripten::val::null());
}

return FilteredLogEventMapType(emscripten::val::array(m_filtered_log_event_map.value()));
}

auto StreamReader::build() -> size_t {
if (nullptr != m_stream_reader_data_context) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use an early return to reduce indentation.

constexpr size_t cDefaultNumReservedLogEvents{500'000};
m_encoded_log_events.reserve(cDefaultNumReservedLogEvents);

std::string logtype;
constexpr size_t cDefaultReservedMessageLength{512};
logtype.reserve(cDefaultReservedMessageLength);
while (true) {
auto result{m_stream_reader_data_context->get_deserializer().deserialize_log_event()};
if (false == result.has_error()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that the positive case handling is much longer, shall we flip the if? i.e., let's write if (result.has_error()) {...} instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

m_encoded_log_events.emplace_back(std::move(result.value()));

const auto log_event = result.value();
const auto message = log_event.get_message();

logtype.clear();
logtype = message.get_logtype();
junhaoliao marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need the upper-level declaration of logtype, right? (i.e., if appropriate, let's make logtype a local variable within the if-block.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I copied this from the code for message in decode() see message.reserve(cDefaultReservedMessageLength); . I assume this is some kind of performance optimization? But no i don't think it is neccesary.


davemarco marked this conversation as resolved.
Show resolved Hide resolved
constexpr size_t cLogLevelPositionInMessages{1};
if (logtype.length() < cLogLevelPositionInMessages) {
SPDLOG_ERROR("Failed to extract log type for log level parsing."); break;
davemarco marked this conversation as resolved.
Show resolved Hide resolved
}

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);
}

const auto log_viewer_event = LogViewerEvent<four_byte_encoded_variable_t>(
log_event.get_timestamp(),
log_event.get_utc_offset(),
message,
log_level
);

m_encoded_log_events.emplace_back(std::move(log_viewer_event));
continue;
}
auto const error{result.error()};
Expand All @@ -135,27 +171,38 @@ auto StreamReader::deserialize_range(size_t begin_idx, size_t end_idx) -> size_t
}
m_stream_reader_data_context.reset(nullptr);
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If not intentional, let's revert this line removal.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

return m_encoded_log_events.size();
}

auto StreamReader::decode_range(size_t begin_idx, size_t end_idx) const -> DecodedResultsTsType {
if (m_encoded_log_events.size() < end_idx || begin_idx >= end_idx) {
auto StreamReader::decode_range(size_t begin_idx, size_t end_idx, bool use_filter) const -> DecodedResultsTsType {

if (use_filter && (std::nullopt == m_filtered_log_event_map)) {
davemarco marked this conversation as resolved.
Show resolved Hide resolved
return DecodedResultsTsType(emscripten::val::null());
davemarco marked this conversation as resolved.
Show resolved Hide resolved
}

std::span const log_events_span{
m_encoded_log_events.begin()
+ static_cast<decltype(m_encoded_log_events)::difference_type>(begin_idx),
m_encoded_log_events.begin()
+ static_cast<decltype(m_encoded_log_events)::difference_type>(end_idx)
};
size_t length;
if (use_filter) {
length = m_filtered_log_event_map->size();
} else {
length = m_encoded_log_events.size();
}

davemarco marked this conversation as resolved.
Show resolved Hide resolved
if (length < end_idx || 0 > begin_idx) {
junhaoliao marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As 🐰 mentioned, begin_idx is a size_t, so it can't be < 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥕 -> 🐰

return DecodedResultsTsType(emscripten::val::null());
davemarco marked this conversation as resolved.
Show resolved Hide resolved
}
davemarco marked this conversation as resolved.
Show resolved Hide resolved
davemarco marked this conversation as resolved.
Show resolved Hide resolved
std::string message;
constexpr size_t cDefaultReservedMessageLength{512};
message.reserve(cDefaultReservedMessageLength);
size_t log_num{begin_idx + 1};
auto const results{emscripten::val::array()};
for (auto const& log_event : log_events_span) {

for (size_t i = begin_idx; i < end_idx; ++i) {
size_t log_event_idx;
if (use_filter) {
log_event_idx = m_filtered_log_event_map->at(i);
} else {
log_event_idx = i;
}
const auto& log_event = m_encoded_log_events[log_event_idx];
message.clear();

auto const parsed{log_event.get_message().decode_and_unparse()};
Expand All @@ -165,36 +212,38 @@ auto StreamReader::decode_range(size_t begin_idx, size_t end_idx) const -> Decod
}
message.append(parsed.value());

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 message.substr(cLogLevelPositionInMessages).starts_with(level);
}
)};
if (log_level_name_it != cLogLevelNames.end()) {
log_level = std::distance(cLogLevelNames.begin(), log_level_name_it);
}

m_ts_pattern.insert_formatted_timestamp(log_event.get_timestamp(), message);

EM_ASM(
{ Emval.toValue($0).push([UTF8ToString($1), $2, $3, $4]); },
results.as_handle(),
message.c_str(),
log_event.get_timestamp(),
log_level,
log_num
log_event.get_log_level(),
log_event_idx +1
);
++log_num;
}

return DecodedResultsTsType(results);
}

void StreamReader::filter_log_events(const emscripten::val& logLevelFilter) {
if (logLevelFilter.isNull()) {
m_filtered_log_event_map.reset();
return;
}

m_filtered_log_event_map.emplace();
std::vector<int> filter_levels = emscripten::vecFromJSArray<int>(logLevelFilter);
davemarco marked this conversation as resolved.
Show resolved Hide resolved

davemarco marked this conversation as resolved.
Show resolved Hide resolved
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);
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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);
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverting back to previous. I dont think most people are using c++23 yet, so not sure we want to use std::views::enumerate

}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Optimization suggestion for filter_log_events method

The filter_log_events method is well-implemented, but consider the following optimization:

Replace the linear search with a more efficient data structure for log level lookup. Using an std::unordered_set instead of std::vector for filter_levels can improve performance, especially for large datasets.

Here's a suggested optimization:

void StreamReader::filter_log_events(const emscripten::val& logLevelFilter) {
    if (logLevelFilter.isNull()) {
        m_filtered_log_event_map.reset();
        return;
    }

    m_filtered_log_event_map.emplace();
    std::unordered_set<int> filter_levels(emscripten::vecFromJSArray<int>(logLevelFilter).begin(),
                                          emscripten::vecFromJSArray<int>(logLevelFilter).end());

    for (size_t index = 0; index < m_encoded_log_events.size(); ++index) {
        const auto& logEvent = m_encoded_log_events[index];
        if (filter_levels.count(logEvent.get_log_level()) > 0) {
            m_filtered_log_event_map->push_back(index);
        }
    }
}

This change reduces the time complexity of the level lookup from O(n) to O(1), where n is the number of filter levels.


StreamReader::StreamReader(
StreamReaderDataContext<four_byte_encoded_variable_t>&& stream_reader_data_context
)
Expand All @@ -211,6 +260,10 @@ EMSCRIPTEN_BINDINGS(ClpIrStreamReader) {
emscripten::register_type<clp_ffi_js::ir::DecodedResultsTsType>(
"Array<[string, number, number, number]>"
);
emscripten::register_type<clp_ffi_js::ir::FilteredLogEventMapType>(
"number[]"
);

emscripten::class_<clp_ffi_js::ir::StreamReader>("ClpIrStreamReader")
.constructor(
&clp_ffi_js::ir::StreamReader::create,
Expand All @@ -220,7 +273,9 @@ EMSCRIPTEN_BINDINGS(ClpIrStreamReader) {
"getNumEventsBuffered",
&clp_ffi_js::ir::StreamReader::get_num_events_buffered
)
.function("deserializeRange", &clp_ffi_js::ir::StreamReader::deserialize_range)
.function("decodeRange", &clp_ffi_js::ir::StreamReader::decode_range);
.function("getFilteredLogEventMap", &clp_ffi_js::ir::StreamReader::get_filtered_log_event_map)
.function("build", &clp_ffi_js::ir::StreamReader::build)
.function("decodeRange", &clp_ffi_js::ir::StreamReader::decode_range)
.function("filterLogEvents", &clp_ffi_js::ir::StreamReader::filter_log_events);
davemarco marked this conversation as resolved.
Show resolved Hide resolved
}
} // namespace
39 changes: 27 additions & 12 deletions src/clp_ffi_js/ir/StreamReader.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

#include <cstddef>
#include <memory>
#include <optional>
#include <vector>

#include <clp/ir/LogEvent.hpp>
Expand All @@ -12,10 +13,12 @@
#include <emscripten/val.h>

#include <clp_ffi_js/ir/StreamReaderDataContext.hpp>
#include <clp_ffi_js/ir/LogViewerEvent.hpp>

namespace clp_ffi_js::ir {
EMSCRIPTEN_DECLARE_VAL_TYPE(DataArrayTsType);
EMSCRIPTEN_DECLARE_VAL_TYPE(DecodedResultsTsType);
EMSCRIPTEN_DECLARE_VAL_TYPE(FilteredLogEventMapType);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be XxxTsType for consistency with the others.


/**
* Class to deserialize and decode Zstandard-compressed CLP IR streams as well as format decoded
Expand Down Expand Up @@ -50,42 +53,54 @@ class StreamReader {
[[nodiscard]] auto get_num_events_buffered() const -> size_t;

/**
* Deserializes and buffers log events in the range `[beginIdx, endIdx)`. After the stream has
* been exhausted, it will be deallocated.
* @return The filtered log events map.
*/
[[nodiscard]] auto get_filtered_log_event_map() const -> FilteredLogEventMapType;

/**
* Filters log events and generates `m_filtered_log_event_map`. If `logLevelFilter` is `null`,
* `m_filtered_log_event_map` will be set to `std::nullopt`.
davemarco marked this conversation as resolved.
Show resolved Hide resolved
*
* NOTE: Currently, this class only supports deserializing the full range of log events in the
* stream.
* @param logLevelFilter Array of selected log levels
davemarco marked this conversation as resolved.
Show resolved Hide resolved
*/
void filter_log_events(const emscripten::val& logLevelFilter);

/**
* Deserializes all log events in the file. After the stream has been exhausted, it will be
davemarco marked this conversation as resolved.
Show resolved Hide resolved
* deallocated.
*
* @param begin_idx
* @param end_idx
* @return The number of successfully deserialized ("valid") log events.
*/
[[nodiscard]] auto deserialize_range(size_t begin_idx, size_t end_idx) -> size_t;
[[nodiscard]] auto build() -> size_t;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about deserialize_stream? From a library perspective (i.e., ignoring how the log viewer uses this library), build sounds like it's going to build the StreamReader, not deserialize all events in the stream.

Copy link
Contributor Author

@davemarco davemarco Oct 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am okay with this, but also consider the fact i don't really believe this code base is a "generic ir library". For example, clp-ffi-go exposes more general functions and presents itself as a go wrapper for the c++ ir library. The functions in this code base are very specific to log viewer and appear to really be an accelerator for a js implemetation of log viewer irDecoder. From this perspective, build() is more appropriate.

Copy link
Contributor Author

@davemarco davemarco Oct 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example I could not build a js implementation of fluent-bit-clp with this "ir library"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but this library needs to start somewhere and we don't have time to implement all of the IR methods right now, right? It would also waste more time to create a log-viewer-specific accelerator library just to eventually throw it away and replace it with this library. Fwiw, build is a pretty poor name in general, even in the log viewer, lol.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay my opinions are not so strong here. I am happy with deserialize_stream.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done!

davemarco marked this conversation as resolved.
Show resolved Hide resolved

/**
* Decodes the deserialized log events in the range `[beginIdx, endIdx)`.
* 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.
davemarco marked this conversation as resolved.
Show resolved Hide resolved
* @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 file).
* of log events in collection)
davemarco marked this conversation as resolved.
Show resolved Hide resolved
*/
[[nodiscard]] auto decode_range(size_t begin_idx, size_t end_idx) const -> DecodedResultsTsType;
[[nodiscard]] auto decode_range(size_t begin_idx, size_t end_idx, bool use_filter) const -> DecodedResultsTsType;


private:
// Constructor
explicit StreamReader(StreamReaderDataContext<clp::ir::four_byte_encoded_variable_t>&&
stream_reader_data_context);

// Variables
std::vector<clp::ir::LogEvent<clp::ir::four_byte_encoded_variable_t>> m_encoded_log_events;
std::vector<LogViewerEvent<clp::ir::four_byte_encoded_variable_t>> m_encoded_log_events;
std::unique_ptr<StreamReaderDataContext<clp::ir::four_byte_encoded_variable_t>>
m_stream_reader_data_context;
m_stream_reader_data_context;
std::optional<std::vector<size_t>> m_filtered_log_event_map;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Update member variables for new functionality

The changes to m_encoded_log_events and the addition of m_filtered_log_event_map are consistent with the new filtering functionality.

However, m_filtered_log_event_map is not initialized, which could lead to undefined behaviour. Consider initializing it in the constructor or at declaration:

std::optional<std::vector<size_t>> m_filtered_log_event_map{};

davemarco marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's replace the type with a type we define at the top of the class.

    /**
     * Mapping between an index in the filtered log events collection to an index in the unfiltered
     * log events collection.
     */
    using FilteredLogEventsMap = std::optional<std::vector<size_t>>;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

clp::TimestampPattern m_ts_pattern;
};
} // namespace clp_ffi_js::ir
Expand Down