feat: add logging when create channel index #1460
Conversation
WalkthroughThe changes introduce logging and error handling to the Changes
Sequence Diagram(s)sequenceDiagram
participant Repo as OpensearchRepository
participant OS as OpenSearch
participant Log as Logger
Repo->>OS: Attempt index creation (createIndex)
alt Successful Creation
OS-->>Repo: Return success response
Repo->>Log: Log success message with response details
else Error Occurred
OS-->>Repo: Return error response
Repo->>Log: Log error message with details
end
Repo->>OS: Attempt to put mappings (putMappings)
alt Successful Mapping
OS-->>Repo: Return success response
Repo->>Log: Log success message with response details
else Error Occurred
OS-->>Repo: Return error response
Repo->>Log: Log error message with details
end
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
🪧 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 (
|
jihun
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: 2
🧹 Nitpick comments (1)
apps/api/src/common/repositories/opensearch.repository.ts (1)
76-78: Consider adding structured loggingWhile the current logging approach works, consider using a more structured approach for better searchability and filtering in log aggregation tools.
- this.logger.log( - `Index created successfully: ${JSON.stringify(response.body, null, 2)}`, - ); + this.logger.log({ + message: 'Index created successfully', + indexName, + responseBody: response.body, + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/api/src/common/repositories/opensearch.repository.ts(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
apps/api/src/common/repositories/opensearch.repository.ts
[error] 81-81: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 GitHub Check: lint
apps/api/src/common/repositories/opensearch.repository.ts
[failure] 81-81:
Prefer using an optional chain expression instead, as it's more concise and easier to read
🪛 GitHub Actions: CI
apps/api/src/common/repositories/opensearch.repository.ts
[error] 81-81: Prefer using an optional chain expression instead, as it's more concise and easier to read @typescript-eslint/prefer-optional-chain
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: integration-test
🔇 Additional comments (2)
apps/api/src/common/repositories/opensearch.repository.ts (2)
24-24: Great addition of logging capabilities!Adding the Logger import and instantiating it with the class name follows NestJS best practices. This will help with observability and debugging.
Also applies to: 42-42
50-79: Well-structured try-catch implementation for error handlingThe addition of a try-catch block with proper logging of the operation's success is a good improvement to the codebase's observability.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
apps/api/src/common/repositories/opensearch.repository.ts (1)
89-92: 🛠️ Refactor suggestionConsider moving putAlias into the try-catch block
The putAlias operation is outside the try-catch block, which means if index creation succeeds but alias creation fails, the error wouldn't be logged with the same detail as index creation errors.
try { const response = await this.opensearchClient.indices.create({ index: indexName, body: { settings: { // ...existing settings... }, }, }); this.logger.log( `Index created successfully: ${JSON.stringify(response.body, null, 2)}`, ); + + await this.opensearchClient.indices.putAlias({ + index: indexName, + name: index, + }); + this.logger.log(`Alias created successfully for index: ${indexName}, alias: ${index}`); } catch (error) { this.logger.error(`Error creating index: ${error}`); if (error?.meta?.body) { this.logger.error( `OpenSearch error details:${JSON.stringify(error.meta.body, null, 2)}`, ); } throw error; } - await this.opensearchClient.indices.putAlias({ - index: indexName, - name: index, - });
🧹 Nitpick comments (3)
apps/api/src/common/repositories/opensearch.repository.ts (3)
51-88: Comprehensive error handling for index creation, but consider log levelsThe try-catch implementation properly logs both successful operations and errors, but consider using
logger.error()instead oflogger.log()for error conditions to improve log filtering and visibility.} catch (error) { - this.logger.log(`Error creating index: ${error}`); + this.logger.error(`Error creating index: ${error}`); if (error?.meta?.body) { - this.logger.log( + this.logger.error( `OpenSearch error details:${JSON.stringify(error.meta.body, null, 2)}`, ); } throw error; }
101-117: Good implementation of error handling for putMappingsThe putMappings method now properly captures the response, handles errors with appropriate logging, and returns the response to the caller. However, similar to the createIndex method, consider using logger.error for error conditions.
try { response = await this.opensearchClient.indices.putMapping({ index, body: { properties: mappings }, }); } catch (error) { - this.logger.log(`Error put mapping: ${error}`); + this.logger.error(`Error put mapping: ${error}`); if (error?.meta?.body) { - this.logger.log( + this.logger.error( `OpenSearch error details:${JSON.stringify(error.meta.body, null, 2)}`, ); } throw error; }
120-248: Consider adding similar error handling to other methodsFor consistency, consider adding similar try-catch blocks with logging to other methods that interact with OpenSearch, such as createData, updateData, deleteIndex, etc. This would provide a more comprehensive logging solution across the repository.
For example, for the createData method:
async createData({ id, index, data }: CreateDataDto) { const indexName = 'channel_' + index; - const existence = await this.opensearchClient.indices.exists({ - index: indexName, - }); - if (existence.statusCode !== 200) - throw new NotFoundException('index is not found'); - - const response = await this.opensearchClient.indices.getMapping({ - index: indexName, - }); + try { + const existence = await this.opensearchClient.indices.exists({ + index: indexName, + }); + if (existence.statusCode !== 200) + throw new NotFoundException('index is not found'); + + const response = await this.opensearchClient.indices.getMapping({ + index: indexName, + }); + + const mappingKeys = Object.keys( + response.body[indexName].mappings.properties as object, + ); + const dataKeys = Object.keys(data); + if (!dataKeys.every((v) => mappingKeys.includes(v))) { + throw new InternalServerErrorException('error!!!'); + } + + const { body } = await this.opensearchClient.index({ + id, + index: indexName, + body: data, + refresh: true, + }); + + this.logger.log(`Data created successfully for index: ${indexName}, id: ${id}`); + return { id: body._id as unknown as number }; + } catch (error) { + this.logger.error(`Error creating data: ${error}`); + if (error?.meta?.body) { + this.logger.error( + `OpenSearch error details:${JSON.stringify(error.meta.body, null, 2)}`, + ); + } + throw error; + } - const mappingKeys = Object.keys( - response.body[indexName].mappings.properties as object, - ); - const dataKeys = Object.keys(data); - if (!dataKeys.every((v) => mappingKeys.includes(v))) { - throw new InternalServerErrorException('error!!!'); - } - - const { body } = await this.opensearchClient.index({ - id, - index: indexName, - body: data, - refresh: true, - }); - - return { id: body._id as unknown as number }; }Apply similar patterns to other methods that interact with OpenSearch for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/api/src/common/repositories/opensearch.repository.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: integration-test
🔇 Additional comments (3)
apps/api/src/common/repositories/opensearch.repository.ts (3)
24-24: Appropriate imports added for new functionalityThe added imports for
LoggerandIndices_PutMapping_Responseare correctly implemented to support the new logging functionality and type safety.Also applies to: 28-28
43-43: Good implementation of the LoggerThe private logger instance is properly initialized with the class name as context, following NestJS best practices for logging.
82-82: Great use of optional chainingThe implementation now uses optional chaining for checking error metadata, which improves code readability and addresses previous review comments.
Also applies to: 109-109
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/api/src/common/repositories/opensearch.repository.ts (1)
91-94: Consider adding try-catch for putAlias operation.For consistency with the error handling pattern implemented for index creation and mapping operations, consider wrapping the putAlias operation in a try-catch block with similar logging.
- await this.opensearchClient.indices.putAlias({ - index: indexName, - name: index, - }); + try { + await this.opensearchClient.indices.putAlias({ + index: indexName, + name: index, + }); + this.logger.log(`Alias created successfully for index: ${indexName}, alias: ${index}`); + } catch (error) { + this.logger.log(`Error creating alias: ${error}`); + if (error?.meta?.body) { + this.logger.log( + `OpenSearch error details:${JSON.stringify(error.meta.body, null, 2)}`, + ); + } + throw error; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/api/src/common/repositories/opensearch.repository.ts(3 hunks)
🧰 Additional context used
🪛 GitHub Check: lint
apps/api/src/common/repositories/opensearch.repository.ts
[failure] 77-77:
Unnecessary conditional, value is always truthy
🪛 GitHub Actions: CI
apps/api/src/common/repositories/opensearch.repository.ts
[error] 77-77: Unnecessary conditional, value is always truthy @typescript-eslint/no-unnecessary-condition
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: integration-test
🔇 Additional comments (8)
apps/api/src/common/repositories/opensearch.repository.ts (8)
24-24: Good import addition for logging functionality.Adding the Logger import is essential for enabling logging capabilities in the repository.
28-28: Good type import for better type safety.Using the specific
Indices_PutMapping_Responsetype enhances type safety when working with OpenSearch responses.
43-43: Proper logger initialization with class context.Initializing the logger with the class name follows NestJS best practices and helps with log filtering and organization.
51-76: Well-structured try-catch block with detailed logging.The implementation properly wraps the index creation operation in a try-catch block with comprehensive logging.
82-90: Comprehensive error logging with proper error propagation.The error logging captures both the error message and detailed OpenSearch error information. Re-throwing the error after logging is the correct approach to allow proper error handling up the call stack.
103-108: Well-typed response variable with proper try-catch structure.The response variable is properly typed with
Indices_PutMapping_Responseand the mapping operation is correctly wrapped in a try block.
109-117: Good error handling with detailed logging.The catch block provides comprehensive error logging for putMapping failures, capturing both the error message and detailed error information from OpenSearch.
119-119: Good improvement to return the response.Returning the response from the putMappings method allows callers to access the operation results if needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
apps/api/src/common/repositories/opensearch.repository.ts (1)
51-96: 🛠️ Refactor suggestionConsider consistent error handling across the repository
You've added error handling for
createIndexandputMappings, but other methods likecreateData,getData,scroll, etc. don't have similar error handling patterns with logging.Consider adding similar try-catch blocks with logging to all operations that interact with OpenSearch for consistency and better observability.
What are best practices for error handling in NestJS applications?
♻️ Duplicate comments (1)
apps/api/src/common/repositories/opensearch.repository.ts (1)
51-91:⚠️ Potential issueError handling still has an issue with control flow
While you've correctly added try-catch with proper logging and error re-throwing, the
putAliasoperation (lines 92-95) remains outside the try-catch block. If index creation fails, the code will still attempt to executeputAliasand likely fail again.Consider one of these approaches:
async createIndex({ index }: CreateIndexDto) { const indexName = 'channel_' + index; try { const response = await this.opensearchClient.indices.create({ index: indexName, body: { // ... existing settings ... }, }); // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition if (response) { this.logger.log( `Index created successfully: ${JSON.stringify(response.body, null, 2)}`, ); } + + await this.opensearchClient.indices.putAlias({ + index: indexName, + name: index, + }); + this.logger.log(`Alias created successfully for index: ${indexName}, alias: ${index}`); } catch (error) { this.logger.log(`Error creating index: ${error}`); if (error?.meta?.body) { this.logger.log( `OpenSearch error details:${JSON.stringify(error.meta.body, null, 2)}`, ); } throw error; } - await this.opensearchClient.indices.putAlias({ - index: indexName, - name: index, - }); }
🧹 Nitpick comments (2)
apps/api/src/common/repositories/opensearch.repository.ts (2)
78-82: Remove unnecessary conditional checkThe condition
if (response)is redundant. If the OpenSearch operation fails, it will throw an exception caught by the catch block. The response will always be truthy at this point.- // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition - if (response) { this.logger.log( `Index created successfully: ${JSON.stringify(response.body, null, 2)}`, ); - }
92-95: Add logging for putAlias operationCurrently, you've added logging for index creation but not for the alias creation. For consistency, you should also log the result of the
putAliasoperation.await this.opensearchClient.indices.putAlias({ index: indexName, name: index, }); + this.logger.log(`Alias created successfully for index: ${indexName}, alias: ${index}`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api/package.json(1 hunks)apps/api/src/common/repositories/opensearch.repository.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: integration-test
🔇 Additional comments (4)
apps/api/package.json (1)
4-4:❓ Verification inconclusive
Project type updated to ESM: Consider the implications
Adding
"type": "module"changes the project from CommonJS to ECMAScript Modules. This is a significant architectural change that affects how modules are imported and exported throughout the codebase.
🏁 Script executed:
#!/bin/bash # Check for potential ESM compatibility issues by looking for CommonJS patterns echo "Checking for CommonJS require statements that need to be converted to import..." rg "require\(" --type ts echo "Checking for module.exports patterns that need to be converted to export..." rg "module\.exports" --type ts echo "Checking for dynamic imports that might need special handling in ESM..." rg "require\.resolve" --type tsLength of output: 850
ESM Transition: Verify and Update CommonJS Usage
The addition of
"type": "module"inapps/api/package.jsonclearly marks the project as ESM, which impacts how modules are imported/exported. However, our verification found several CommonJS patterns in the codebase that could lead to runtime issues under ESM:
Configuration and Setup Files:
apps/web/jest.setup.tscontains arequire('resize-observer-polyfill')which might need to be replaced with an ESM-compatible alternative or handled via dynamic import.apps/web/tailwind.config.tsusesrequire('@ufb/tailwindcss')andrequire('tailwind-scrollbar-hide')— ensure these usages are either safe in an ESM context or updated accordingly.Action Required:
Please verify that these instances are intentionally left as-is (for compatibility with specific tooling) or update them to use ESM syntax. Review how these files are executed (e.g., via bundlers or Node with interop settings) to avoid unexpected issues.apps/api/src/common/repositories/opensearch.repository.ts (3)
24-24: LGTM: Good addition of LoggerAdding a Logger instance is a good practice for tracking operations in the repository. This aligns with the PR objective of adding logging when creating channel indexes.
Also applies to: 43-43
28-28: LGTM: Proper typing for the responseAdding the explicit type import for
Indices_PutMapping_Responseimproves type safety in the codebase.
104-120: LGTM: Good implementation of error handling and response handlingThe implementation for
putMappingsnow correctly:
- Captures and logs errors with detailed information
- Properly types and returns the response
- Re-throws errors after logging them
This allows callers to handle errors appropriately and provides valuable debugging information.
Summary by CodeRabbit