pass initial kafka timestamp from Ender -> Vulcan for better e2e order latency tracking #1211
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThese changes introduce a new 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 (21)
- indexer/packages/kafka/tests/batch-kafka-producer.test.ts (2 hunks)
- indexer/packages/kafka/src/batch-kafka-producer.ts (3 hunks)
- indexer/services/ender/tests/lib/block-processor.test.ts (2 hunks)
- indexer/services/ender/src/handlers/handler.ts (2 hunks)
- indexer/services/ender/src/lib/block-processor.ts (2 hunks)
- indexer/services/ender/src/lib/on-message.ts (1 hunks)
- indexer/services/ender/src/lib/types.ts (2 hunks)
- indexer/services/ender/src/validators/asset-validator.ts (1 hunks)
- indexer/services/ender/src/validators/deleveraging-validator.ts (1 hunks)
- indexer/services/ender/src/validators/funding-validator.ts (1 hunks)
- indexer/services/ender/src/validators/liquidity-tier-validator.ts (1 hunks)
- indexer/services/ender/src/validators/market-validator.ts (1 hunks)
- indexer/services/ender/src/validators/order-fill-validator.ts (1 hunks)
- indexer/services/ender/src/validators/perpetual-market-validator.ts (1 hunks)
- indexer/services/ender/src/validators/stateful-order-validator.ts (2 hunks)
- indexer/services/ender/src/validators/subaccount-update-validator.ts (1 hunks)
- indexer/services/ender/src/validators/trading-rewards-validator.ts (1 hunks)
- indexer/services/ender/src/validators/transfer-validator.ts (1 hunks)
- indexer/services/ender/src/validators/update-clob-pair-validator.ts (1 hunks)
- indexer/services/ender/src/validators/update-perpetual-validator.ts (1 hunks)
- indexer/services/ender/src/validators/validator.ts (1 hunks)
Additional comments: 17
indexer/services/ender/src/validators/asset-validator.ts (1)
- 13-13: The addition of the parameter
_in thecreateHandlersmethod is noted. It's common to use_for unused parameters, but for clarity and future maintainability, consider renaming it to something more descriptive, even if it remains unused for now. Could you share the intended use of this parameter?indexer/services/ender/src/validators/subaccount-update-validator.ts (1)
- 22-22: Similar to the previous file, consider renaming the unused parameter
_in thecreateHandlersmethod to something more descriptive for clarity and future code maintainability. Could you clarify the intended use of this parameter?indexer/services/ender/src/validators/update-perpetual-validator.ts (1)
- 21-21: As seen in other validators, the unused parameter
_in thecreateHandlersmethod could benefit from a more descriptive name for clarity. What is the intended future use of this parameter?indexer/services/ender/src/validators/update-clob-pair-validator.ts (1)
- 23-23: Following the pattern in other validators, renaming the unused parameter
_in thecreateHandlersmethod to something more descriptive would enhance clarity. Could you explain the intended use of this parameter?indexer/services/ender/src/validators/liquidity-tier-validator.ts (1)
- 37-37: Consistent with previous observations, consider renaming the unused parameter
_in thecreateHandlersmethod for enhanced clarity. What is the purpose of this parameter?indexer/services/ender/src/validators/deleveraging-validator.ts (1)
- 41-41: In line with previous files, renaming the unused parameter
_in thecreateHandlersmethod could improve clarity. Could you share the intended use of this parameter?indexer/services/ender/src/validators/trading-rewards-validator.ts (1)
- 39-39: Noticing a slight variation, the unused parameter
__in thecreateHandlersmethod could also benefit from a more descriptive name. What is the intended use of this parameter?indexer/services/ender/src/validators/perpetual-market-validator.ts (1)
- 41-41: As with the other validators, consider renaming the unused parameter
_in thecreateHandlersmethod to enhance clarity. Could you explain the intended use of this parameter?indexer/services/ender/src/validators/funding-validator.ts (1)
- 45-45: The addition of a new parameter
_of type string to thecreateHandlersmethod is noted. It's important to ensure that this parameter is named descriptively if it's being used, or if it's intended to be a placeholder for future use, consider adding a comment to clarify its purpose. This will improve code readability and maintainability.indexer/services/ender/src/validators/validator.ts (1)
- 59-59: The addition of the
messageReceivedTimestampparameter to thecreateHandlersmethod is a positive change that aligns with the PR's objectives to incorporate Kafka message timestamps into the stateful order processing workflow. This enhances the application's ability to manage stateful operations with additional context.indexer/services/ender/src/validators/transfer-validator.ts (1)
- 49-49: Similar to the change in the
FundingValidator, the addition of a new parameter_of type string to thecreateHandlersmethod is noted. If this parameter is being used, consider renaming it to reflect its purpose more clearly. If it's intended to be a placeholder for future use, adding a comment to clarify its purpose would improve code readability and maintainability.indexer/packages/kafka/src/batch-kafka-producer.ts (1)
- 2-2: The addition of
IHeadersto the import statement, the inclusion ofheaders?: IHeadersin theProducerMessagetype, and the handling ofheaders: message.headersin thepushmethod forproducerMessagesare all positive changes that align with the PR's objectives. These modifications enable the incorporation of Kafka message timestamps and potentially other metadata into the stateful order processing workflow, enhancing the application's capabilities.Also applies to: 13-13, 56-56
indexer/services/ender/src/lib/on-message.ts (1)
- 86-86: The addition of
message.timestampas a parameter to theBlockProcessorconstructor aligns with the PR's objectives to incorporate Kafka message timestamps into the processing workflow. Ensure that theBlockProcessorclass is updated accordingly to handle this new parameter effectively. Additionally, verify the handling of the timestamp's format and time zone to ensure consistency and accuracy in temporal information processing.indexer/services/ender/src/lib/types.ts (1)
- 36-36: The addition of the
IHeadersimport and theheadersfield to theVulcanMessageinterface is a suitable enhancement for incorporating Kafka message headers. Ensure that the usage of theheadersfield is correctly handled throughout the codebase, and take precautions to secure any sensitive information that might be included in these headers.indexer/services/ender/src/handlers/handler.ts (1)
- 36-36: The addition of the
messageReceivedTimestampparameter to theHandlerInitializertype and theHandlerclass constructor is well-aligned with the PR's objectives. Ensure that all handlers inheriting from theHandlerclass are updated to handle this new parameter effectively. Additionally, verify the handling of the timestamp's format and time zone within handlers to ensure consistency and accuracy in temporal information processing.indexer/services/ender/src/validators/stateful-order-validator.ts (1)
- 214-214: The addition of the
messageReceivedTimestampparameter to thecreateHandlersmethod in theStatefulOrderValidatorclass aligns with the PR's objectives. Ensure that handlers created by this class are updated to handle this new parameter effectively. Additionally, verify the handling of the timestamp's format and time zone within these handlers to ensure consistency and accuracy in temporal information processing.indexer/services/ender/src/lib/block-processor.ts (1)
- 80-80: The addition of the
messageReceivedTimestampproperty to theBlockProcessorclass constructor is well-aligned with the PR's objectives. Ensure that the handling of this property within the class is consistent and accurate, especially regarding the timestamp's format and time zone. Additionally, consider the potential impacts on downstream processing logic that relies on theBlockProcessor.
dydxwill
changed the title
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (11)
- indexer/packages/kafka/tests/batch-kafka-producer.test.ts (3 hunks)
- indexer/services/bazooka/src/vulcan-helpers.ts (3 hunks)
- indexer/services/ender/tests/handlers/stateful-order/conditional-order-triggered-handler.test.ts (1 hunks)
- indexer/services/ender/tests/handlers/stateful-order/stateful-order-placement-handler.test.ts (1 hunks)
- indexer/services/ender/tests/handlers/stateful-order/stateful-order-removal-handler.test.ts (2 hunks)
- indexer/services/ender/tests/helpers/indexer-proto-helpers.ts (4 hunks)
- indexer/services/ender/src/handlers/handler.ts (5 hunks)
- indexer/services/ender/src/handlers/stateful-order/conditional-order-triggered-handler.ts (1 hunks)
- indexer/services/ender/src/handlers/stateful-order/stateful-order-placement-handler.ts (1 hunks)
- indexer/services/ender/src/handlers/stateful-order/stateful-order-removal-handler.ts (1 hunks)
- indexer/services/ender/src/lib/kafka-publisher.ts (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- indexer/packages/kafka/tests/batch-kafka-producer.test.ts
- indexer/services/ender/src/handlers/handler.ts
Additional comments: 13
indexer/services/ender/src/handlers/stateful-order/stateful-order-removal-handler.ts (1)
- 45-48: The addition of
message_received_timestampandevent_typeto thegenerateConsolidatedVulcanKafkaEventmethod call aligns with the PR's objective to incorporate Kafka message timestamps for stateful order processing. This change enhances the temporal context of the events being processed, which can be beneficial for order management and debugging.Ensure that the
messageReceivedTimestampproperty is being correctly set and passed to this handler, as its correct functioning is crucial for the intended temporal tracking.indexer/services/ender/src/handlers/stateful-order/stateful-order-placement-handler.ts (1)
- 57-60: The inclusion of
message_received_timestampandevent_typein thegenerateConsolidatedVulcanKafkaEventmethod call within thecreateKafkaEventsmethod is consistent with the PR's goal to leverage Kafka message timestamps for improved order processing. This enhancement provides valuable temporal information for the events being handled.It's important to verify that the
messageReceivedTimestampis accurately captured and utilized, as it plays a critical role in the temporal aspects of order processing.indexer/services/ender/src/handlers/stateful-order/conditional-order-triggered-handler.ts (1)
- 57-60: Adding
message_received_timestampandevent_typeto thegenerateConsolidatedVulcanKafkaEventmethod call in theConditionalOrderTriggeredHandlerclass aligns with the PR's objectives. This modification enriches the event data with temporal context, which is crucial for managing stateful order processing more effectively.Confirm that the
messageReceivedTimestampis being correctly set and utilized within this handler to ensure the temporal data's accuracy and relevance.indexer/services/ender/__tests__/handlers/stateful-order/stateful-order-removal-handler.test.ts (1)
- 136-136: The addition of
headersin the test case for theStatefulOrderRemovalHandlerclass, specifically settingmessage_received_timestampandevent_type, is a good practice to ensure that the new functionality is covered by tests. This change helps validate that theheadersare correctly included and processed in the Kafka events generated by the handler.Ensure that the
message_received_timestampvalue used in tests accurately reflects the format and type expected in production to maintain test reliability and relevance.indexer/services/bazooka/src/vulcan-helpers.ts (2)
- 34-34: The addition of the
headersfield to theVulcanMessageinterface is a crucial update that aligns with the PR's objectives to incorporate Kafka message timestamps into the stateful order processing workflow. This change enables the inclusion of metadata such asmessage_received_timestampandevent_typein the messages sent to Vulcan, enhancing the temporal context and categorization of the events.Ensure that all instances where
VulcanMessageis used are updated to consider the newheadersfield, maintaining consistency and leveraging the added metadata effectively.
- 134-134: Including the
headersproperty in the messages sent by thesendStatefulOrderMessagesfunction is a positive step towards enriching the messages with additional context. This modification facilitates better tracking and processing of stateful order events by providing temporal and type information directly within the message headers.Verify that the
headersare correctly populated with the intended values, especially themessage_received_timestamp, to ensure the accuracy and usefulness of the temporal data included in the messages.indexer/services/ender/__tests__/handlers/stateful-order/conditional-order-triggered-handler.test.ts (1)
- 146-146: The inclusion of
headersin the test case for theConditionalOrderTriggeredHandlerclass, specifically settingmessage_received_timestampandevent_type, ensures that the new functionality is adequately tested. This addition helps verify that theheadersare correctly included and processed in the Kafka events generated by the handler.It's important to ensure that the
message_received_timestampused in tests accurately reflects the expected format and type in production environments to maintain the reliability and relevance of the tests.indexer/services/ender/__tests__/handlers/stateful-order/stateful-order-placement-handler.test.ts (1)
- 183-183: The addition of
headersin the test case for theStatefulOrderPlacementHandlerclass, with specific values formessage_received_timestampandevent_type, is commendable as it ensures comprehensive test coverage for the new functionality. This change validates that theheadersare correctly included and processed in the Kafka events generated by the handler.Confirm that the
message_received_timestampused in the tests accurately mirrors the format and type expected in production to ensure the tests remain reliable and relevant.indexer/services/ender/src/lib/kafka-publisher.ts (1)
- 250-250: Including the
headersfield in the messages generated by theKafkaPublisherclass is a significant enhancement that aligns with the PR's objectives. This change allows for the inclusion of additional metadata, such asmessage_received_timestampandevent_type, directly within the Kafka messages, providing valuable context for the message consumers.Ensure that the
headersare populated with accurate and relevant information, particularly themessage_received_timestamp, to maximize the utility of the added metadata.indexer/services/ender/__tests__/helpers/indexer-proto-helpers.ts (4)
- 54-54: The import of
IHeadersfrom 'kafkajs' aligns with the PR's objective to incorporate Kafka message timestamps for stateful order processing. This addition is crucial for handling message metadata, including timestamps.- 321-326: The addition of an optional
headersparameter to theexpectVulcanKafkaMessagefunction is a direct implementation of the PR's objective to leverage Kafka message timestamps. This change allows the function to accept message headers, including timestamps, for more precise testing and validation of Kafka messages.- 352-352: Assigning the
headersproperty from themessageobject to the returnedVulcanMessageis a necessary step to ensure that message metadata, including timestamps, is correctly propagated and available for further processing or testing. This change effectively integrates the newheadersfunctionality into the existing message handling logic.- 360-360: Ensuring that the
headersare included in theexpectassertion forVulcanMessageis crucial for validating that the message headers, including any timestamp information, are correctly attached and handled. This addition enhances the test's ability to verify the correct handling of message metadata.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- indexer/packages/kafka/tests/batch-kafka-producer.test.ts (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- indexer/packages/kafka/tests/batch-kafka-producer.test.ts
| ) { | ||
| this.block = block; | ||
| this.blockEventIndex = blockEventIndex; | ||
| this.indexerTendermintEvent = indexerTendermintEvent; | ||
| this.timestamp = DateTime.fromJSDate(block.time!); | ||
| this.txId = txId; | ||
| this.event = event; | ||
| this.messageReceivedTimestamp = messageReceivedTimestamp; |
There was a problem hiding this comment.
can't we just use the timestamp from this.block? I'm pretty sure we already have a helper to pull timestamp out of it
There was a problem hiding this comment.
We could, but I wanted to only track Indexer system latency without block gossiping latency.
| ) { | ||
| this.block = block; | ||
| this.txId = txId; | ||
| this.messageReceivedTimestamp = messageReceivedTimestamp; |
yeah |
Christopher-Li
approved these changes
Mar 20, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changelist
pass initial kafka timestamp from Ender -> Vulcan for better e2e order latency tracking
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.