Optimize pnl queries #1812
Optimize pnl queries #1812
Conversation
WalkthroughThe recent updates involve refining test cases, database migration scripts, and Pnl ticks handling in various modules of the indexer packages. Tests were adjusted, new migration scripts were added to manage a Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AuxoService
participant Redis
participant PnlTicksTable
Client->>AuxoService: event with onlyRunDbMigrationAndCreateKafkaTopics
AuxoService-->>Client: Return success early
Client->>AuxoService: request PNL ticks for subaccounts
AuxoService->>Redis: getMostRecentPnlTicksForEachAccount
Redis-->>AuxoService: return recent PNL ticks
AuxoService->>PnlTicksTable: findLatestProcessedBlocktimeAndCount
PnlTicksTable-->>AuxoService: return maxBlockTime and count
AuxoService-->>Client: Return PNL ticks and metadata
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- indexer/packages/postgres/tests/stores/pnl-ticks-table.test.ts (1 hunks)
- indexer/packages/postgres/src/db/migrations/migration_files/20240628141851_create_pnl_ticks_block_time_index.ts (1 hunks)
- indexer/packages/postgres/src/stores/pnl-ticks-table.ts (1 hunks)
- indexer/services/roundtable/tests/tasks/pnl-instrumentation.test.ts (4 hunks)
- indexer/services/roundtable/src/tasks/pnl-instrumentation.ts (2 hunks)
Files skipped from review due to trivial changes (1)
- indexer/packages/postgres/tests/stores/pnl-ticks-table.test.ts
Additional comments not posted (6)
indexer/packages/postgres/src/db/migrations/migration_files/20240628141851_create_pnl_ticks_block_time_index.ts (2)
9-13: Ensure the use ofCONCURRENTLYin index dropping.Using
CONCURRENTLYwithDROP INDEXis correct and can help avoid locking issues during the migration. Good use of PostgreSQL features to ensure smoother operations.
15-17: Review transaction configuration.The migration script sets
transaction: false. Ensure this is intentional, as running index operations outside of a transaction can be risky if not handled properly.indexer/services/roundtable/src/tasks/pnl-instrumentation.ts (1)
14-14: Ensure proper usage of helper functions.The import of
getMostRecentPnlTicksForEachAccountsuggests a change in how PNL data is retrieved. Ensure this change is reflected throughout the file and aligns with the project's data handling strategies.indexer/services/roundtable/__tests__/tasks/pnl-instrumentation.test.ts (2)
8-8: Review updated imports and mock setup.The test file has updated imports and a new mock setup for
getMostRecentPnlTicksForEachAccount. Ensure that these changes are necessary and correctly implemented to reflect the updated logic in the main task file.Also applies to: 11-11, 14-14, 16-16
50-61: Ensure test coverage for new logic.The mock implementations for
getMostRecentPnlTicksForEachAccountin various scenarios need to accurately reflect the changes in data handling. Ensure that the test cases cover all new edge cases and behaviors introduced by the updates.Also applies to: 81-92, 117-124
indexer/packages/postgres/src/stores/pnl-ticks-table.ts (1)
223-236: Review the refactored query using CTE.The refactored function
findLatestProcessedBlocktimeAndCountuses a CTE to optimize the query. Ensure that the CTE is correctly formatted and that the query efficiently retrieves the required data.
| export async function up(knex: Knex): Promise<void> { | ||
| await knex.raw(` | ||
| CREATE INDEX CONCURRENTLY IF NOT EXISTS "pnl_ticks_blocktime_index" ON "pnl_ticks" ("blockTime"); | ||
| `); | ||
| } |
There was a problem hiding this comment.
Ensure the use of CONCURRENTLY in index creation.
The use of CONCURRENTLY with IF NOT EXISTS in the CREATE INDEX statement is generally not supported by PostgreSQL. You might need to remove CONCURRENTLY if this causes issues during migration.
- CREATE INDEX CONCURRENTLY IF NOT EXISTS "pnl_ticks_blocktime_index" ON "pnl_ticks" ("blockTime");
+ CREATE INDEX IF NOT EXISTS "pnl_ticks_blocktime_index" ON "pnl_ticks" ("blockTime");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.
| export async function up(knex: Knex): Promise<void> { | |
| await knex.raw(` | |
| CREATE INDEX CONCURRENTLY IF NOT EXISTS "pnl_ticks_blocktime_index" ON "pnl_ticks" ("blockTime"); | |
| `); | |
| } | |
| export async function up(knex: Knex): Promise<void> { | |
| await knex.raw(` | |
| CREATE INDEX IF NOT EXISTS "pnl_ticks_blocktime_index" ON "pnl_ticks" ("blockTime"); | |
| `); | |
| } |
| // Get the most recent PNL ticks for each subaccount from Redis | ||
| const mostRecentPnlTicks: PnlTickForSubaccounts = await getMostRecentPnlTicksForEachAccount(); | ||
| const mostRecentPnlTickTimes: | ||
| { | ||
| [subaccountId: string]: string | ||
| } = await PnlTicksTable.findMostRecentPnlTickTimeForEachAccount( | ||
| '1', | ||
| } = _.mapValues( | ||
| mostRecentPnlTicks, | ||
| (pnlTick: PnlTicksCreateObject) => pnlTick.blockTime, |
There was a problem hiding this comment.
Optimize data retrieval and processing.
The retrieval and processing of PNL data from Redis using getMostRecentPnlTicksForEachAccount and mapping over the results with lodash could be optimized or simplified if performance issues are noted.
- const mostRecentPnlTicks: PnlTickForSubaccounts = await getMostRecentPnlTicksForEachAccount();
- const mostRecentPnlTickTimes:
- {
- [subaccountId: string]: string
- } = _.mapValues(
- mostRecentPnlTicks,
- (pnlTick: PnlTicksCreateObject) => pnlTick.blockTime,
- );
+ const mostRecentPnlTickTimes = await getMostRecentPnlTicksForEachAccount();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.
| // Get the most recent PNL ticks for each subaccount from Redis | |
| const mostRecentPnlTicks: PnlTickForSubaccounts = await getMostRecentPnlTicksForEachAccount(); | |
| const mostRecentPnlTickTimes: | |
| { | |
| [subaccountId: string]: string | |
| } = await PnlTicksTable.findMostRecentPnlTickTimeForEachAccount( | |
| '1', | |
| } = _.mapValues( | |
| mostRecentPnlTicks, | |
| (pnlTick: PnlTicksCreateObject) => pnlTick.blockTime, | |
| // Get the most recent PNL ticks for each subaccount from Redis | |
| const mostRecentPnlTickTimes = await getMostRecentPnlTicksForEachAccount(); |
| @@ -1,15 +1,17 @@ | |||
| import { logger, stats } from '@dydxprotocol-indexer/base'; | |||
| import { | |||
| BlockFromDatabase, | |||
| BlockTable, | |||
| PnlTicksTable, | |||
| BlockTable, PnlTicksCreateObject, | |||
There was a problem hiding this comment.
Tip
Codebase Verification
Review updated imports for consistency and necessity.
The imports for PnlTicksCreateObject and PnlTickForSubaccounts are present in the file but are not used. Please review and remove any unused imports to maintain code cleanliness.
PnlTicksCreateObjectimported at line 4.PnlTickForSubaccountsimported at line 9.
Analysis chain
Review updated imports for consistency and necessity.
The imports for PnlTicksCreateObject and PnlTickForSubaccounts have been added. Verify that these are used appropriately in the file and check for any unused imports.
Also applies to: 9-9
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of newly imported modules.
# Test: Search for usage of `PnlTicksCreateObject` and `PnlTickForSubaccounts` in the file.
ast-grep --lang javascript --pattern $'import { $$$ } from "@dydxprotocol-indexer/postgres"; $$$ PnlTicksCreateObject $$$'
ast-grep --lang javascript --pattern $'import { $$$ } from "@dydxprotocol-indexer/redis"; $$$ PnlTickForSubaccounts $$$'
Length of output: 1253
Script:
#!/bin/bash
# Description: Verify the usage of newly imported modules.
# Test: Search for the import statements of `PnlTicksCreateObject` and `PnlTickForSubaccounts`.
ast-grep --lang typescript --pattern 'import { PnlTicksCreateObject } from "@dydxprotocol-indexer/postgres"' indexer/services/roundtable/src/tasks/pnl-instrumentation.ts
ast-grep --lang typescript --pattern 'import { PnlTickForSubaccounts } from "@dydxprotocol-indexer/redis"' indexer/services/roundtable/src/tasks/pnl-instrumentation.ts
# Test: Search for the usage of `PnlTicksCreateObject` and `PnlTickForSubaccounts` in the file.
ast-grep --lang typescript --pattern 'PnlTicksCreateObject' indexer/services/roundtable/src/tasks/pnl-instrumentation.ts
ast-grep --lang typescript --pattern 'PnlTickForSubaccounts' indexer/services/roundtable/src/tasks/pnl-instrumentation.ts
Length of output: 815
Script:
#!/bin/bash
# Description: Verify the usage of newly imported modules within the file.
# Test: Search for the usage of `PnlTicksCreateObject` in the file.
ast-grep --lang typescript --pattern 'PnlTicksCreateObject' indexer/services/roundtable/src/tasks/pnl-instrumentation.ts
# Test: Search for the usage of `PnlTickForSubaccounts` in the file.
ast-grep --lang typescript --pattern 'PnlTickForSubaccounts' indexer/services/roundtable/src/tasks/pnl-instrumentation.ts
Length of output: 473
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
indexer/services/auxo/src/index.ts (1)
Line range hint
217-468: Optimization suggestion: Remove unnecessary else clauses.The static analysis has identified unnecessary else clauses which can be safely removed to improve code readability.
- else { - throw new Error('No services found'); - } + throw new Error('No services found');- else { - throw new Error('Not all services found'); - } + throw new Error('Not all services found');Aside from this, the functions related to ECS task definition creation and service upgrades are well-implemented.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- indexer/packages/postgres/tests/stores/pnl-ticks-table.test.ts (2 hunks)
- indexer/packages/postgres/src/stores/pnl-ticks-table.ts (2 hunks)
- indexer/services/auxo/src/index.ts (1 hunks)
- indexer/services/auxo/src/types.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- indexer/services/auxo/src/types.ts
Files skipped from review as they are similar to previous changes (2)
- indexer/packages/postgres/tests/stores/pnl-ticks-table.test.ts
- indexer/packages/postgres/src/stores/pnl-ticks-table.ts
Additional context used
Biome
indexer/services/auxo/src/index.ts
[error] 156-158: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
[error] 459-468: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
Additional comments not posted (4)
indexer/services/auxo/src/index.ts (4)
74-81: Early return implementation is correct and clear.This change aligns with the PR objectives to optimize operations by conditionally running tasks based on the event's properties.
Line range hint
86-149: FunctionupgradeBazookais well-implemented with appropriate logging and error handling.The function correctly updates the Lambda function and waits for the update status, providing detailed logging throughout the process.
Line range hint
151-193: FunctiongetImageDetailcorrectly handles image retrieval with comprehensive logging and error management.The implementation ensures that if no image details are found, an error is thrown, which is crucial for preventing further erroneous operations.
Line range hint
195-215: FunctionrunDbAndKafkaMigrationis implemented correctly with detailed logging and appropriate Lambda invocation.The use of the
RequestResponseinvocation type ensures that the function waits for the Lambda execution to complete, which is necessary for synchronous operations.
|
@Mergifyio backport release/indexer/v5.x |
✅ Backports have been created
|
(cherry picked from commit 3c07400) # Conflicts: # indexer/services/auxo/src/types.ts
Co-authored-by: dydxwill <[email protected]> Co-authored-by: Will Liu <[email protected]>
Changelist
Add auxo option to only run db migration. Plan is to manually deploy roundtable first to make sure it's not the culprit.
Add blockTime index to pnl_ticks table. Without index,
EXPLAIN ANALYZE WITH maxBlockTime AS ( SELECT MAX("blockTime") as "maxBlockTime" FROM "pnl_ticks" ) SELECT maxBlockTime."maxBlockTime" as max, COUNT(*) as count FROM "pnl_ticks", maxBlockTime WHERE "pnl_ticks"."blockTime" = maxBlockTime."maxBlockTime" GROUP BY 1;has Execution Time: 14363.724 ms. With index, Execution Time: 0.119 msIn pnl instrumentation task, query the cache since data exists there. This will help reduce db load. Remove unused helper functions
Test Plan
See internal mainnet dashboards for decrease in pnl instrumentation task latency
Unit tested
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit
New Features
blockTimecolumn in thepnl_tickstable for improved query performance.Bug Fixes
Improvements
findLatestProcessedBlocktimeAndCountfor more efficient data retrieval.