Add instance id to socks metrics #2137
Conversation
WalkthroughThis update enhances the Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant InstanceManager as Instance ID Manager
participant Logger
App->>InstanceManager: setInstanceId()
InstanceManager->>Logger: Log start of ID retrieval
InstanceManager->>InstanceManager: Check environment
alt Valid Environment
InstanceManager->>InstanceManager: Fetch task ARN
InstanceManager->>InstanceManager: Set instance ID
else Invalid Environment
InstanceManager->>InstanceManager: Generate UUID
end
InstanceManager->>Logger: Log instance ID
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 Configuration 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: 1
Outside diff range, codebase verification and nitpick comments (2)
indexer/packages/base/package.json (1)
13-13:jestis necessary, butuuidis unused.
jestis actively used for testing across the codebase, confirming its necessity.- There is no evidence of
uuidbeing used, suggesting it may not be needed.Consider removing
uuidfrom the dependencies if it is not required.Analysis chain
Verify the necessity and usage of new dependencies.
The addition of
jestanduuidindicates enhancements in testing and unique identifier generation. Ensure these dependencies are necessary and used appropriately in the codebase.Run the following script to verify the usage of
jestanduuid:Also applies to: 42-42
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `jest` and `uuid` in the codebase. # Test: Search for the usage of `jest` and `uuid`. Expect: Occurrences of their usage in the codebase. rg --type js -A 5 $'jest' rg --type js -A 5 $'uuid'Length of output: 4264
Script:
#!/bin/bash # Search for usage of `jest` in test files rg --type js -A 5 'jest.' --glob '**/__tests__/**' --glob '**/*.test.js' # Search for direct imports or usage of `uuid` in the codebase rg --type js -A 5 'require("uuid")' rg --type js -A 5 'import uuid'Length of output: 296146
indexer/packages/base/src/instance-id.ts (1)
44-49: Clarify the purpose ofresetForTests.The
resetForTestsfunction is intended for testing environments only. Ensure this is clearly documented and consider using a more descriptive name to avoid accidental misuse in production code.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
indexer/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
Files selected for processing (11)
- indexer/packages/base/tests/instance-id.test.ts (1 hunks)
- indexer/packages/base/package.json (3 hunks)
- indexer/packages/base/src/config.ts (1 hunks)
- indexer/packages/base/src/index.ts (1 hunks)
- indexer/packages/base/src/instance-id.ts (1 hunks)
- indexer/packages/base/tsconfig.json (1 hunks)
- indexer/services/socks/src/helpers/wss.ts (4 hunks)
- indexer/services/socks/src/index.ts (2 hunks)
- indexer/services/socks/src/lib/message-forwarder.ts (10 hunks)
- indexer/services/socks/src/lib/subscription.ts (5 hunks)
- indexer/services/socks/src/websocket/index.ts (5 hunks)
Files skipped from review due to trivial changes (1)
- indexer/packages/base/src/index.ts
Additional comments not posted (29)
indexer/packages/base/tsconfig.json (1)
7-8: Good addition to the TypeScript configuration.Including the
__tests__directory ensures that test files are compiled, enhancing type safety and integration of tests within the project.indexer/packages/base/src/config.ts (1)
33-33: Default values enhance configuration robustness.Adding default values for
SERVICE_NAMEandECS_CONTAINER_METADATA_URI_V4ensures they are initialized, preventing potential runtime issues.Also applies to: 41-41
indexer/packages/base/package.json (2)
5-5: Main entry point update is consistent with potential restructuring.Changing the main entry point to
build/src/index.jssuggests a more organized build structure.
22-22: Improved test script using Jest.The test script now uses Jest, which is a robust testing framework, enhancing the project's testing capabilities.
indexer/packages/base/src/instance-id.ts (1)
6-10: Ensure thread safety forINSTANCE_ID.The
INSTANCE_IDvariable is shared across asynchronous operations. Consider using a locking mechanism or atomic operations to ensure thread safety if this code will be run in a multi-threaded environment.indexer/services/socks/src/index.ts (1)
42-53: Evaluate the impact of async operations on startup time.The
setInstanceIdfunction is called asynchronously during startup. Consider evaluating the impact on the startup time and whether this operation should be made non-blocking or moved to a background task.indexer/packages/base/__tests__/instance-id.test.ts (1)
12-86: Great test coverage for instance ID scenarios.The test cases comprehensively cover different scenarios for setting the instance ID, including production, staging, and error handling. Consider adding tests for concurrent calls to
setInstanceIdto ensure thread safety.indexer/services/socks/src/helpers/wss.ts (3)
108-110: LGTM! Enhanced observability with instance ID in metrics.The addition of the
instancefield to the metrics improves tracking and debugging.
122-125: LGTM! Improved error tracking with instance ID.Including the instance ID in error metrics enhances the ability to diagnose issues.
155-158: LGTM! Instance ID added to stream destruction error metrics.The instance ID inclusion in stream destruction error metrics provides better context for troubleshooting.
indexer/services/socks/src/websocket/index.ts (5)
111-116: LGTM! Enhanced connection metrics with instance ID.Including the instance ID in connection metrics provides better tracking of connections per instance.
121-123: LGTM! Improved concurrent connection tracking with instance ID.The addition of the instance ID to concurrent connection metrics enhances monitoring capabilities.
191-195: LGTM! Enhanced disconnect metrics with instance ID.Including the instance ID in disconnect metrics allows for more granular insights into connection issues.
232-234: LGTM! Improved message handling metrics with instance ID.The instance ID inclusion in message handling metrics enhances observability.
343-345: LGTM! Enhanced message type metrics with instance ID.Adding the instance ID to message type metrics provides better context for message tracking.
indexer/services/socks/src/lib/message-forwarder.ts (10)
102-106: LGTM! Enhanced batch processing metrics with instance ID.The addition of the instance ID to batch processing metrics provides better tracking of batch operations per instance.
203-205: LGTM! Improved message queue metrics with instance ID.Including the instance ID in message queue metrics enhances observability of message processing times.
236-238: LGTM! Enhanced message forwarding metrics with instance ID.The instance ID inclusion in message forwarding metrics provides better context for performance monitoring.
249-251: LGTM! Improved message timing metrics with instance ID.Adding the instance ID to message timing metrics enhances the ability to track message delays.
263-265: LGTM! Enhanced message forwarding success metrics with instance ID.Including the instance ID in message forwarding success metrics provides better insights into message delivery.
337-339: LGTM! Improved subscriber metrics with instance ID.The instance ID inclusion in subscriber metrics enhances tracking of message delivery to subscribers.
350-352: LGTM! Enhanced subscriber message metrics with instance ID.Adding the instance ID to subscriber message metrics provides better context for message delivery tracking.
445-450: LGTM! Improved batch error metrics with instance ID.Including the instance ID in batch error metrics enhances the ability to diagnose issues.
457-462: LGTM! Enhanced batch success metrics with instance ID.The addition of the instance ID to batch success metrics provides better insights into successful batch operations.
507-512: LGTM! Improved client error metrics with instance ID.Including the instance ID in client error metrics enhances the ability to diagnose client-related issues.
indexer/services/socks/src/lib/subscription.ts (4)
3-3: Import ofgetInstanceIdis appropriate.The import of
getInstanceIdis necessary for the new functionality and is correctly added.
183-183: Integration ofgetInstanceIdin metrics is correct.The
getInstanceIdfunction is consistently used to enhance metrics with the instance identifier, improving traceability.Also applies to: 194-194, 286-286, 293-295
351-351: Integration ofgetInstanceIdinunsubscribemetrics is correct.The
getInstanceIdfunction is correctly used to enhance metrics with the instance identifier, improving traceability.
Line range hint
183-295: Overall integration ofgetInstanceIdis consistent and beneficial.The integration of
getInstanceIdacross the file enhances the traceability and monitoring of metrics without altering core logic.Also applies to: 351-351
| export async function setInstanceId(): Promise<void> { | ||
| if (INSTANCE_ID !== '') { | ||
| return; | ||
| } | ||
| if (config.ECS_CONTAINER_METADATA_URI_V4 !== '' && | ||
| ( | ||
| config.isProduction() || config.isStaging() | ||
| ) | ||
| ) { | ||
| // https://docs.aws.amazon.com/AmazonECS/latest/developerguide/task-metadata-endpoint-v4.html | ||
| const taskUrl = `${config.ECS_CONTAINER_METADATA_URI_V4}/task`; | ||
| try { | ||
| const response = await axiosRequest({ | ||
| method: 'GET', | ||
| url: taskUrl, | ||
| }) as { TaskARN: string }; | ||
| INSTANCE_ID = response.TaskARN; | ||
| } catch (error) { | ||
| logger.error({ | ||
| at: 'instance-id#setInstanceId', | ||
| message: 'Failed to retrieve task arn from metadata endpoint. Falling back to uuid.', | ||
| error, | ||
| taskUrl, | ||
| }); | ||
| INSTANCE_ID = uuidv4(); | ||
| } | ||
| } else { | ||
| INSTANCE_ID = uuidv4(); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider adding retries for network requests.
The setInstanceId function makes a network request to retrieve the task ARN. Consider implementing a retry mechanism with exponential backoff to handle transient network failures more gracefully.
| expect(axiosRequest).not.toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
| }); |
| throw new Error(`resetForTests() cannot be called for env: ${config.NODE_ENV}`); | ||
| } | ||
| INSTANCE_ID = ''; | ||
| } |
roy-dydx
force-pushed
the
roy/instanceid
branch
from
fa11b5f to
1fc5b7e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
indexer/services/socks/src/index.ts (1)
42-45: Logging before instance ID retrieval is clear.The log message before calling
setInstanceIdprovides clarity on the process being initiated.Consider adding more context if needed, but this is generally good for tracing.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
indexer/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
Files selected for processing (11)
- indexer/packages/base/tests/instance-id.test.ts (1 hunks)
- indexer/packages/base/package.json (3 hunks)
- indexer/packages/base/src/config.ts (1 hunks)
- indexer/packages/base/src/index.ts (1 hunks)
- indexer/packages/base/src/instance-id.ts (1 hunks)
- indexer/packages/base/tsconfig.json (1 hunks)
- indexer/services/socks/src/helpers/wss.ts (4 hunks)
- indexer/services/socks/src/index.ts (2 hunks)
- indexer/services/socks/src/lib/message-forwarder.ts (10 hunks)
- indexer/services/socks/src/lib/subscription.ts (5 hunks)
- indexer/services/socks/src/websocket/index.ts (5 hunks)
Files skipped from review as they are similar to previous changes (10)
- indexer/packages/base/tests/instance-id.test.ts
- indexer/packages/base/package.json
- indexer/packages/base/src/config.ts
- indexer/packages/base/src/index.ts
- indexer/packages/base/src/instance-id.ts
- indexer/packages/base/tsconfig.json
- indexer/services/socks/src/helpers/wss.ts
- indexer/services/socks/src/lib/message-forwarder.ts
- indexer/services/socks/src/lib/subscription.ts
- indexer/services/socks/src/websocket/index.ts
Additional comments not posted (4)
indexer/services/socks/src/index.ts (4)
1-1: Import changes look good.The new imports
setInstanceIdandgetInstanceIdare correctly added and used in the code.
47-47: Ensure error handling forsetInstanceId.The
setInstanceIdfunction is awaited, which is correct. However, consider adding error handling to manage any potential issues during the instance ID setting.Check if
setInstanceIdhas built-in error handling or if additional try-catch blocks are needed.
49-52: Logging after instance ID retrieval is effective.The log message correctly indicates the successful retrieval of the instance ID.
Line range hint
53-93: Integration with existing functionality is seamless.The new instance ID management and logging features integrate well with the existing
startfunction without disrupting its flow.
roy-dydx
force-pushed
the
roy/instanceid
branch
from
1fc5b7e to
0f13199
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
indexer/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
Files selected for processing (11)
- indexer/packages/base/tests/instance-id.test.ts (1 hunks)
- indexer/packages/base/package.json (3 hunks)
- indexer/packages/base/src/config.ts (1 hunks)
- indexer/packages/base/src/index.ts (1 hunks)
- indexer/packages/base/src/instance-id.ts (1 hunks)
- indexer/packages/base/tsconfig.json (1 hunks)
- indexer/services/socks/src/helpers/wss.ts (4 hunks)
- indexer/services/socks/src/index.ts (2 hunks)
- indexer/services/socks/src/lib/message-forwarder.ts (10 hunks)
- indexer/services/socks/src/lib/subscription.ts (5 hunks)
- indexer/services/socks/src/websocket/index.ts (5 hunks)
Files skipped from review as they are similar to previous changes (11)
- indexer/packages/base/tests/instance-id.test.ts
- indexer/packages/base/package.json
- indexer/packages/base/src/config.ts
- indexer/packages/base/src/index.ts
- indexer/packages/base/src/instance-id.ts
- indexer/packages/base/tsconfig.json
- indexer/services/socks/src/helpers/wss.ts
- indexer/services/socks/src/index.ts
- indexer/services/socks/src/lib/message-forwarder.ts
- indexer/services/socks/src/lib/subscription.ts
- indexer/services/socks/src/websocket/index.ts
roy-dydx
force-pushed
the
roy/instanceid
branch
from
0f13199 to
253945b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
indexer/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
Files selected for processing (12)
- indexer/Dockerfile.bazooka.remote (1 hunks)
- indexer/packages/base/tests/instance-id.test.ts (1 hunks)
- indexer/packages/base/package.json (3 hunks)
- indexer/packages/base/src/config.ts (1 hunks)
- indexer/packages/base/src/index.ts (1 hunks)
- indexer/packages/base/src/instance-id.ts (1 hunks)
- indexer/packages/base/tsconfig.json (1 hunks)
- indexer/services/socks/src/helpers/wss.ts (4 hunks)
- indexer/services/socks/src/index.ts (2 hunks)
- indexer/services/socks/src/lib/message-forwarder.ts (10 hunks)
- indexer/services/socks/src/lib/subscription.ts (5 hunks)
- indexer/services/socks/src/websocket/index.ts (5 hunks)
Files skipped from review as they are similar to previous changes (10)
- indexer/packages/base/tests/instance-id.test.ts
- indexer/packages/base/package.json
- indexer/packages/base/src/config.ts
- indexer/packages/base/src/index.ts
- indexer/packages/base/src/instance-id.ts
- indexer/packages/base/tsconfig.json
- indexer/services/socks/src/helpers/wss.ts
- indexer/services/socks/src/lib/message-forwarder.ts
- indexer/services/socks/src/lib/subscription.ts
- indexer/services/socks/src/websocket/index.ts
Additional comments not posted (3)
indexer/Dockerfile.bazooka.remote (1)
14-14: Verify the necessity of including thedevdirectory.Including the
devdirectory in the Docker image might increase the image size or expose unnecessary files. Ensure that this inclusion is necessary for the build or runtime environment.Check if the
devdirectory is required for the application to function correctly in the intended environment.indexer/services/socks/src/index.ts (2)
44-54: Ensure logging messages are informative and consistent.The logging messages for instance ID retrieval are clear and informative. Verify that these messages are consistent with other logging practices in the codebase.
Check other parts of the codebase to ensure logging messages follow a consistent format and provide useful information.
1-3: Verify the correctness of the new imports.Ensure that
setInstanceIdandgetInstanceIdare correctly implemented and exported from@dydxprotocol-indexer/base.Run the following script to verify the implementation and export of these functions:
Verification successful
Imports are correctly implemented and exported.
The functions
setInstanceIdandgetInstanceIdare correctly implemented and exported from@dydxprotocol-indexer/base. The imports inindex.tsare valid.
setInstanceIdandgetInstanceIdare exported fromindexer/packages/base/src/instance-id.ts.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation and export of `setInstanceId` and `getInstanceId`. # Test: Search for the function implementations. Expect: Implementations and exports of both functions. ast-grep --lang typescript --pattern $'export function setInstanceId() { $$$ }' ast-grep --lang typescript --pattern $'export function getInstanceId() { $$$ }'Length of output: 161
Script:
#!/bin/bash # Description: Search for any export of `setInstanceId` and `getInstanceId`. # Search for any export of `setInstanceId`. rg 'export.*setInstanceId' --context 5 # Search for any export of `getInstanceId`. rg 'export.*getInstanceId' --context 5Length of output: 1455
|
backport release/indexer/v6.x |
|
backport release/indexer/v5.x |
|
@Mergifyio backport release/indexer/v6.x |
✅ Backports have been created
|
|
https://github.com/Mergifyio backport release/indexer/v5.x |
✅ Backports have been created
|
(cherry picked from commit 76bd1c8)
(cherry picked from commit 76bd1c8) # Conflicts: # indexer/pnpm-lock.yaml
Co-authored-by: roy-dydx <[email protected]>
Co-authored-by: roy-dydx <[email protected]> Co-authored-by: Roy Li <[email protected]>
Changelist
Title
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
Documentation
Tests
Chores