-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WEB-2001] feat: Fix local cache issues r4 #5726
Conversation
- Add IS null where condition in query utils
WalkthroughThe pull request introduces enhancements to the database operations within the application, focusing on integrating Sentry for performance monitoring and error tracking. Key methods in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (5)
web/core/local-db/utils/utils.ts (1)
13-19
: LGTM: NewlogError
function enhances error handlingThe new
logError
function significantly improves error handling by integrating Sentry for exception tracking and providing special handling for SQLite3Errors. This aligns well with the PR objective of addressing local cache issues.Consider adding a severity level parameter to the
logError
function. This would allow for more granular control over which errors are sent to Sentry versus just logged locally. For example:export const logError = (e: any, severity: Sentry.Severity = Sentry.Severity.Error) => { if (e?.result?.errorClass === "SQLite3Error") { e = parseSQLite3Error(e); } Sentry.captureException(e, { level: severity }); console.log(e); };This way, you can call
logError(error, Sentry.Severity.Warning)
for less critical errors.web/core/local-db/utils/query.utils.ts (1)
333-333
: Approved: Unused parameter removed. Consider using template literals for consistency.The removal of the unused
index
parameter from themap
function is a good cleanup. This change improves code readability without affecting functionality.For consistency with modern JavaScript practices and to improve readability, consider using a template literal instead of string concatenation. Here's a suggested improvement:
- const sql = ` ${keys.map((key) => `i.${key}`).join(`, - `)}`; + const sql = keys.map((key) => ` i.${key}`).join(',\n');This change would make the code more consistent with template literal usage and improve readability by explicitly showing the newline character.
web/core/services/issue/issue.service.ts (1)
73-76
: LGTM: Sentry tracing added correctly. Consider enhancing error handling.The Sentry tracing has been implemented correctly:
- The
startSpan
method is used appropriately to wrap thepersistence.getIssues
call.- The span name "GET_ISSUES" is clear and descriptive.
- The overall functionality of the method remains unchanged.
Consider enhancing the error handling within the Sentry span. You could catch any errors, log them with Sentry, and then rethrow. This would provide more detailed error tracking. Here's a suggested implementation:
async getIssues(workspaceSlug: string, projectId: string, queries?: any, config = {}): Promise<TIssuesResponse> { const response = await Sentry.startSpan({ name: "GET_ISSUES" }, async (span) => { try { const res = await persistence.getIssues(workspaceSlug, projectId, queries, config); return res; } catch (error) { span.setStatus("error"); Sentry.captureException(error); throw error; } }); return response as TIssuesResponse; }This change would:
- Catch any errors that occur within the span.
- Set the span status to "error" when an exception occurs.
- Log the error to Sentry for better visibility.
- Rethrow the error to maintain the current error handling behavior.
web/core/local-db/storage.sqlite.ts (2)
309-312
: Capture exceptions with Sentry for better error monitoringIn the catch block of the
getIssues
method:logError(e); const issueService = new IssueService(); return await issueService.getIssuesFromServer(workspaceSlug, projectId, queries);Recommendation:
- Use
Sentry.captureException(e);
to report the exception to Sentry. This enhances error tracking and allows for better monitoring and alerting.Updated code snippet:
logError(e); + Sentry.captureException(e); const issueService = new IssueService(); return await issueService.getIssuesFromServer(workspaceSlug, projectId, queries);
389-389
: Assess the impact of logging in high-frequency methodsAt line 389:
logError(err);Concern:
- The
getIssue
method might be called frequently. Logging errors every time could impact performance and clutter logs.Recommendation:
- Evaluate the necessity of logging at this point.
- Implement conditional logging or rate limiting to prevent log flooding.
- Consider logging only critical errors that require immediate attention.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- web/core/local-db/storage.sqlite.ts (12 hunks)
- web/core/local-db/utils/query.utils.ts (1 hunks)
- web/core/local-db/utils/utils.ts (3 hunks)
- web/core/services/issue/issue.service.ts (2 hunks)
🔇 Additional comments (6)
web/core/local-db/utils/utils.ts (2)
1-1
: LGTM: Sentry import added for error trackingThe addition of Sentry for error tracking aligns well with the PR objective of improving error handling and logging. This will enhance our ability to monitor and debug issues in production.
Line range hint
1-154
: Overall assessment: Changes align well with PR objectiveThe modifications to
utils.ts
successfully address the PR objective of improving error handling for local cache issues. The introduction of Sentry for error tracking and the newlogError
andparseSQLite3Error
functions significantly enhance the application's ability to capture, log, and analyze errors, particularly those related to SQLite operations.While the core functionality of the file remains intact, these additions provide a more robust error handling mechanism, which should aid in identifying and resolving local cache issues more effectively.
The suggested improvements to
logError
andparseSQLite3Error
functions, if implemented, would further refine the error handling process, making it more flexible and resilient.These changes are a positive step towards improving the reliability and debuggability of the local caching system.
web/core/local-db/utils/query.utils.ts (1)
333-333
: Verify relevance to PR objectives and check for additional changes.While this change improves code quality, it doesn't directly address the PR objectives of fixing local cache issues or implementing a fallback mechanism to the server when a query fails.
Please confirm:
- Are there other changes in different files that more directly address the PR objectives?
- How does this specific change contribute to fixing local cache issues or implementing the fallback mechanism?
If this is part of a larger set of changes, it would be helpful to see the context of how this file interacts with the local cache and server fallback mechanism. Consider running the following script to find related changes:
web/core/services/issue/issue.service.ts (2)
1-1
: LGTM: Sentry import added correctly.The Sentry import has been added at the top of the file, which is the correct location. This import is necessary for the Sentry tracing implemented in the
getIssues
method.
Line range hint
1-76
: Summary: Changes align with PR objectives and improve observability.The modifications to the
IssueService
class, specifically thegetIssues
method, align well with the PR objectives of addressing local cache issues. The addition of Sentry tracing provides enhanced observability, which will be crucial for monitoring and debugging cache-related problems.Key points:
- Sentry import added correctly.
getIssues
method now includes Sentry tracing without altering its core functionality.- The changes support the implementation of a fallback mechanism to the server when a query fails, as mentioned in the PR description.
These improvements will contribute to better reliability and easier troubleshooting of data retrieval issues.
web/core/local-db/storage.sqlite.ts (1)
292-294
: Verify the condition for logging fallback to serverAt lines 292-294:
if (rootStore.user.localDBEnabled) { logInfo(`Project ${projectId} is loading, falling back to server`); }Concern:
- The log message indicates falling back to the server, but the condition checks if
localDBEnabled
istrue
. This might be confusing, as you would typically fall back to the server when local DB is not enabled.Recommendation:
- Verify whether the condition should be
!rootStore.user.localDBEnabled
instead.- If the condition is correct, consider updating the log message for clarity.
const parseSQLite3Error = (error: any) => { | ||
error.result = JSON.stringify(error.result); | ||
return error; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Improve error parsing and handling in parseSQLite3Error
While the parseSQLite3Error
function serves its purpose of converting the error result to a JSON string, there are a few improvements that could make it more robust:
- Avoid mutating the original error object.
- Handle potential JSON.stringify errors.
- Preserve the original error information.
Consider refactoring the function as follows:
const parseSQLite3Error = (error: any): any => {
try {
const parsedError = {
...error,
result: typeof error.result === 'object' ? JSON.stringify(error.result) : error.result
};
return parsedError;
} catch (jsonError) {
console.warn('Failed to stringify SQLite3Error result:', jsonError);
return {
...error,
result: 'Failed to stringify error result'
};
}
};
This version:
- Creates a new error object instead of mutating the original.
- Only stringifies the result if it's an object.
- Handles potential JSON.stringify errors.
- Preserves all original error properties.
@@ -67,7 +68,7 @@ export class Storage { | |||
this.reset(); | |||
} | |||
try { | |||
await this._initialize(workspaceSlug); | |||
await Sentry.startSpan({ name: "INIT_DB" }, async () => await this._initialize(workspaceSlug)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent usage of Sentry.startSpan
The Sentry.startSpan
method is used inconsistently across the codebase:
-
Line 71:
await Sentry.startSpan({ name: "INIT_DB" }, async () => await this._initialize(workspaceSlug));
Used with
await
and anasync
callback. -
Lines 152-154:
await Sentry.startSpan({ name: "LOAD_WS", attributes: { slug: this.workspaceSlug } }, async () => { await loadWorkSpaceData(this.workspaceSlug); });
Similar usage with
await
and anasync
callback. -
Line 179:
const sync = Sentry.startSpan({ name: `SYNC_ISSUES` }, () => this._syncIssues(projectId));
Here,
Sentry.startSpan
is called withoutawait
, and the result is assigned toconst sync
.
Recommendations:
- Ensure consistent usage of
Sentry.startSpan
. Decide whether toawait
the function call and whether to use anasync
callback consistently. - Verify that
Sentry.startSpan
is the correct method to use. According to the Sentry SDK documentation, you might need to useSentry.startTransaction
or create a span from an existing transaction. - Check if
Sentry.startSpan
accepts a callback function. If not, you may need to manually start and finish the span.
Also applies to: 152-154, 179-179
const activeSpan = Sentry.getActiveSpan(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential issues with active span management
In several places, you retrieve the active span and set attributes:
-
Lines 189-190:
const activeSpan = Sentry.getActiveSpan();
-
Lines 251-254:
activeSpan?.setAttributes({ projectId: projectId, count: response.total_count, });
-
Lines 366-376:
const activeSpan = Sentry.getActiveSpan(); activeSpan?.setAttributes({ projectId, count: total_count, groupBy: group_by, subGroupBy: sub_group_by, queries: queries, local: true, groupCount, subGroupCount, });
Concerns:
- If there is no active span,
activeSpan
will beundefined
, and the attributes won't be set. This could lead to missing data in your tracing. - Setting attributes after asynchronous operations might not associate the data with the correct span.
Recommendations:
- Ensure that an active span exists before attempting to set attributes.
- Consider passing the span explicitly or using span contexts to maintain consistency.
- Verify that setting attributes at these points correctly captures the intended data.
Also applies to: 251-254, 366-376
let issuesRaw: any[] = []; | ||
let count: any[]; | ||
try { | ||
[issuesRaw, count] = await Promise.all([runQuery(query), runQuery(countQuery)]); | ||
} catch (e) { | ||
logError(e); | ||
const issueService = new IssueService(); | ||
return await issueService.getIssuesFromServer(workspaceSlug, projectId, queries); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Enhance user feedback when falling back to server due to query failure
In the getIssues
method:
try {
[issuesRaw, count] = await Promise.all([runQuery(query), runQuery(countQuery)]);
} catch (e) {
logError(e);
const issueService = new IssueService();
return await issueService.getIssuesFromServer(workspaceSlug, projectId, queries);
}
Recommendation:
- Provide user feedback when falling back to the server. Inform the user that local data retrieval failed, and the server is being used instead.
- This can help with debugging and improves transparency.
Example:
logError(e);
+ logInfo('Local query failed, fetching issues from server');
const issueService = new IssueService();
return await issueService.getIssuesFromServer(workspaceSlug, projectId, queries);
Fallback to server when the query fails.
Summary by CodeRabbit