Fix profiles worker #2708
Fix profiles worker #2708
Conversation
ILP has some buffer size limit, and we have some activities with very huge body sizes
WalkthroughThe pull request introduces several changes across different files. The Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
services/libs/data-access-layer/src/activities/ilp.ts (2)
208-211: Improve type checking in objectToBytesThe current implementation might allow non-string/non-object types through.
Consider adding more robust type checking:
- if (typeof input !== 'object') { - return input - } + if (typeof input !== 'object' || input === null) { + return typeof input === 'string' ? input : JSON.stringify(input); + }
48-48: Update attribute handling chainThe chaining of
objectToBytes(tryToUnfuckAttributes())could be simplified with the new implementation.Consider:
- .stringColumn('attributes', objectToBytes(tryToUnfuckAttributes(activity.attributes))) + .stringColumn('attributes', objectToBytes(parseAttributes(activity.attributes)))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
services/apps/profiles_worker/src/workflows/member/memberUpdate.ts(1 hunks)services/libs/data-access-layer/src/activities/ilp.ts(3 hunks)services/libs/questdb/src/sql.ts(1 hunks)
🔇 Additional comments (1)
services/apps/profiles_worker/src/workflows/member/memberUpdate.ts (1)
10-10:
Investigate root cause for requiring 60-minute timeout
The timeout increase from 60 seconds to 60 minutes is concerning and might be masking underlying performance issues. While this change may temporarily fix timeout issues, it could lead to:
- Resource exhaustion from long-running operations
- Delayed error detection
- Poor user experience
Consider:
- Investigating why member updates take longer than expected
- Adding performance monitoring
- Implementing partial updates or batching if dealing with large datasets
Let's check for potential performance bottlenecks:
| ;(client as any).query = async (query, options, ...args) => { | ||
| const { replacements } = options || {} | ||
| const timer = telemetry.timer('questdb.query_duration') | ||
| try { | ||
| return oldQuery.apply(client, [query, options]) | ||
| return oldQuery.apply(client, [query, options, ...args]) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for variable arguments
While adding support for variable arguments increases flexibility, it could lead to issues if invalid arguments are passed. Consider:
- Validating args before passing to oldQuery
- Adding type safety for the args parameter
- Documenting expected argument types
Here's a suggested improvement:
- (client as any).query = async (query, options, ...args) => {
+ (client as any).query = async (query: string, options?: Record<string, unknown>, ...args: unknown[]) => {
+ if (args.some(arg => arg === undefined)) {
+ throw new Error('Undefined arguments are not allowed');
+ }
const { replacements } = options || {}
const timer = telemetry.timer('questdb.query_duration')
try {
return oldQuery.apply(client, [query, options, ...args])📝 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.
| ;(client as any).query = async (query, options, ...args) => { | |
| const { replacements } = options || {} | |
| const timer = telemetry.timer('questdb.query_duration') | |
| try { | |
| return oldQuery.apply(client, [query, options]) | |
| return oldQuery.apply(client, [query, options, ...args]) | |
| (client as any).query = async (query: string, options?: Record<string, unknown>, ...args: unknown[]) => { | |
| if (args.some(arg => arg === undefined)) { | |
| throw new Error('Undefined arguments are not allowed'); | |
| } | |
| const { replacements } = options || {} | |
| const timer = telemetry.timer('questdb.query_duration') | |
| try { | |
| return oldQuery.apply(client, [query, options, ...args]) |
| function tryToUnfuckAttributes(attributes: string | object): object { | ||
| // eslint-disable-next-line no-constant-condition | ||
| while (true) { | ||
| if (typeof attributes === 'object') { | ||
| return attributes | ||
| } | ||
| attributes = JSON.parse(attributes) | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Fix infinite loop risk in attribute parsing
The tryToUnfuckAttributes function has several critical issues:
- Infinite loop risk if JSON.parse results in a string
- No error handling for invalid JSON
- Unprofessional function naming
Replace with this safer implementation:
- function tryToUnfuckAttributes(attributes: string | object): object {
- // eslint-disable-next-line no-constant-condition
- while (true) {
- if (typeof attributes === 'object') {
- return attributes
- }
- attributes = JSON.parse(attributes)
- }
- }
+ function parseAttributes(attributes: string | object): object {
+ try {
+ if (typeof attributes === 'object') {
+ return attributes || {};
+ }
+ const parsed = JSON.parse(attributes);
+ return typeof parsed === 'object' ? parsed : {};
+ } catch (error) {
+ return {};
+ }
+ }📝 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.
| function tryToUnfuckAttributes(attributes: string | object): object { | |
| // eslint-disable-next-line no-constant-condition | |
| while (true) { | |
| if (typeof attributes === 'object') { | |
| return attributes | |
| } | |
| attributes = JSON.parse(attributes) | |
| } | |
| } | |
| function parseAttributes(attributes: string | object): object { | |
| try { | |
| if (typeof attributes === 'object') { | |
| return attributes || {}; | |
| } | |
| const parsed = JSON.parse(attributes); | |
| return typeof parsed === 'object' ? parsed : {}; | |
| } catch (error) { | |
| return {}; | |
| } | |
| } |
| @@ -122,7 +122,7 @@ | |||
| } | |||
|
|
|||
| if (activity.body) { | |||
| row.stringColumn('body', activity.body) | |||
| row.stringColumn('body', activity.body.slice(0, 2000)) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add warning for truncated body content
The body field is silently truncated to 2000 characters, which could lead to data loss without notification.
Consider:
- Logging when truncation occurs
- Making the limit configurable
- Preserving important content
- row.stringColumn('body', activity.body.slice(0, 2000))
+ const MAX_BODY_LENGTH = 2000;
+ if (activity.body.length > MAX_BODY_LENGTH) {
+ telemetry.increment('activity.body.truncated', 1);
+ }
+ row.stringColumn('body', activity.body.slice(0, MAX_BODY_LENGTH))Committable suggestion skipped: line range outside the PR's diff.
sausage-todd
force-pushed
the
fix-profiles-worker
branch
from
0f97a54 to
1e945c5
Compare
Summary by CodeRabbit
New Features
Bug Fixes
Documentation