feat: Add support for deserializing and decoding auto-generated keys in Structured IR streams. #60
Conversation
WalkthroughThis pull request refines methods in the IR handling components. In the Changes
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| auto const json_result{log_event.serialize_to_json()}; | ||
| if (false == json_result.has_value()) { | ||
| auto error_code{json_result.error()}; | ||
| auto json_result{log_event.serialize_to_json()}; |
There was a problem hiding this comment.
Can you refactor the loop in UnstructuredIrStreamReader.cpp to use has_error() as well? There is a very similar log_event_to_string function
There was a problem hiding this comment.
I think we should do it in another PR though
There was a problem hiding this comment.
Just double-checked. has_error has been used in other places correctly. In UnstructuredIrStreamReader.cpp, has_value() is used because the return type is std::optional.
| constexpr std::string_view cAutoGeneratedKey{"auto-generated"}; | ||
| constexpr std::string_view cUserGeneratedKey{"user-generated"}; |
There was a problem hiding this comment.
shall we export those two strings in the Emscripten binding at
clp-ffi-js/src/clp_ffi_js/ir/StreamReader.cpp
Line 116 in 8361ba5
so that the Log Viewer can import those strings from clp-ffi-js?
There was a problem hiding this comment.
how to export a const: https://emscripten.org/docs/porting/connecting_cpp_and_javascript/embind.html#constants
There was a problem hiding this comment.
@junhaoliao, we aren't doing this for most constants, for example the strings above (ReaderOptionsKeys) are duplicates
There was a problem hiding this comment.
The ReaderOptionsKeys are fine not to be exported because we do have those specified in the TypeScript definitions:
clp-ffi-js/src/clp_ffi_js/ir/StreamReader.cpp
Line 121 in 8361ba5
however, if we need to parse the string as a JSON from the decoded results, i think it's better if we can avoid using magic strings as keys to extract the two objects.
There was a problem hiding this comment.
I dont really have a preference @LinZhihao-723 can do what he prefers
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/clp_ffi_js/ir/StructuredIrStreamReader.cpp (1)
153-168: Consider extracting the JSON merging logic into a separate function.The JSON merging logic could be moved to a dedicated function to improve readability and reusability. This would also make the lambda function more focused on its primary responsibility.
Here's a suggested refactor:
+ auto merge_kv_pairs = [](auto&& auto_generated, auto&& user_generated) { + return nlohmann::json{ + {std::string{cMergedKvPairsAutoGeneratedKey}, std::forward<decltype(auto_generated)>(auto_generated)}, + {std::string{cMergedKvPairsUserGeneratedKey}, std::forward<decltype(user_generated)>(user_generated)} + }; + }; + auto log_event_to_string = [](StructuredLogEvent const& log_event) -> std::string { auto json_pair_result{log_event.serialize_to_json()}; if (json_pair_result.has_error()) { auto const error_code{json_pair_result.error()}; SPDLOG_ERROR( "Failed to deserialize log event to JSON: {}:{}", error_code.category().name(), error_code.message() ); return std::string{cEmptyJsonStr}; } auto& [auto_generated, user_generated] = json_pair_result.value(); - nlohmann::json const merged_kv_pairs - = {{std::string{cMergedKvPairsAutoGeneratedKey}, std::move(auto_generated)}, - {std::string{cMergedKvPairsUserGeneratedKey}, std::move(user_generated)}}; - return dump_json_with_replace(merged_kv_pairs); + return dump_json_with_replace(merge_kv_pairs(std::move(auto_generated), std::move(user_generated))); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/clp_ffi_js/ir/StructuredIrStreamReader.cpp(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ...
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
src/clp_ffi_js/ir/StructuredIrStreamReader.cpp
🪛 Cppcheck (2.10-2)
src/clp_ffi_js/ir/StructuredIrStreamReader.cpp
[error] 48-48: syntax error
(syntaxError)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: lint (ubuntu-latest)
- GitHub Check: lint (macos-latest)
🔇 Additional comments (2)
src/clp_ffi_js/ir/StructuredIrStreamReader.cpp (2)
32-33: LGTM! Well-defined constants.The constants follow the project's naming convention and are properly defined as constexpr std::string_view.
48-57: LGTM! Well-implemented Emscripten bindings.The constants are properly exported and converted to std::string, which will help avoid magic strings in the JavaScript code.
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 48-48: syntax error
(syntaxError)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/clp_ffi_js/ir/StructuredIrStreamReader.cpp (1)
153-169: LGTM! The decode_range changes effectively handle auto-generated key-value pairs.The implementation correctly:
- Uses structured binding to unpack the auto-generated and user-generated pairs
- Handles errors appropriately
- Creates a well-structured JSON output
Consider extracting the JSON structure creation into a separate function for better reusability:
auto log_event_to_string = [](StructuredLogEvent const& log_event) -> std::string { auto json_pair_result{log_event.serialize_to_json()}; if (json_pair_result.has_error()) { auto const error_code{json_pair_result.error()}; SPDLOG_ERROR( "Failed to deserialize log event to JSON: {}:{}", error_code.category().name(), error_code.message() ); return std::string{cEmptyJsonStr}; } - auto& [auto_generated, user_generated] = json_pair_result.value(); - nlohmann::json const merged_kv_pairs - = {{std::string{cMergedKvPairsAutoGeneratedKey}, std::move(auto_generated)}, - {std::string{cMergedKvPairsUserGeneratedKey}, std::move(user_generated)}}; - return dump_json_with_replace(merged_kv_pairs); + return create_merged_kv_pairs_json(json_pair_result.value()); }; +auto create_merged_kv_pairs_json(auto const& [auto_generated, user_generated]) -> std::string { + nlohmann::json const merged_kv_pairs + = {{std::string{cMergedKvPairsAutoGeneratedKey}, std::move(auto_generated)}, + {std::string{cMergedKvPairsUserGeneratedKey}, std::move(user_generated)}}; + return dump_json_with_replace(merged_kv_pairs); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/clp_ffi_js/ir/StructuredIrStreamReader.cpp(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ...
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
src/clp_ffi_js/ir/StructuredIrStreamReader.cpp
🪛 Cppcheck (2.10-2)
src/clp_ffi_js/ir/StructuredIrStreamReader.cpp
[error] 48-48: syntax error
(syntaxError)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: lint (ubuntu-latest)
- GitHub Check: lint (macos-latest)
🔇 Additional comments (2)
src/clp_ffi_js/ir/StructuredIrStreamReader.cpp (2)
16-16: LGTM! The new include and constants are well-defined.The new constants follow the existing naming convention and are appropriately scoped.
Also applies to: 32-33
48-57: LGTM! The Emscripten bindings are correctly implemented.The constants are properly exported for use in JavaScript/TypeScript, which aligns with the discussion in past review comments about making these keys available to the Log Viewer.
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 48-48: syntax error
(syntaxError)
|
title: |
Shall we call it "deserialize" to match our glossary? |
We are using deserialize ("to load into c++ objects") and decode ("to string") in this lib. I feel like we are doing both in this PR. |
LinZhihao-723
changed the title
StructuredIrStreamReader. 
Description
This PR syncs with the latest CLP core to support auto-generated kv-pairs with the following changes:
StructuredIrUnitHandlersignature to handle auto-generated key insertion.StructuredIrStreamReader::decode_rangeto return both auto-generated and user-generated kv-pairs.{"auto-generated": {}, "user-generated": {}}Checklist
breaking change.
Validation performed
Use the following js script to read IR streams (modified from #30):
Streams being tested:
Ensured that both streams can be successfully read into expected JSON strings.
Summary by CodeRabbit
Summary by CodeRabbit