feat(sfa): Add API for decoding all log events. - #155
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Walkthrough
ChangesArchive event decoding
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ClpArchiveReader
participant ClpSfaReader
participant SfaReader
ClpArchiveReader->>ClpSfaReader: decodeAll()
ClpSfaReader->>SfaReader: decode_all()
SfaReader-->>ClpSfaReader: decoded event array
ClpSfaReader-->>ClpArchiveReader: raw event values
ClpArchiveReader-->>ClpArchiveReader: create LogEvent instances
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
junhaoliao
left a comment
junhaoliao
left a comment
There was a problem hiding this comment.
lgtm
as discussed offline, it would be better if we can add some unit test to cover the changes
| * @return Decoded log events. | ||
| * @throws {Error} If the reader has been closed. | ||
| */ | ||
| decodeAll (): LogEvent[] { |
There was a problem hiding this comment.
as discussed offline - can we add coverage for this new public WASM path? something like
const events = reader.decodeAll();
expect(events).toHaveLength(Number(CLP_JSON_TEST_LOG_FILES_EXPECTED_EVENT_COUNT));
events.forEach((event, index) => {
expect(event).toBeInstanceOf(LogEvent);
expect(event.logEventIdx).toBe(BigInt(index));
expect(typeof event.timestamp).toBe("bigint");
expect(typeof event.message).toBe("string");
});
expect(reader.decodeAll()).toEqual(events);| * Decodes all log events in global log-event-index order. | ||
| * | ||
| * @return Decoded log events. | ||
| * @throws {Error} If the reader has been closed. |
There was a problem hiding this comment.
| * @throws {Error} If the reader has been closed. | |
| * @throws {Error} If the reader has been closed or the archive cannot be decoded. |
| /** | ||
| * Epoch timestamp. | ||
| */ |
There was a problem hiding this comment.
would this be more clear?
| /** | |
| * Epoch timestamp. | |
| */ | |
| /** | |
| * Timestamp in milliseconds since the Unix epoch. | |
| */ |
| error.message() | ||
| )}; | ||
| SPDLOG_ERROR("{}", err_msg); | ||
| throw std::runtime_error{err_msg}; |
There was a problem hiding this comment.
shall we use ClpFfiJsException for this new FFI failure path?
| throw std::runtime_error{err_msg}; | |
| throw clp_ffi_js::ClpFfiJsException{ | |
| clp::ErrorCode::ErrorCode_Failure, | |
| __FILENAME__, | |
| __LINE__, | |
| err_msg | |
| }; |
2 participants
Description
Adds the native decodeAll binding and returns raw event objects across the WASM boundary. ClpArchiveReader wraps each raw event in a public LogEvent class with read-only accessors and getKvPairs() support for parsing structured JSON messages.
Checklist
breaking change.
Validation performed
Summary by CodeRabbit
LogEventobject model (index, timestamp, message) and exposed it publicly.JsonValue,JsonObject, andRawLogEvent) and a newisJsonObjecthelper for downstream use.