Revert "[CT-708] Indexer track e2e latency (#1237)" #1292
Conversation
This reverts commit 60b94df.
WalkthroughThe series of changes across various files primarily involve the reorganization and simplification of code related to message handling and timestamp conversions. Key adjustments include the removal of the 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.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (14)
- indexer/packages/v4-protos/src/index.ts (1 hunks)
- indexer/services/ender/tests/helpers/indexer-proto-helpers.ts (2 hunks)
- indexer/services/ender/src/lib/helper.ts (3 hunks)
- indexer/services/socks/src/lib/message-forwarder.ts (3 hunks)
- indexer/services/vulcan/tests/handlers/order-place-handler.test.ts (3 hunks)
- indexer/services/vulcan/tests/handlers/order-remove-handler.test.ts (24 hunks)
- indexer/services/vulcan/tests/helpers/helpers.ts (3 hunks)
- indexer/services/vulcan/tests/helpers/websocket-helpers.ts (1 hunks)
- indexer/services/vulcan/tests/lib/on-message.test.ts (1 hunks)
- indexer/services/vulcan/src/handlers/handler.ts (2 hunks)
- indexer/services/vulcan/src/handlers/order-place-handler.ts (7 hunks)
- indexer/services/vulcan/src/handlers/order-remove-handler.ts (11 hunks)
- indexer/services/vulcan/src/handlers/order-update-handler.ts (3 hunks)
- indexer/services/vulcan/src/lib/on-message.ts (2 hunks)
Additional Context Used
Path-based Instructions (14)
indexer/packages/v4-protos/src/index.ts (1)
**/**:
Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.indexer/services/vulcan/__tests__/helpers/websocket-helpers.ts (1)
**/**:
Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.indexer/services/vulcan/src/handlers/handler.ts (1)
**/**:
Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.indexer/services/vulcan/__tests__/helpers/helpers.ts (1)
**/**:
Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.indexer/services/vulcan/src/lib/on-message.ts (1)
**/**:
Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.indexer/services/vulcan/__tests__/lib/on-message.test.ts (1)
**/**:
Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.indexer/services/ender/src/lib/helper.ts (1)
**/**:
Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.indexer/services/socks/src/lib/message-forwarder.ts (1)
**/**:
Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.indexer/services/vulcan/src/handlers/order-update-handler.ts (1)
**/**:
Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.indexer/services/vulcan/src/handlers/order-place-handler.ts (1)
**/**:
Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.indexer/services/ender/__tests__/helpers/indexer-proto-helpers.ts (1)
**/**:
Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.indexer/services/vulcan/src/handlers/order-remove-handler.ts (1)
**/**:
Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.indexer/services/vulcan/__tests__/handlers/order-place-handler.test.ts (1)
**/**:
Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.indexer/services/vulcan/__tests__/handlers/order-remove-handler.test.ts (1)
**/**:
Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.
Additional comments not posted (19)
indexer/packages/v4-protos/src/index.ts (1)
16-18: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-23]
The removal of the
utilsexport fromindex.tscould impact other parts of the project that rely on utilities from this file. Ensure that any dependencies on the removedutilsexport have been addressed or that the functionalities provided byutilsare no longer needed.Verification successful
The search for imports of the removed
utilsfrom@dydxprotocol-indexer/v4-protosacross the project did not yield any results. This suggests that there are no direct dependencies on the removedutilsexport in TypeScript files within the project. However, it's important to consider any potential indirect dependencies or more complex usage patterns that weren't captured by this search.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for imports of the removed `utils` from `v4-protos` across the project rg --type ts "from '@dydxprotocol-indexer/v4-protos'.*utils"Length of output: 68
indexer/services/vulcan/__tests__/helpers/websocket-helpers.ts (1)
1-18: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [3-22]
The removal of
expectedHeadersfromexpectWebsocketSubaccountMessagesimplifies the function by focusing on the message content. Ensure that this change aligns with the testing requirements and that any tests relying on header comparisons have been updated or are no longer necessary.indexer/services/vulcan/src/handlers/handler.ts (1)
18-22: The removal of theheaders: IHeadersparameter fromhandleUpdatesimplifies the method signature. Ensure that any logic previously relying onheadershas been appropriately adjusted or is no longer necessary.indexer/services/vulcan/__tests__/helpers/helpers.ts (1)
13-18: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-81]
The removal of
defaultKafkaHeadersand its usage in testing helpers simplifies the code by removing unnecessary header assignments. Ensure that this change aligns with the testing requirements and that any tests relying on specific header values have been updated or are no longer necessary.indexer/services/vulcan/src/lib/on-message.ts (1)
86-92: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [89-133]
The removal of timing statistics tracking from
onMessagesimplifies the function by focusing solely on message handling. Ensure that this change aligns with the project's performance monitoring strategy and that any necessary adjustments have been made.indexer/services/vulcan/__tests__/lib/on-message.test.ts (1)
67-67: The modification of the assertion in theonMessagetest to remove the check formessage.headersaligns with the changes made in theonMessagefunction. Ensure that this change aligns with the testing requirements and that any tests relying on header comparisons have been updated or are no longer necessary.indexer/services/ender/src/lib/helper.ts (1)
11-11: The addition of theprotoTimestampToDatefunction and related imports (Timestamp,MILLIS_IN_NANOS,SECONDS_IN_MILLIS) enhances the utility of the helper file by providing a standardized way to convertTimestamptoDate. Ensure that the implementation ofprotoTimestampToDateis correct and aligns with the project's date handling requirements.Also applies to: 32-35, 73-80
indexer/services/socks/src/lib/message-forwarder.ts (1)
90-90: The refactoring of time metrics calculation in theMessageForwarderclass simplifies the logic for measuring message queue time and message forwarding time. Ensure that the refactored logic accurately measures the intended metrics and aligns with the project's performance monitoring strategy.Also applies to: 128-133
indexer/services/vulcan/src/handlers/order-update-handler.ts (1)
54-54: The removal of theheadersparameter from thehandlemethod simplifies the message handling process by focusing solely on the content of the messages rather than their metadata. This change aligns with the PR's objective to streamline message and Kafka headers handling.indexer/services/vulcan/src/handlers/order-place-handler.ts (1)
55-55: The removal of theheadersparameter from thehandlemethod and related adjustments reflect the PR's objective to simplify message handling by eliminating the use of Kafka headers in the message processing logic.indexer/services/ender/__tests__/helpers/indexer-proto-helpers.ts (1)
66-66: The change in the import location ofprotoTimestampToDatefrom@dydxprotocol-indexer/v4-protosto../../src/lib/helperlikely reflects a reorganization of helper functions within the project. This adjustment aligns with the PR's objective to reorganize timestamp conversion utilities.indexer/services/vulcan/src/handlers/order-remove-handler.ts (2)
71-71: The changes from lines 41 to 138 involve the removal of theheadersparameter from thehandlemethod signature. This simplification could improve code readability and maintainability by focusing on the essential data needed for processing. Ensure that all calls to this method across the codebase have been updated accordingly to prevent runtime errors.Verification successful
Given the files identified as potentially using
OrderRemoveHandler, we need to inspect these files to ensure that thehandlemethod is called correctly without the previously mentionedheadersparameter. This step is crucial to confirm that the changes have been properly propagated throughout the codebase and to verify the review comment accurately.
The examination of the provided context suggests that the removal of the
headersparameter from thehandlemethod inOrderRemoveHandlerhas been correctly propagated throughout the codebase, as no calls to thehandlemethod with two parameters were found. However, it's important to note that the direct invocation of thehandlemethod was not explicitly shown in the outputs, introducing a slight uncertainty. Based on the evidence available, the review comment is considered verified, with the recommendation for a manual check to ensure complete confidence in the method's usage across the codebase.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for files that might import and use OrderRemoveHandler. Expecting list of dependent files. rg --type ts "OrderRemoveHandler" --files-with-matches # Verify that no calls to handle method include the headers parameter. Expecting no results. ast-grep --lang typescript --pattern $'OrderRemoveHandler.prototype.handle($_, $_)'Length of output: 375
Script:
#!/bin/bash # Inspect the usage of OrderRemoveHandler in the identified files to ensure the handle method is called correctly. # File: indexer/services/vulcan/src/lib/on-message.ts rg "OrderRemoveHandler" indexer/services/vulcan/src/lib/on-message.ts # File: indexer/services/vulcan/__tests__/handlers/order-remove-handler.test.ts rg "OrderRemoveHandler" indexer/services/vulcan/__tests__/handlers/order-remove-handler.test.ts # File: indexer/services/vulcan/__tests__/lib/on-message.test.ts rg "OrderRemoveHandler" indexer/services/vulcan/__tests__/lib/on-message.test.tsLength of output: 2370
138-138: The removal of theheadersparameter and its related adjustments within theOrderRemoveHandlerclass streamline the message handling process. This change should simplify the code and potentially improve performance by reducing the complexity of method signatures and calls. However, ensure that this simplification does not remove any critical functionality related to message headers that might be required for other features or performance tracking.indexer/services/vulcan/__tests__/handlers/order-place-handler.test.ts (1)
1228-1228: The changes from lines 1228 to 1228 involve the call toexpectWebsocketSubaccountMessagewithin theexpectWebsocketMessagesSentfunction. This update is part of the refactoring effort to simplify message handling and Kafka headers usage throughout the project. It's important to ensure that the updated call correctly matches the expected structure and content of the subaccount message, especially after the removal ofdefaultKafkaHeaders. The test logic here appears to correctly validate the expected subaccount message against the actual message sent by the producer, which is a good practice for ensuring the integrity of message handling logic.indexer/services/vulcan/__tests__/handlers/order-remove-handler.test.ts (5)
52-65: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-1065]
The setup and teardown code, including the use of mocks and timers, is appropriate for the tests being conducted. It ensures that the tests are isolated from external effects and can run in a controlled environment. Good job on maintaining a clean testing environment.
182-182: The tests for handling invalid updates are well-implemented, using parameterized tests withit.eachto efficiently cover multiple scenarios. The use ofexpect().rejects.toThrow()andexpect(logger.error)ensures that the handler's error handling and logging are correctly tested.
323-323: The tests for successful order removal are comprehensive and cover a wide range of scenarios. They effectively use helper functions and spies to simulate system state and verify outcomes. Consider adding comments to clarify the purpose and setup of each test case, enhancing future maintainability.
1426-1426: The tests for error scenarios and special conditions are correctly implemented, testing important aspects of the handler's behavior in edge cases. Ensure that all edge cases are covered and that the tests remain up-to-date with any changes to the handler's logic. The use of specific assertions likeexpectNoWebsocketMessagesSentandexpect(logger.error)is commendable.
2047-2047: The utility functions used in the tests, such asexpectOrderStatusandexpectWebsocketMessagesSent, are well-designed and improve the readability and maintainability of the tests. Their consistent use across the test suite is commendable. Consider adding more utility functions for other repetitive tasks to further improve test code quality.
* Add subaccountNumber to PerpetualPositionResponseObject (#1274) Signed-off-by: Shrenuj Bansal <[email protected]> * [CT-712] send updates for both normal order matches and liquidation (#1280) * Remove volatile market (#1263) * [SKI-21] Bump slinky version to v0.3.1 (#1275) * Bump slinky version * Add cp number method * clean up socks logging (#1285) * [CT-681] fix liquidated side and offsetting side for indexer delevera… (#1284) * [CT-681] fix liquidated side and offsetting side for indexer deleveraging events * fix test * fix test * fix lint * [TRA-105] Add API for parent subaccount perpetual positions (#1282) Signed-off-by: Shrenuj Bansal <[email protected]> * Use sample rate with stream destroyed stats. (#1294) * Revert "[CT-708] Indexer track e2e latency (#1237)" (#1292) This reverts commit 60b94df. * Fix swagger generation makefile command / regen swagger docs (#1299) * pull dydx fork to generate swagger properly * remove the print * remove vault constants (#1293) * Remove custom ping message from socks (#1301) * Add subaccountNumber to the OrderResponseObject (#1296) Signed-off-by: Shrenuj Bansal <[email protected]> * sample more metrics (#1304) * [OTE-256] Add upgrade handler to initialize OI during upgrade handler (#1302) * Add upgrade handler to initialize OI during upgrade handler * nits * Fix lib.ErrorLogWithError: properly pass in args (#1306) * fix broken tests (#1312) * Explicitly close websockets on errors (#1290) * Increase the number of allowed connections to 8000 (#1317) * [TRA-104] Add parentSubaccountNumber API for orders (#1313) Signed-off-by: Shrenuj Bansal <[email protected]> * Improve Slinky logs to prevent unnecessary logs (#1289) * [SKI-26]: Prevent funding index update with no oracle prices from (#1321) halting indexer * Skip equity tier limit check in PlaceShortTermOrder (#1318) * Skip equity tier limit check in PlaceShortTermOrder * remove tests * Add comment * fix lint (#1323) --------- Signed-off-by: Shrenuj Bansal <[email protected]> Co-authored-by: shrenujb <[email protected]> Co-authored-by: jayy04 <[email protected]> Co-authored-by: Eric Warehime <[email protected]> Co-authored-by: vincentwschau <[email protected]> Co-authored-by: Jonathan Fung <[email protected]> Co-authored-by: Tian <[email protected]> Co-authored-by: Teddy Ding <[email protected]> Co-authored-by: roy-dydx <[email protected]> Co-authored-by: Christopher-Li <[email protected]>
This reverts commit 60b94df.
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.