fix(affiliate): Use DB transaction for username generation; add more stats for update… #2579
Conversation
WalkthroughThe pull request introduces significant modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
teddyding
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: 1
🧹 Outside diff range and nitpick comments (3)
indexer/services/roundtable/src/tasks/update-affiliate-info.ts (2)
59-70: Consider enhancing metrics for better observability.The timing metric could be improved in several ways:
- The metric name includes "txn" but only measures the update operation, not the entire transaction
- Missing tags/labels that could help with metric categorization
- Cache update timing is not measured separately
Consider this improvement:
const updateAffiliateInfoStartTime: number = Date.now(); await AffiliateInfoTable.updateInfo(windowStartTime.toISO(), windowEndTime.toISO(), txId); + stats.timing( + `${config.SERVICE_NAME}.update-affiliate-info.update_operation.timing`, + Date.now() - updateAffiliateInfoStartTime, + { operation: 'update_info', window_size: windowEndTime.diff(windowStartTime).as('days') } + ); + + const cacheUpdateStartTime: number = Date.now(); await PersistentCacheTable.upsert({ key: PersistentCacheKeys.AFFILIATE_INFO_UPDATE_TIME, value: windowEndTime.toISO(), }, { txId }); + stats.timing( + `${config.SERVICE_NAME}.update-affiliate-info.cache_update.timing`, + Date.now() - cacheUpdateStartTime, + { operation: 'cache_update' } + ); await Transaction.commit(txId); - stats.timing( - `${config.SERVICE_NAME}.update-affiliate-info.update-txn.timing`, - Date.now() - updateAffiliateInfoStartTime, - );
79-80: Enhance the total timing metric with additional context.While the total timing metric is useful, it could provide more context about the task execution.
Consider adding tags to provide more context:
- stats.timing(`${config.SERVICE_NAME}.update-affiliate-info.total.timing`, Date.now() - taskStart); + stats.timing( + `${config.SERVICE_NAME}.update-affiliate-info.total.timing`, + Date.now() - taskStart, + { + status: 'success', + window_start: windowStartTime.toISO(), + window_end: windowEndTime.toISO(), + } + );indexer/services/roundtable/src/tasks/subaccount-username-generator.ts (1)
45-45: Address the TODO comment or remove if obsolete.The comment
// NEXT: use TxnID.at line 45 appears to be a leftover note or a reminder for future implementation. If the transaction ID (txId) is already being used correctly in the code, consider removing this comment to avoid confusion. If additional implementation is needed regarding the transaction ID, please address it accordingly.Would you like assistance in updating the code to utilize the transaction ID where necessary?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
indexer/services/roundtable/src/tasks/subaccount-username-generator.ts(1 hunks)indexer/services/roundtable/src/tasks/update-affiliate-info.ts(3 hunks)
🔇 Additional comments (1)
indexer/services/roundtable/src/tasks/update-affiliate-info.ts (1)
21-24: LGTM! Well-structured transaction management.
The transaction properly wraps both the cache operations and affiliate info updates, ensuring atomicity. The timing variable naming is consistent with other files.
| at: 'update-wallet-total-volume#runTask', | ||
| message: 'Error when updating totalVolume in wallets table', |
There was a problem hiding this comment.
Correct the logging context and message in the error handler.
At lines 96-97, the logger.error function is using an incorrect context and message:
- The
atfield is set to'update-wallet-total-volume#runTask', which does not match the current function context. - The
messagestates'Error when updating totalVolume in wallets table', which is unrelated to the username generation task.
Please update the logging to reflect the correct context and message for this function.
Apply this diff to fix the logging:
logger.error({
- at: 'update-wallet-total-volume#runTask',
- message: 'Error when updating totalVolume in wallets table',
+ at: 'subaccount-username-generator#runTask',
+ message: 'Error when generating usernames',
error,
});📝 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.
| at: 'update-wallet-total-volume#runTask', | |
| message: 'Error when updating totalVolume in wallets table', | |
| at: 'subaccount-username-generator#runTask', | |
| message: 'Error when generating usernames', |
| } catch (error) { | ||
| await Transaction.rollback(txId); | ||
| logger.error({ | ||
| at: 'update-wallet-total-volume#runTask', |
There was a problem hiding this comment.
nit: Update name to subaccount-username-generator.
| @@ -149,10 +149,8 @@ | |||
| "hip", | |||
| "holy", | |||
| "honest", | |||
| "hot", | |||
There was a problem hiding this comment.
Refactored both word lists and spot checked 34 proposed whitelist addresses
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
indexer/services/roundtable/__tests__/tasks/subaccount-username-generator.test.ts (1)
Line range hint
31-61: Consider adding transaction-specific test cases.Since the main implementation now uses DB transactions for username generation, consider adding test cases that verify:
- Rollback behavior on partial failures
- Concurrent username generation scenarios
- Duplicate username handling within transactions
Example test structure:
it('Should rollback all username creations if any fails', async () => { // Setup: Create a scenario where one username creation would fail // Action: Run username generator // Assert: No usernames should be created due to transaction rollback }); it('Should handle concurrent username generation correctly', async () => { // Setup: Create multiple subaccounts // Action: Run username generator concurrently // Assert: Each subaccount gets a unique username without conflicts });indexer/services/roundtable/__tests__/helpers/usernames-helper.test.ts (4)
21-21: Improve test description clarity.The test description "Check result of generated username" could be more specific about what aspects are being verified.
Consider renaming to: "should generate expected usernames and ensure uniqueness across nonces"
40-51: Enhance test readability and maintainability.The test logic could be improved in several ways:
- The magic number '10' should be defined as a constant with a meaningful name
- The comment has a typo ("difference" should be "different")
- The test's dual purpose (checking expected values and uniqueness) could be split into separate test cases
Consider this refactor:
+ const NONCE_TEST_COUNT = 10; + const gotUserNames = []; for (let i = 0; i < addresses.length; i++) { const address = addresses[i]; const namesForOneAddress = new Set(); - for (let k = 0; k < 10; k++) { + for (let k = 0; k < NONCE_TEST_COUNT; k++) { const username: string = generateUsernameForSubaccount(address, 0, k); if (k === 0) { gotUserNames.push(username); } namesForOneAddress.add(username); } - // for same address, difference nonce should result in different username + // for same address, different nonces should result in different usernames - expect(namesForOneAddress.size).toEqual(10); + expect(namesForOneAddress.size).toEqual(NONCE_TEST_COUNT); } expect(gotUserNames).toEqual(expectedUsernames);
54-64: Improve determinism test implementation.The test has similar issues to the previous one:
- Uses magic number '10'
- Contains the same comment typo
- Could be more explicit about testing determinism
Consider this refactor:
+ const DETERMINISM_TEST_ITERATIONS = 10; + it('Check determinism of generated username', () => { for (let i = 0; i < addresses.length; i++) { const address = addresses[i]; const namesForOneAddress = new Set(); - for (let k = 0; k < 10; k++) { + for (let k = 0; k < DETERMINISM_TEST_ITERATIONS; k++) { const username: string = generateUsernameForSubaccount(address, 0, 0); namesForOneAddress.add(username); } - // for same address, difference nonce should result in different username + // Multiple calls with same inputs should produce the same username expect(namesForOneAddress.size).toEqual(1); } });
20-65: Consider adding more test coverage.The current test suite could benefit from additional test cases:
- Error cases (e.g., invalid addresses)
- Boundary testing for nonce values
- Edge cases for subaccount IDs
Would you like me to provide examples of additional test cases?
indexer/services/roundtable/src/tasks/subaccount-username-generator.ts (2)
33-36: Track technical debt regarding table design.The comment indicates a known technical debt regarding the
subaccount_usernamestable design.Would you like me to create a GitHub issue to track this technical debt for future refactoring?
49-71: Consider enhancing collision metrics.While the collision handling is good, consider adding these additional metrics for better monitoring:
- Track consecutive collisions per address
- Record collision frequency over time
Would you like me to propose the implementation for these additional metrics?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
indexer/services/roundtable/__tests__/helpers/usernames-helper.test.ts(1 hunks)indexer/services/roundtable/__tests__/tasks/subaccount-username-generator.test.ts(1 hunks)indexer/services/roundtable/src/helpers/adjectives.json(1 hunks)indexer/services/roundtable/src/helpers/nouns.json(0 hunks)indexer/services/roundtable/src/tasks/subaccount-username-generator.ts(1 hunks)
💤 Files with no reviewable changes (1)
- indexer/services/roundtable/src/helpers/nouns.json
🔇 Additional comments (9)
indexer/services/roundtable/__tests__/tasks/subaccount-username-generator.test.ts (1)
52-54: LGTM! Expected username updates look correct.
The updated expected usernames maintain the test's integrity while properly mapping to their respective addresses through the inline comments.
indexer/services/roundtable/__tests__/helpers/usernames-helper.test.ts (1)
5-18: LGTM! Good test data coverage.
The test addresses provide good coverage including edge cases with similar addresses (differing by one character).
indexer/services/roundtable/src/tasks/subaccount-username-generator.ts (5)
13-23: LGTM! Good addition of performance monitoring.
The timing metrics for subaccount fetching will help track performance and identify potential bottlenecks.
25-27: LGTM! Good transaction management setup.
Transaction management is properly implemented with separate timing metrics.
75-91: LGTM! Comprehensive transaction completion and logging.
Good implementation with:
- Proper transaction commit
- Detailed logging including batch size and success count
- Performance metrics
94-98: Fix incorrect error logging message.
The error message still references "updating totalVolume in wallets table" which is incorrect for this context.
As mentioned in the previous review, please apply this fix:
logger.error({
at: 'subaccount-username-generator#runTask',
- message: 'Error when updating totalVolume in wallets table',
+ message: 'Error when generating usernames',
error,
});101-104: LGTM! Good addition of total execution timing.
The total timing metric will help monitor the overall task performance.
indexer/services/roundtable/src/helpers/adjectives.json (2)
Line range hint 1-373: LGTM! Appropriate cleanup of adjectives.
The removal of potentially problematic adjectives ("hot", "humid", "lousy", "low", "pale", "rude", "used", "untidy", "zoom") improves the quality of generated usernames by eliminating terms that could be inappropriate or have negative connotations.
373-373: Verify the impact of adjective reordering on username generation.
Moving "zippy" to the end of the list could affect username generation if the order is used deterministically.
✅ Verification successful
Let me check the username generation logic in the found file to understand how adjectives are used.
Reordering adjectives has no impact on username generation
The username generation logic uses a seeded random number generator (seedrandom) to select adjectives randomly from the array. The order of adjectives in the array doesn't affect the randomness or deterministic nature of the selection since it uses Math.floor(rng() * adjectives.length) to generate random indices. Moving "zippy" to the end of the list will not impact the username generation process.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the username generation logic depends on the order of adjectives
# Search for files that import or use the adjectives.json
rg -l "adjectives.json" --type ts
# Look for any deterministic username generation logic that might depend on array indices
ast-grep --pattern 'function $FUNC($ARGS) {
$$$
adjectives[$_]
$$$
}'
Length of output: 171
Script:
#!/bin/bash
# Check the implementation of username generation
cat indexer/services/roundtable/src/helpers/usernames-helper.ts
Length of output: 1138
❌ Sorry but I didn't understand the command. Please consult the commands documentation 📚. |
|
@Mergifyio backport release/indexer/v7.x |
✅ Backports have been created
|
…stats for update… (backport #2579) (#2581) Co-authored-by: Teddy Ding <[email protected]>
…stats for update… (#2579)
Changelist
[Describe or list the changes made in this PR]
Test Plan
[Describe how this PR was tested (if applicable)]
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
Bug Fixes
Tests
Chores