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 LogEventWithFilterData template class to extend log event classes with processed fields for filtering. #32

Merged
merged 22 commits into from
Nov 11, 2024
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
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
53 changes: 53 additions & 0 deletions src/clp_ffi_js/ir/LogEventWithFilterData.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
#ifndef CLP_FFI_JS_IR_LOGEVENTWITHFILTERDATA_HPP
#define CLP_FFI_JS_IR_LOGEVENTWITHFILTERDATA_HPP

#include <utility>

#include <clp/ir/types.hpp>

#include <clp_ffi_js/constants.hpp>

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.
Copy link
Member

@junhaoliao junhaoliao Nov 9, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@davemarco davemarco Nov 10, 2024

Choose a reason for hiding this comment

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

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 streamReader to be more efficient since there is no need to "reparse" these values on every call to filterLogEvents() and decodeRange().

Copy link
Member

Choose a reason for hiding this comment

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

All good except I think you meant to capitalize StreamReader.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry I just read the latest code. I think that's good enough. (Kirk might have better ideas about improving that though

davemarco marked this conversation as resolved.
Show resolved Hide resolved
* @tparam LogEvent The type of the log event.
*/
template <typename LogEvent>
Copy link
Member

Choose a reason for hiding this comment

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

If I'm not wrong, all template parameters should following naming convention type_name_t.
cc @LinZhihao-723 / @kirkrodrigues please help confirm.

Copy link
Member

Choose a reason for hiding this comment

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

Only primitive template parameters are named following the format type_name_t. Non-primitives are named how Marco has them.

Copy link
Member

Choose a reason for hiding this comment

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

A little adding on:
Since we're using C++20, we should use requires or define a concept about the accepted LogEvent type

Copy link
Member

@junhaoliao junhaoliao Nov 10, 2024

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@davemarco davemarco Nov 10, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

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 added something to do this.

class LogEventWithFilterData {
public:
// Constructor
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} {}

// Disable copy constructor and assignment operator
Copy link
Member

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

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 makes sense, but left it as is for now until we update KeyValuePairLogEvent

LogEventWithFilterData(LogEventWithFilterData const&) = delete;
auto operator=(LogEventWithFilterData const&) -> LogEventWithFilterData& = delete;

// Default move constructor and assignment operator
LogEventWithFilterData(LogEventWithFilterData&&) = default;
auto operator=(LogEventWithFilterData&&) -> LogEventWithFilterData& = default;

// Destructor
~LogEventWithFilterData() = default;

[[nodiscard]] auto get_log_event() const -> LogEvent const& { return m_log_event; }

[[nodiscard]] auto get_log_level() const -> LogLevel { return m_log_level; }

[[nodiscard]] auto get_timestamp() const -> clp::ir::epoch_time_ms_t { return m_timestamp; }

private:
LogEvent m_log_event;
LogLevel m_log_level;
clp::ir::epoch_time_ms_t m_timestamp;
};
} // namespace clp_ffi_js::ir

#endif // CLP_FFI_JS_IR_LOGEVENTWITHFILTERDATA_HPP
41 changes: 0 additions & 41 deletions src/clp_ffi_js/ir/LogEventWithLevel.hpp

This file was deleted.

28 changes: 15 additions & 13 deletions src/clp_ffi_js/ir/UnstructuredIrStreamReader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,7 @@
#include <spdlog/spdlog.h>

#include <clp_ffi_js/ClpFfiJsException.hpp>
#include <clp_ffi_js/constants.hpp>
#include <clp_ffi_js/ir/LogEventWithLevel.hpp>
#include <clp_ffi_js/ir/LogEventWithFilterData.hpp>
#include <clp_ffi_js/ir/StreamReader.hpp>
#include <clp_ffi_js/ir/StreamReaderDataContext.hpp>

Expand Down Expand Up @@ -147,13 +146,12 @@ auto UnstructuredIrStreamReader::deserialize_stream() -> size_t {
}
}

auto log_viewer_event{LogEventWithLevel<four_byte_encoded_variable_t>(
log_event.get_timestamp(),
log_event.get_utc_offset(),
message,
log_level
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));
Copy link
Member

Choose a reason for hiding this comment

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

As the rabbit suggested, we could emplace directly?

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

Copy link
Member

@junhaoliao junhaoliao Nov 11, 2024

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that's what I mean.

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 oops changed!

}
m_stream_reader_data_context.reset(nullptr);
return m_encoded_log_events.size();
Expand Down Expand Up @@ -187,9 +185,13 @@ auto UnstructuredIrStreamReader::decode_range(size_t begin_idx, size_t end_idx,
} else {
log_event_idx = i;
}
auto const& log_event{m_encoded_log_events[log_event_idx]};
auto const& log_event_with_filter_data{m_encoded_log_events[log_event_idx]};
auto const& log_level = log_event_with_filter_data.get_log_level();
auto const& timestamp = log_event_with_filter_data.get_timestamp();

auto const parsed{log_event.get_message().decode_and_unparse()};
auto const& unstructured_log_event = log_event_with_filter_data.get_log_event();
kirkrodrigues marked this conversation as resolved.
Show resolved Hide resolved

auto const parsed{unstructured_log_event.get_message().decode_and_unparse()};
if (false == parsed.has_value()) {
throw ClpFfiJsException{
clp::ErrorCode::ErrorCode_Failure,
Expand All @@ -200,14 +202,14 @@ auto UnstructuredIrStreamReader::decode_range(size_t begin_idx, size_t end_idx,
}
message = parsed.value();

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

EM_ASM(
{ Emval.toValue($0).push([UTF8ToString($1), $2, $3, $4]); },
results.as_handle(),
message.c_str(),
log_event.get_timestamp(),
log_event.get_log_level(),
timestamp,
log_level,
log_event_idx + 1
);
}
Expand Down
6 changes: 4 additions & 2 deletions src/clp_ffi_js/ir/UnstructuredIrStreamReader.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -7,18 +7,20 @@
#include <optional>
#include <vector>

#include <clp/ir/LogEvent.hpp>
#include <clp/ir/LogEventDeserializer.hpp>
#include <clp/ir/types.hpp>
#include <clp/TimestampPattern.hpp>
#include <emscripten/val.h>

#include <clp_ffi_js/ir/LogEventWithLevel.hpp>
#include <clp_ffi_js/ir/LogEventWithFilterData.hpp>
#include <clp_ffi_js/ir/StreamReader.hpp>
#include <clp_ffi_js/ir/StreamReaderDataContext.hpp>

namespace clp_ffi_js::ir {
using clp::ir::four_byte_encoded_variable_t;
using UnstructuredIrDeserializer = clp::ir::LogEventDeserializer<four_byte_encoded_variable_t>;
using UnstructuredLogEvent = clp::ir::LogEvent<four_byte_encoded_variable_t>;

/**
* Mapping between an index in the filtered log events collection to an index in the unfiltered
Expand Down Expand Up @@ -85,7 +87,7 @@ class UnstructuredIrStreamReader : public StreamReader {
);

// Variables
std::vector<LogEventWithLevel<four_byte_encoded_variable_t>> m_encoded_log_events;
std::vector<LogEventWithFilterData<UnstructuredLogEvent>> m_encoded_log_events;
std::unique_ptr<StreamReaderDataContext<UnstructuredIrDeserializer>>
m_stream_reader_data_context;
FilteredLogEventsMap m_filtered_log_event_map;
Expand Down
Loading