-
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
Add LogEventWithFilterData
template class to extend log event classes with processed fields for filtering.
#32
Changes from 11 commits
1a3da51
29a87d2
e5a8e4c
921c322
865e41d
0234365
0c86cf4
84037cf
ef1ff5d
cdcad94
a1e9142
8ae1fa2
3167111
f3862fe
04ea35b
84c5611
cb57bc4
5bdab3c
d48e191
4304325
6936edb
53544ed
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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. | ||
davemarco marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* @tparam LogEvent The type of the log event. | ||
*/ | ||
template <typename LogEvent> | ||
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. If I'm not wrong, all template parameters should following naming convention 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. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A little adding on: 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. @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 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. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. 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. Let me know if you disagree. 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. 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 | ||
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. Why? 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. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeah, I disagree with that decision, lol. I will follow up on that separately.
Fair enough. I will come back to this once we resolve the issue with KeyValuePairLogEvent.
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. Choose a reason for hiding this commentThe 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 |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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> | ||
|
||
|
@@ -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)); | ||
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. As the rabbit suggested, we could emplace directly? 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. 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. Choose a reason for hiding this commentThe 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()
); 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. Yeah, that's what I mean. 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. okay oops changed! |
||
} | ||
m_stream_reader_data_context.reset(nullptr); | ||
return m_encoded_log_events.size(); | ||
|
@@ -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, | ||
|
@@ -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 | ||
); | ||
} | ||
|
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.
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
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.
how about
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.
All good except I think you meant to capitalize
StreamReader
.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.
Sorry I just read the latest code. I think that's good enough. (Kirk might have better ideas about improving that though