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

feat: Add support for log-level filtering of structured IR streams. #35

Merged
merged 55 commits into from
Dec 18, 2024
Merged
Changes from 1 commit
Commits
Show all changes
55 commits
Select commit Hold shift + click to select a range
1a3da51
add new class
davemarco Nov 9, 2024
29a87d2
fix-lint
davemarco Nov 9, 2024
e5a8e4c
fix lint
davemarco Nov 9, 2024
921c322
small change
davemarco Nov 9, 2024
865e41d
small change
davemarco Nov 9, 2024
0234365
fix lint
davemarco Nov 9, 2024
0c86cf4
Update src/clp_ffi_js/ir/LogEventWithFilterData.hpp
davemarco Nov 9, 2024
84037cf
fix lint
davemarco Nov 10, 2024
ef1ff5d
fix lint
davemarco Nov 10, 2024
cdcad94
fix lint
davemarco Nov 10, 2024
a1e9142
fix lint
davemarco Nov 10, 2024
2738dd5
first draft
davemarco Nov 10, 2024
d4afd35
add missing filtering
davemarco Nov 10, 2024
3f32f91
fix lint
davemarco Nov 10, 2024
01ef415
add support for int
davemarco Nov 10, 2024
de5aaf5
add new functions
davemarco Nov 11, 2024
67f43f0
cleanup
davemarco Nov 11, 2024
be0e5ba
lint
davemarco Nov 11, 2024
8e8c648
add files
davemarco Nov 11, 2024
f2907f4
changes
davemarco Nov 11, 2024
a1b776f
fix filter
davemarco Nov 11, 2024
a199924
Merge branch 'main' into sFilter
davemarco Nov 11, 2024
d00d0a2
change version
davemarco Nov 11, 2024
e9ed45d
fix lint
davemarco Nov 11, 2024
72029f1
small change
davemarco Nov 11, 2024
c757571
junhao review
davemarco Nov 15, 2024
4975f79
fix lint
davemarco Nov 25, 2024
7346715
fix lint
davemarco Nov 25, 2024
04a8d1f
junhao changes
davemarco Dec 2, 2024
2a202b8
add missing code in structured reader
davemarco Dec 5, 2024
5332e9b
add template for decode
davemarco Dec 6, 2024
da6350f
lint
davemarco Dec 6, 2024
ee2eb42
fix lint
davemarco Dec 6, 2024
90b977f
reword some comments
davemarco Dec 6, 2024
8cc73d0
junhao review
davemarco Dec 9, 2024
4d905e8
remove double declaration
davemarco Dec 9, 2024
5854e36
fix lint
davemarco Dec 10, 2024
f3f6e76
move reserve up
davemarco Dec 10, 2024
d4467ea
Apply suggestions from code review
davemarco Dec 12, 2024
6660763
add reader option param
davemarco Dec 12, 2024
5b684ab
refactor using lambda
davemarco Dec 12, 2024
9a5a502
small nits
davemarco Dec 12, 2024
cce7980
fix lint
davemarco Dec 12, 2024
feec5ef
Apply suggestions from code review
davemarco Dec 13, 2024
61e1e83
kirk review
davemarco Dec 13, 2024
907c327
fix lint
davemarco Dec 13, 2024
1da4c7d
fix lint
davemarco Dec 13, 2024
f08e07f
Apply suggestions from code review
davemarco Dec 13, 2024
aee6422
kirk review
davemarco Dec 13, 2024
0581310
added rerror for invalid range
davemarco Dec 14, 2024
2aabf7b
Update src/clp_ffi_js/ir/StreamReader.hpp
davemarco Dec 18, 2024
2d7e02e
add throws doc strings
davemarco Dec 18, 2024
7d0a2c8
fix
davemarco Dec 18, 2024
88e6d3e
junhao review
davemarco Dec 18, 2024
7df5e26
add mising word
davemarco Dec 18, 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
Prev Previous commit
Next Next commit
add support for int
davemarco committed Nov 10, 2024
commit 01ef4154c19369ebe7683833f025711bcd0508b1
1 change: 1 addition & 0 deletions src/clp_ffi_js/constants.hpp
Original file line number Diff line number Diff line change
@@ -19,6 +19,7 @@ enum class LogLevel : std::uint8_t {
FATAL,
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add a Length as the last item and use it to replace cValidLogLevelsEndIdx?

Suggested change
};
Length
};

That way we don't always have to update cValidLogLevelsEndIdx if we ever add more log levels.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was looking at https://github.com/y-scope/clp/blob/f1876cd63a56dd79a6243f6c8b6b6a7fd48348cc/components/core/src/clp/GlobalMySQLMetadataDB.cpp#L22-L23 when i suggested the enum name "length" though I don't see the convention being documented anywhere. If that's a convention, i feel we can omit the comment.

@kirkrodrigues could you confirm the convention?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's a general C/C++ convention to call it length; that said, it's probably not a bad idea to leave a comment because maybe the reader would confuse "SIZE" for a log level.

I also realized that the case of these enum values is wrong. They should be None, Trace, etc. Can we open an issue and fix in another PR?

Copy link
Collaborator

@junhaoliao junhaoliao Dec 2, 2024

Choose a reason for hiding this comment

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

@coderabbitai let's open an issue to correct cases of enum property names as PascalCase

Copy link

Choose a reason for hiding this comment

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

@junhaoliao, I've successfully created the issue to correct the enum property names to PascalCase: Issue #39.

🔗 Analysis chain

@junhaoliao, I've opened an issue to correct the cases of enum property names to PascalCase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
gh issue create \
  --title "Correct enum property names to PascalCase" \
  --body "As discussed in [PR #35](https://github.com/y-scope/clp-ffi-js/pull/35) and [this comment](https://github.com/y-scope/clp-ffi-js/pull/35#discussion_r1842284173), the enum property names in \`src/clp_ffi_js/constants.hpp\` should be updated to PascalCase to adhere to coding conventions.

Requested by @junhaoliao." \
  --assignee junhaoliao

Length of output: 460

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 changed to Length.

Copy link

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

constexpr LogLevel cValidLogLevelsBeginIdx{LogLevel::TRACE};
constexpr LogLevel cValidLogLevelsEndIdx{LogLevel::FATAL};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Indices bounds are non-inclusive, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure i changed


/**
* Strings corresponding to `LogLevel`.
6 changes: 6 additions & 0 deletions src/clp_ffi_js/ir/StructuredIrStreamReader.cpp
Original file line number Diff line number Diff line change
@@ -6,6 +6,7 @@
#include <string>
#include <string_view>
#include <system_error>
#include <type_utils.hpp>
#include <utility>
#include <vector>

@@ -113,6 +114,11 @@ auto IrUnitHandler::handle_log_event(StructuredLogEvent&& log_event
auto const& log_level_name
= log_level_pair.value().get_immutable_view<std::string>();
log_level = get_log_level(log_level_name);
} else if (log_level_pair->is<clp::ffi::value_int_t>()) {
auto const& value = (log_level_pair.value().get_immutable_view<clp::ffi::value_int_t>());
if (value <= (clp::enum_to_underlying_type(cValidLogLevelsEndIdx))) {
log_level = static_cast<LogLevel>(value);
}
} else {
SPDLOG_ERROR("Log level type is not string");
}