[IND-557] fix occasional missing pnl ticks #951
Conversation
WalkthroughThe recent update involves refactoring the PnL (Profit and Loss) ticks calculation logic within a service related to the DYdX protocol indexer. A new function, Changes
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- indexer/services/roundtable/tests/helpers/pnl-ticks-helper.test.ts (3 hunks)
- indexer/services/roundtable/src/helpers/pnl-ticks-helper.ts (4 hunks)
Additional comments: 4
indexer/services/roundtable/src/helpers/pnl-ticks-helper.ts (2)
163-185: The new function
getAccountsToUpdatecorrectly filters accounts based on the last updated block time and the configured update interval. It's important to ensure thatconfig.PNL_TICK_UPDATE_INTERVAL_MSis properly set and that the 0.9 multiplier is intentional and consistent with the business logic.62-68: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [6-162]
In the
getPnlTicksCreateObjectsfunction, the logic to determine accounts to update has been refactored to use the newgetAccountsToUpdatefunction. This change should streamline the process and potentially fix the issue of missing pnl ticks. However, it's crucial to verify that the refactoring has not altered the intended behavior and that all dependent logic correctly integrates with this change.Verification successful
The search results indicate that the method
getMostRecentPnlTicksForEachAccountis defined and called within thepnl-ticks-helper.tsfile. Similarly, the methodSubaccountTable.getSubaccountsWithTransfersis used in thepnl-ticks-helper.tsfile and tested insubaccount-table.test.tswithin thepostgrespackage tests.Given this information, it appears that the refactoring mentioned in the review comment is localized to the
pnl-ticks-helper.tsfile, and the old logic is not used elsewhere in the codebase, except for in tests which is expected. The tests forSubaccountTable.getSubaccountsWithTransfersin thepostgrespackage are likely ensuring that the method works as intended, which is separate from the refactoring in thepnl-ticks-helper.tsfile.Based on the provided context and the results from the scripts, the refactoring in the
getPnlTicksCreateObjectsfunction to use the newgetAccountsToUpdatefunction seems to be correctly integrated, and there are no indications of the old logic being used elsewhere that would affect the intended behavior of the system.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the refactored logic in getPnlTicksCreateObjects integrates correctly with getAccountsToUpdate. # This script checks for any other occurrences of the old logic that might need updating. rg --type ts "getMostRecentPnlTicksForEachAccount" "src/helpers/" rg --type ts "SubaccountTable.getSubaccountsWithTransfers" "src/helpers/"Length of output: 241
Script:
#!/bin/bash # Search the entire codebase for occurrences of the methods "getMostRecentPnlTicksForEachAccount" and "SubaccountTable.getSubaccountsWithTransfers" # to verify if the old logic is still being used elsewhere. rg --type ts "getMostRecentPnlTicksForEachAccount" rg --type ts "SubaccountTable.getSubaccountsWithTransfers"Length of output: 873
indexer/services/roundtable/__tests__/helpers/pnl-ticks-helper.test.ts (2)
233-250: The test for
getAccountsToUpdateappears to be well-structured and covers the basic scenario for the function. It's important to ensure that the test cases cover various edge cases, such as accounts exactly at the threshold of the update interval. Additionally, the test relies on theONE_HOUR_IN_MILLISECONDSconstant, which should be verified for correctness.39-39: The import of
ONE_HOUR_IN_MILLISECONDSfrom@dydxprotocol-indexer/baseis used in the test to setconfig.PNL_TICK_UPDATE_INTERVAL_MS. This is a good practice as it ensures that the test uses the same constant as the application code, which maintains consistency.
| * @param mostRecentPnlTicks | ||
| * @param blockTime | ||
| */ | ||
| export function getAccountsToUpdate( |
There was a problem hiding this comment.
how about instead of that we get the list of subaccounts that haven't been updated this hour? For example(all pseudocode):
const blockTime = new Date(blockTime);
const normalizedBlockTime = normalizeStartTime(new Date(blockTime)); // 12:00:01 -> 12:00:00
const lastUpdatedBlockTime = accountToLastUpdatedBlockTime[accountId];
const normalizedLastUpdatedBlockTime = normalizeStartTime(lastUpdatedBlockTime);
// process if normalizedBlockTime != normalizedLastUpdatedBlockTime
There was a problem hiding this comment.
that's cleaner. done.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (4)
- indexer/services/roundtable/tests/helpers/pnl-ticks-helper.test.ts (3 hunks)
- indexer/services/roundtable/tests/tasks/create-pnl-ticks.test.ts (2 hunks)
- indexer/services/roundtable/src/helpers/pnl-ticks-helper.ts (5 hunks)
- indexer/services/roundtable/src/tasks/create-pnl-ticks.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- indexer/services/roundtable/tests/tasks/create-pnl-ticks.test.ts
Files skipped from review as they are similar to previous changes (2)
- indexer/services/roundtable/tests/helpers/pnl-ticks-helper.test.ts
- indexer/services/roundtable/src/helpers/pnl-ticks-helper.ts
Additional comments: 3
indexer/services/roundtable/src/tasks/create-pnl-ticks.ts (3)
13-13: The import statement for
normalizeStartTimeis redundant since the AI-generated summary states that this function was removed from thepnl-ticks-helpermodule and moved to this file. If the function is indeed moved here, it should not be imported frompnl-ticks-helper. Please verify the location ofnormalizeStartTimeand update the import statements accordingly.10-18: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [13-34]
The logic within
runTaskseems to correctly check if the latest block time is within thePNL_TICK_UPDATE_INTERVAL_MSof the last computed PNL tick block time. However, the comparison usesDate.parse(latestBlockTime)andnormalizeStartTime(new Date(pnlTickLatestBlocktime)).getTime(). Ensure that both dates are being compared in the same timezone and format to avoid any potential issues with time comparisons.
- 10-18: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [34-44]
The transaction logic within
runTaskis correctly implemented with a try-finally block, ensuring that the transaction is always rolled back to prevent hanging DB connections. This is a good practice as it ensures that resources are cleaned up even if an error occurs during the transaction.
| import _ from 'lodash'; | ||
|
|
||
| import config from '../config'; | ||
| import { getPnlTicksCreateObjects } from '../helpers/pnl-ticks-helper'; | ||
| import { getPnlTicksCreateObjects, normalizeStartTime } from '../helpers/pnl-ticks-helper'; | ||
| import { redisClient } from '../helpers/redis'; | ||
|
|
||
| export function normalizeStartTime( | ||
| time: Date, | ||
| ): Date { | ||
| const epochMs: number = time.getTime(); | ||
| const normalizedTimeMs: number = epochMs - ( | ||
| epochMs % config.PNL_TICK_UPDATE_INTERVAL_MS | ||
| ); | ||
|
|
||
| return new Date(normalizedTimeMs); | ||
| } | ||
|
|
||
| export default async function runTask(): Promise<void> { | ||
| const startGetNewTicks: number = Date.now(); | ||
| const [ |
There was a problem hiding this comment.
Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [46-66]
The batchCreateNewTicks function correctly chunks the ticks and processes them in batches, which is good for performance. However, it's important to ensure that the PNL_TICK_MAX_ROWS_PER_UPSERT constant is set to a value that optimizes the balance between the number of database operations and the size of each operation. Additionally, the error handling within the catch block should log the error for better debugging and possibly alerting, as it currently only rolls back the transaction and rethrows the error.
} catch (e) {
- await Transaction.rollback(txId);
+ logger.error({
+ at: 'create-pnl-ticks#batchCreateNewTicks',
+ message: 'Failed to create PNL ticks batch',
+ error: e.toString(),
+ batch: ticksToCreate,
+ });
+ await Transaction.rollback(txId);
throw e;
}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- indexer/services/roundtable/tests/tasks/create-pnl-ticks.test.ts (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- indexer/services/roundtable/tests/tasks/create-pnl-ticks.test.ts
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- indexer/services/roundtable/tests/tasks/create-pnl-ticks.test.ts (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- indexer/services/roundtable/tests/tasks/create-pnl-ticks.test.ts
Changelist
Fix occasional missing pnl ticks.
Pnl computation is skipping some hours for some subaccounts.
For example, 106bcbc8-f80d-5d62-bf65-e88708c88ec9 subaccount id in staging missed the pnl computation at 2024-01-09 04:00:25.572+00 because the last pnl tick happened at 2024-01-09 03:00:50.58+00, which is slightly < 1 hr ago.
When checking whether or not to update a subaccount's pnl, change the time threshold from 1 hr to 0.9 hrs.
Test Plan
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.