feat(log-viewer-webui): Refactor S3Manager into a Fastify plugin. #689
Conversation
WalkthroughThe pull request modifies the S3 management functionality in the log viewer web UI by updating 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 (
|
haiqi96
changed the title
haiqi96
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
components/log-viewer-webui/server/src/S3Manager.js (1)
25-31: Handle potential undefinedregionparameterIn the constructor, consider handling the case where
regionmight beundefined. Currently, ifregionisundefined,null !== regionevaluates totrue, which could lead to unintended behaviour when initializingS3Clientwith anundefinedregion.Apply this diff to ensure
regionis notundefined:constructor ({region}) { - if (null !== region) { + if (null !== region && undefined !== region) { this.#s3Client = new S3Client({ region: region, }); } }Alternatively, you can use a more concise check:
constructor ({region}) { - if (null !== region) { + if (region) { this.#s3Client = new S3Client({ region: region, }); } }components/log-viewer-webui/server/src/routes/query.js (2)
11-13: Improve JSDoc parameter type annotationsThe use of the union type
|in the JSDoc comments may not accurately represent the structure of thefastifyobject, which includes all listed properties. Consider using the intersection type&to indicate thatfastifycontains all specified properties.Apply this diff to adjust the JSDoc annotations:
/** * @param {object} props - * @param {import("fastify").FastifyInstance | - * {dbManager: DbManager} | - * {s3Manager: S3Manager}} props.fastify + * @param {import("fastify").FastifyInstance & + * {dbManager: DbManager} & + * {s3Manager: S3Manager}} props.fastify * @param {EXTRACT_JOB_TYPES} props.jobType * @param {number} props.logEventIdx * @param {string} props.streamId
59-61: Adjust JSDoc annotations for accurate typingSimilar to the earlier comment, update the parameter type annotations to reflect that
fastifyincludes all the specified properties.Apply this diff:
/** * @param {import("fastify").FastifyInstance & * {dbManager: DbManager} & * {s3Manager: S3Manager}} fastify * @param {import("fastify").FastifyPluginOptions} options * @return {Promise<void>} */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/log-viewer-webui/server/src/S3Manager.js(4 hunks)components/log-viewer-webui/server/src/app.js(2 hunks)components/log-viewer-webui/server/src/routes/query.js(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
components/log-viewer-webui/server/src/app.js (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/log-viewer-webui/server/src/routes/query.js (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/log-viewer-webui/server/src/S3Manager.js (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint-check (macos-latest)
🔇 Additional comments (7)
components/log-viewer-webui/server/src/S3Manager.js (5)
19-19: Initialization of private field#s3ClientProperly initializing
#s3Clienttonullenhances code safety by ensuring the variable has a known state before use.
22-23: Clarify constructor parameter documentationThe JSDoc comments accurately reflect the updated constructor parameters, improving code readability and maintainability.
33-34:isEnabledmethod implementation looks goodThe
isEnabledmethod correctly checks the initialization state of#s3Client, enhancing the class's usability.
45-47: Usage offalse === <expression>complies with coding guidelinesThe condition
if (false === this.isEnabled())aligns with the project's coding standards.
68-70: Proper use of Fastify plugin to registerS3ManagerExporting
S3Manageras a Fastify plugin and decorating it onto the app instance improves modularity and follows best practices.components/log-viewer-webui/server/src/app.js (2)
9-9: ImportingS3ManagerThe import statement correctly brings in the
S3Managerplugin for registration.
45-45: RegisteringS3Managerwith appropriate optionsRegistering
S3Managerwith theregionoption ensures proper configuration. This aligns with previous review suggestions and enhances code clarity.
| if (fastify.s3Manager.isEnabled()) { | ||
| streamMetadata.path = await fastify.s3Manager.getPreSignedUrl( | ||
| `s3://${settings.StreamFilesS3PathPrefix}${streamMetadata.path}` | ||
| ); | ||
| } else { | ||
| streamMetadata.path = `/streams/${streamMetadata.path}`; |
There was a problem hiding this comment.
Ensure s3Manager is defined before use
Accessing fastify.s3Manager without checking if it is defined could lead to runtime errors, especially in environments where s3Manager is not registered (e.g., when NODE_ENV === "test"). Modify the condition to check for the existence of s3Manager before calling isEnabled().
Apply this diff to prevent potential null reference errors:
-if (fastify.s3Manager.isEnabled()) {
+if (fastify.s3Manager && fastify.s3Manager.isEnabled()) {
streamMetadata.path = await fastify.s3Manager.getPreSignedUrl(
`s3://${settings.StreamFilesS3PathPrefix}${streamMetadata.path}`
);
} else {
streamMetadata.path = `/streams/${streamMetadata.path}`;
}📝 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.
| if (fastify.s3Manager.isEnabled()) { | |
| streamMetadata.path = await fastify.s3Manager.getPreSignedUrl( | |
| `s3://${settings.StreamFilesS3PathPrefix}${streamMetadata.path}` | |
| ); | |
| } else { | |
| streamMetadata.path = `/streams/${streamMetadata.path}`; | |
| if (fastify.s3Manager && fastify.s3Manager.isEnabled()) { | |
| streamMetadata.path = await fastify.s3Manager.getPreSignedUrl( | |
| `s3://${settings.StreamFilesS3PathPrefix}${streamMetadata.path}` | |
| ); | |
| } else { | |
| streamMetadata.path = `/streams/${streamMetadata.path}`; |
| @@ -55,4 +65,6 @@ class S3Manager { | |||
| } | |||
| } | |||
|
|
|||
| export default S3Manager; | |||
| export default fastifyPlugin(async (app, options) => { | |||
| await app.decorate("s3Manager", new S3Manager(options)); | |||
There was a problem hiding this comment.
Sorry for overlooking this enablement check mechanism when we discussed this offline. Some changes can be made to make the code even cleaner, if you also agree:
- Let's treat this callback provided to
fastifyPlugin()as a factory function of class S3Manager and perform all argument checks inside. - If any such checks fail, we don't decorate the Fastify app. Consider adding an error log / throwing an error so developers become aware of this.
- That way we can (hopefully) ensure all arguments passed to S3Manager are valid and do not cause the constructor to crash, and we can also guarantee the
s3Clientis always valid within the scope of the class (no need to doisEnabledchecks). - Most importantly, we can make the query route code unaware of any S3Manager's exact implementation, by checking
fastify.hasDecorator('s3Manager')instead offastify.s3Manager.isEnabled()
There was a problem hiding this comment.
For 1, can you give a more concrete example?
Do you mean instead of writing new S3Manager() as the second argument, pass in a function that
- Takes in
optionsas argument - If argument verification passes, return calls S3Manager constructor and return a
new S3Manager()instance - If fails, throw an error, and does it need to return anythinig?
There was a problem hiding this comment.
right, let's do something like
export default fastifyPlugin(async (app, options) => {
const {xxx} = options;
if (xxx is invalid) {
// if throwing halts the server (please help confirm), we shall require users (developers who register this plugin) to validate the args.
// or simply, instead, `console.error("error message")` then `return`
throw new Error("error message");
}
await app.decorate("s3Manager", new S3Manager(options));
}There was a problem hiding this comment.
I have tried throwing an error, and the entire container is down, with the following log
2025-01-29 15:00:07 Error: I will throw an error message
2025-01-29 15:00:07 at file:///opt/clp/var/www/log_viewer_webui/server/src/S3Manager.js:63:15
2025-01-29 15:00:07 at Plugin.exec (/opt/clp/var/www/log_viewer_webui/server/node_modules/avvio/lib/plugin.js:125:28)
2025-01-29 15:00:07 at Boot._loadPlugin (/opt/clp/var/www/log_viewer_webui/server/node_modules/avvio/boot.js:432:10)
2025-01-29 15:00:07 at process.processTicksAndRejections (node:internal/process/task_queues:82:21)
now I think about it, actually we cannot throw an error, because there will be valid case where region is null, simply meaning that s3Manager is not enabled.
So I guess what we should do is:
If region is null, return without doing anything
else, initialize s3 plugin
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
components/log-viewer-webui/server/src/S3Manager.js (1)
63-63: Use structured logging for initializationConsider using a more structured logging approach for better consistency and machine readability.
- console.log(`s3Manager initialized with region ${region}`) + app.log.info({region}, 's3Manager initialized')components/log-viewer-webui/server/src/routes/query.js (1)
94-94: Follow coding guidelines for boolean expressionsPer coding guidelines, prefer
false ==over!for boolean expressions.- if (fastify.hasDecorator('s3Manager')) { + if (false === fastify.hasDecorator('s3Manager')) { + streamMetadata.path = `/streams/${streamMetadata.path}`; + } else { + streamMetadata.path = await fastify.s3Manager.getPreSignedUrl( + `s3://${settings.StreamFilesS3PathPrefix}${streamMetadata.path}` + ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
components/log-viewer-webui/server/src/S3Manager.js(3 hunks)components/log-viewer-webui/server/src/routes/query.js(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
components/log-viewer-webui/server/src/S3Manager.js (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/log-viewer-webui/server/src/routes/query.js (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: build (macos-latest)
- GitHub Check: lint-check (macos-latest)
🔇 Additional comments (3)
components/log-viewer-webui/server/src/S3Manager.js (2)
64-64: Pass complete options object to constructorAs discussed previously, pass the complete options object to allow for future extensibility.
- await app.decorate("s3Manager", new S3Manager(region)); + await app.decorate("s3Manager", new S3Manager(options));
24-24: 🛠️ Refactor suggestionUpdate constructor signature to match previous agreement
Based on the previous discussion, the constructor should accept an options object instead of a direct region parameter.
Apply this diff to align with the agreed approach:
- constructor (region) { + constructor ({region}) {Also update the JSDoc to reflect this change:
- * @param {string} region + * @param {object} props + * @param {string | null} props.regionLikely invalid or redundant comment.
components/log-viewer-webui/server/src/routes/query.js (1)
11-13: LGTM! Type definition updated correctlyThe JSDoc has been properly updated to include the s3Manager type in the FastifyInstance union type.
| if (fastify.hasDecorator('s3Manager')) { | ||
| streamMetadata.path = await fastify.s3Manager.getPreSignedUrl( | ||
| `s3://${settings.StreamFilesS3PathPrefix}${streamMetadata.path}` | ||
| ); | ||
| } else { | ||
| streamMetadata.path = `/streams/${streamMetadata.path}`; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for getPreSignedUrl
The getPreSignedUrl operation can fail and throw errors. These should be handled gracefully.
if (fastify.hasDecorator('s3Manager')) {
- streamMetadata.path = await fastify.s3Manager.getPreSignedUrl(
- `s3://${settings.StreamFilesS3PathPrefix}${streamMetadata.path}`
- );
+ try {
+ streamMetadata.path = await fastify.s3Manager.getPreSignedUrl(
+ `s3://${settings.StreamFilesS3PathPrefix}${streamMetadata.path}`
+ );
+ } catch (error) {
+ resp.code(StatusCodes.INTERNAL_SERVER_ERROR);
+ throw new Error(`Failed to generate pre-signed URL: ${error.message}`);
+ }
} else {
streamMetadata.path = `/streams/${streamMetadata.path}`;
}📝 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.
| if (fastify.hasDecorator('s3Manager')) { | |
| streamMetadata.path = await fastify.s3Manager.getPreSignedUrl( | |
| `s3://${settings.StreamFilesS3PathPrefix}${streamMetadata.path}` | |
| ); | |
| } else { | |
| streamMetadata.path = `/streams/${streamMetadata.path}`; | |
| if (fastify.hasDecorator('s3Manager')) { | |
| try { | |
| streamMetadata.path = await fastify.s3Manager.getPreSignedUrl( | |
| `s3://${settings.StreamFilesS3PathPrefix}${streamMetadata.path}` | |
| ); | |
| } catch (error) { | |
| resp.code(StatusCodes.INTERNAL_SERVER_ERROR); | |
| throw new Error(`Failed to generate pre-signed URL: ${error.message}`); | |
| } | |
| } else { | |
| streamMetadata.path = `/streams/${streamMetadata.path}`; |
junhaoliao
changed the title
|
I modified the PR title directly. |
Co-authored-by: Junhao Liao <[email protected]>
| } | ||
|
|
||
| console.log(`Initializing S3Manager with region="${region}"...`); | ||
| await app.decorate("s3Manager", new S3Manager(region)); |
There was a problem hiding this comment.
Should we add another log after the decorate is done? Just for debugging purpose.
@junhaoliao
There was a problem hiding this comment.
i think for now it's fine not to log after init completion, since we don't expect constructor to hang. In cases where the constructor throws, we shall be notified for any exception messages.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
components/log-viewer-webui/server/src/S3Manager.js (1)
63-66: Consider adding error logging for invalid options.While the null check is correct, adding error logs would help developers understand why the S3Manager wasn't initialized.
Apply this diff to improve error handling:
const {region} = options; if (null === region) { + console.log("S3Manager initialization skipped: region is null"); return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/log-viewer-webui/server/src/S3Manager.js(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
components/log-viewer-webui/server/src/S3Manager.js (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint-check (ubuntu-latest)
🔇 Additional comments (3)
components/log-viewer-webui/server/src/S3Manager.js (3)
1-1: LGTM! The import statement is correctly added.The import of
fastifyPluginis necessary for implementing the Fastify plugin architecture.
24-24: Use object destructuring for constructor parameters.Previous discussions in the PR indicate that object destructuring is preferred for better extensibility.
Apply this diff to implement the agreed-upon pattern:
- constructor (region) { + constructor ({region}) {
58-70: LGTM! The plugin implementation follows best practices.The implementation includes proper JSDoc comments, null checks, logging, and app decoration. The use of
null === regionfollows the coding guidelines.
Description
This PR refactors the S3Manager into a fastify plugin, which allows as to avoid initializing S3 Manager as a global variable.
Validation performed
Manually tested FS and S3 stream viewing.
Summary by CodeRabbit
Release Notes
New Features
Improvements
These updates provide more flexible and reliable file management when working with S3 storage.