feat(webui): Support viewing search results in context for JSON logs (clp-json). #596
Conversation
junhaoliao
left a comment
junhaoliao
left a comment
There was a problem hiding this comment.
Changes in DbManager looks good.
junhaoliao
left a comment
junhaoliao
left a comment
There was a problem hiding this comment.
All good except one suggestion in a method signature.
| if (null === streamId) { | ||
| resp.code(StatusCodes.BAD_REQUEST); | ||
| throw new Error("streamId must not be null"); | ||
| } |
There was a problem hiding this comment.
I agree. Except that we do not need to check !streamId because typeof streamId !== "string" should have handled that.
| async submitAndWaitForExtractStreamJob ( | ||
| jobType, | ||
| logEventIdx, | ||
| streamId, | ||
| targetUncompressedSize | ||
| ) { |
There was a problem hiding this comment.
When there are multiple parameters to a single function, it's be better to use the object destructuring syntax:
| async submitAndWaitForExtractStreamJob ( | |
| jobType, | |
| logEventIdx, | |
| streamId, | |
| targetUncompressedSize | |
| ) { | |
| async submitAndWaitForExtractStreamJob ({ | |
| jobType, | |
| logEventIdx, | |
| streamId, | |
| targetUncompressedSize | |
| }) { |
The JsDoc can be updated as:
* @param {object} props
* @param {number} props.jobType
* @param {number} props.logEventIdx
* @param {string} props.streamId
* @param {number} props.targetUncompressedSize
Then the method call can be modified as:
const extractResult = await fastify.dbManager.submitAndWaitForExtractStreamJob({
jobType: extractJobType,
logEventIdx: sanitizedLogEventIdx,
streamId: streamId,
targetUncompressedSize: settings.StreamTargetUncompressedSize,
});
which looks more readable. What do you think?
| export {EXTRACT_JOB_TYPES}; | ||
| export {QUERY_JOB_TYPE}; |
There was a problem hiding this comment.
| export {EXTRACT_JOB_TYPES}; | |
| export {QUERY_JOB_TYPE}; | |
| export { | |
| EXTRACT_JOB_TYPES, | |
| QUERY_JOB_TYPE, | |
| }; |
| if (null === streamId) { | ||
| resp.code(StatusCodes.BAD_REQUEST); | ||
| throw new Error("streamId must not be null"); | ||
| } |
There was a problem hiding this comment.
| if (null === streamId) { | |
| resp.code(StatusCodes.BAD_REQUEST); | |
| throw new Error("streamId must not be null"); | |
| } | |
| if (typeof streamId !== "string" || 0 === streamId.trim().length) { | |
| resp.code(StatusCodes.BAD_REQUEST); | |
| throw new Error(`"streamId" must be a non-empty string.`); | |
| } |
| if (false === EXTRACT_JOB_TYPES.includes(extractJobType)) { | ||
| resp.code(StatusCodes.BAD_REQUEST); | ||
| throw new Error("Invalid extractJobType"); | ||
| } |
There was a problem hiding this comment.
| if (false === EXTRACT_JOB_TYPES.includes(extractJobType)) { | |
| resp.code(StatusCodes.BAD_REQUEST); | |
| throw new Error("Invalid extractJobType"); | |
| } | |
| if (false === EXTRACT_JOB_TYPES.includes(extractJobType)) { | |
| resp.code(StatusCodes.BAD_REQUEST); | |
| throw new Error(`Invalid extractJobType="${extractJobType}".`); | |
| } |
| fastify.post("/query/extract-ir", async (req, resp) => { | ||
| const {origFileId, logEventIdx} = req.body; | ||
| fastify.post("/query/extract-stream", async (req, resp) => { | ||
| const {extractJobType, logEventIdx, streamId} = req.body; |
There was a problem hiding this comment.
Let's move the parameter validation checks directly below this line.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
components/log-viewer-webui/server/src/routes/query.js (3)
Line range hint
6-14: Enhance route documentationThe JSDoc comment should include details about request parameters and possible error responses.
/** * Creates query routes. * + * @param {Object} req.body + * @param {string} req.body.extractJobType - The type of extraction job (must be in EXTRACT_JOB_TYPES) + * @param {string} req.body.streamId - The ID of the stream to extract + * @param {number} req.body.logEventIdx - The index of the log event + * @throws {Error} When parameters are invalid or extraction fails * @param {import("fastify").FastifyInstance | {dbManager: DbManager}} fastify * @param {import("fastify").FastifyPluginOptions} options * @return {Promise<void>} */
42-44: Add logging for extraction failuresConsider logging extraction failures to help with debugging and monitoring.
+ fastify.log.error(`Stream extraction failed for streamId=${streamId} at logEventIdx=${sanitizedLogEventIdx}`); resp.code(StatusCodes.BAD_REQUEST); throw new Error("Unable to extract stream with " + `streamId=${streamId} at logEventIdx=${sanitizedLogEventIdx}`);
59-59: Consider using a consistent response structureFor consistency across endpoints, consider wrapping the response in a standard structure.
- return streamMetadata; + return { + success: true, + data: streamMetadata + };components/log-viewer-webui/server/src/DbManager.js (2)
69-75: Enhance constant documentationConsider adding more descriptive JSDoc comments to explain the purpose and usage of
EXTRACT_JOB_TYPES. For example:/** - * List of valid extract job types. + * An immutable array containing valid job types for stream extraction operations. + * Used for validation and type checking in stream extraction related operations. + * @type {ReadonlyArray<number>} */
172-177: Track technical debt: field naming in MongoDBAs discussed in previous reviews,
orig_file_idis being used for both IR and JSON streams. Consider creating a technical debt ticket to standardize this field tostream_idin a future release.Would you like me to create a GitHub issue to track this technical debt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
components/log-viewer-webui/server/src/DbManager.js(4 hunks)components/log-viewer-webui/server/src/routes/query.js(2 hunks)
🔇 Additional comments (4)
components/log-viewer-webui/server/src/routes/query.js (2)
15-60: Implementation aligns well with requirements
The route handler successfully implements the stream extraction functionality with proper validation and error handling. The code is well-structured and maintainable.
3-3: Verify JSON import assertion compatibility
The JSON import assertion syntax (with {type: "json"}) is a newer feature. Ensure your deployment environment supports this syntax or consider providing a fallback.
✅ Verification successful
JSON import assertions are properly supported
The project's configuration in log-viewer-webui-server/package.json explicitly supports JSON import assertions through the @babel/plugin-syntax-import-attributes plugin, and the project is configured as an ES module with "type": "module". The syntax is safe to use in this environment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check package.json for Node.js version requirements
rg -A 5 '"engines"' package.json
Length of output: 87
Script:
#!/bin/bash
# Search for package.json files in the repository
fd -t f "package.json" -x cat {}
Length of output: 5779
components/log-viewer-webui/server/src/DbManager.js (2)
65-65: LGTM: Enum value addition follows established pattern
The addition of EXTRACT_JSON to QUERY_JOB_TYPE maintains consistency with the existing enum structure.
271-274: LGTM: Exports follow best practices
The addition of named exports for EXTRACT_JOB_TYPES and QUERY_JOB_TYPE is appropriate and follows JavaScript best practices.
|
|
||
| let irMetadata = await fastify.dbManager.getExtractedIrFileMetadata( | ||
| origFileId, | ||
| const sanitizedLogEventIdx = Number(logEventIdx); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for logEventIdx parameter
The logEventIdx parameter should be validated before conversion to ensure it's defined.
+ if (undefined === logEventIdx) {
+ resp.code(StatusCodes.BAD_REQUEST);
+ throw new Error("\"logEventIdx\" must be provided.");
+ }
const sanitizedLogEventIdx = Number(logEventIdx);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const sanitizedLogEventIdx = Number(logEventIdx); | |
| if (undefined === logEventIdx) { | |
| resp.code(StatusCodes.BAD_REQUEST); | |
| throw new Error("\"logEventIdx\" must be provided."); | |
| } | |
| const sanitizedLogEventIdx = Number(logEventIdx); |
| async submitAndWaitForExtractStreamJob ({ | ||
| jobType, | ||
| logEventIdx, | ||
| streamId, | ||
| targetUncompressedSize, | ||
| }) { | ||
| let jobConfig; | ||
| if (QUERY_JOB_TYPE.EXTRACT_IR === jobType) { | ||
| jobConfig = { | ||
| file_split_id: null, | ||
| msg_ix: logEventIdx, | ||
| orig_file_id: streamId, | ||
| target_uncompressed_size: targetUncompressedSize, | ||
| }; | ||
| } else if (QUERY_JOB_TYPE.EXTRACT_JSON === jobType) { | ||
| jobConfig = { | ||
| archive_id: streamId, | ||
| target_chunk_size: targetUncompressedSize, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for job type parameter
The method should validate that the provided jobType is one of the valid extract job types defined in EXTRACT_JOB_TYPES.
async submitAndWaitForExtractStreamJob ({
jobType,
logEventIdx,
streamId,
targetUncompressedSize,
}) {
+ if (!EXTRACT_JOB_TYPES.includes(jobType)) {
+ throw new Error(`Invalid job type: ${jobType}. Expected one of: ${EXTRACT_JOB_TYPES}`);
+ }
let jobConfig;
if (QUERY_JOB_TYPE.EXTRACT_IR === jobType) {Committable suggestion skipped: line range outside the PR's diff.
junhaoliao
left a comment
junhaoliao
left a comment
There was a problem hiding this comment.
For the PR tile, how about
feat(webui): Support viewing search results in context for JSON logs (clp-json).
haiqi96
changed the title
…(clp-json). (y-scope#596) Co-authored-by: Junhao Liao <[email protected]>
3 participants
Description
The existing implementation of webui only supports viewing unstructured logs. This PR supports viewing JSON logs in the webui.
With refactor in #584, JSON and IR log viewing share a large portion of common code, except two differences:
This PR replaces the original
IR extraction methodswithstream extraction methodswhich can support both JSON and IR viewing. In addition to the original parameters, they take in an extra "JobType" to distinguish if the job is targeting JSON or IR.Also fixes #608..
Validation performed
Tested package locally and verified that both CLP and CLP-S flow works
Summary by CodeRabbit
Release Notes
New Features
QueryStatuscomponent with new job type handling and improved error messages.SearchResultsTablefor better handling of storage types and URL generation.Bug Fixes
Documentation