-
Notifications
You must be signed in to change notification settings - Fork 122
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Grpc Streaming Metrics #1616
Grpc Streaming Metrics #1616
Conversation
WalkthroughThe recent changes enhance the gRPC streaming functionality by integrating a logger into the Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant Client
participant App
participant GrpcStreamingManager
participant Logger
participant Metrics
Client->>App: Enable gRPC Streaming
App->>GrpcStreamingManager: Create instance with Logger
GrpcStreamingManager->>Logger: Initialize Logger
Client->>GrpcStreamingManager: Subscribe to Stream
GrpcStreamingManager->>Metrics: Emit Metrics
Client->>GrpcStreamingManager: Send Orderbook Updates
GrpcStreamingManager->>GrpcStreamingManager: Call sendStreamUpdate
GrpcStreamingManager->>Metrics: Track Metrics
GrpcStreamingManager->>Logger: Log Update
GrpcStreamingManager->>Client: Send Updates
GrpcStreamingManager->>Metrics: Emit Metrics
Poem
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Outside diff range and nitpick comments (6)
protocol/streaming/grpc/grpc_streaming_manager.go (1)
[!TIP]
Codebase VerificationThe
sendStreamUpdate
method contains error handling for theSend
operation, but there is no detailed error logging to aid in debugging issues related to stream updates.
- Consider adding more detailed error logging within the
sendStreamUpdate
method to improve debugging capabilities.Analysis chain
Line range hint
179-225
: ThesendStreamUpdate
method efficiently handles the distribution of updates to subscribers and correctly increments theGrpcEmitProtocolUpdateCount
andGrpcSendResponseToSubscriberCount
metrics. However, ensure that error handling is robust, especially in scenarios whereSend
might fail.Consider adding more detailed error logging within the
sendStreamUpdate
method to aid in debugging issues related to stream updates.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify robust error handling in `sendStreamUpdate` method. # Test: Search for error handling patterns in `sendStreamUpdate`. Expect: Proper error handling and logging. rg --type go $'sendStreamUpdate' protocol/streaming/grpc/grpc_streaming_manager.goLength of output: 279
Script:
#!/bin/bash # Description: Verify robust error handling in `sendStreamUpdate` method. # Test: Search for error handling patterns in `sendStreamUpdate`. Expect: Proper error handling and logging. rg --type go 'sendStreamUpdate' -A 20 protocol/streaming/grpc/grpc_streaming_manager.go | rg 'err|log'Length of output: 102
Script:
#!/bin/bash # Description: Verify robust error handling in `sendStreamUpdate` method. # Test: Search for error handling patterns and logging in the entire grpc_streaming_manager.go file. rg --type go 'sendStreamUpdate' -A 50 protocol/streaming/grpc/grpc_streaming_manager.go | rg 'err|log'Length of output: 157
protocol/app/app.go (5)
Line range hint
376-376
: Undefined functioninitDatadogProfiler
.Please define the function
initDatadogProfiler
or ensure it is imported correctly.
Line range hint
378-378
: Undefined functionGetEncodingConfig
.Please define the function
GetEncodingConfig
or ensure it is imported correctly.
Line range hint
489-489
: Undefined variablemaccPerms
.Please define the variable
maccPerms
or ensure it is imported correctly.
Line range hint
498-498
: Undefined methodMsgServiceRouter
.Please ensure that the method
MsgServiceRouter
is defined in theApp
struct or correctly imported.
Line range hint
506-506
: Undefined functionBlockedAddresses
.Please define the function
BlockedAddresses
or ensure it is imported correctly.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- protocol/app/app.go (1 hunks)
- protocol/lib/metrics/metric_keys.go (1 hunks)
- protocol/streaming/grpc/grpc_streaming_manager.go (9 hunks)
Additional context used
golangci-lint
protocol/app/app.go
376-376: undefined: initDatadogProfiler
378-378: undefined: GetEncodingConfig
489-489: undefined: maccPerms
498-498: app.MsgServiceRouter undefined (type *App has no field or method MsgServiceRouter)
506-506: undefined: BlockedAddresses
598-598: app.MsgServiceRouter undefined (type *App has no field or method MsgServiceRouter)
643-643: app.MsgServiceRouter undefined (type *App has no field or method MsgServiceRouter)
765-765: app.Logger undefined (type *App has no field or method Logger)
1356-1356: app.GRPCQueryRouter undefined (type *App has no field or method GRPCQueryRouter)
1362-1362: app.GRPCQueryRouter undefined (type *App has no field or method GRPCQueryRouter)
1365-1365: app.MountKVStores undefined (type *App has no field or method MountKVStores)
1366-1366: app.MountTransientStores undefined (type *App has no field or method MountTransientStores)
1367-1367: app.MountMemoryStores undefined (type *App has no field or method MountMemoryStores)
1370-1370: app.SetInitChainer undefined (type *App has no field or method SetInitChainer)
1372-1372: app.SetMempool undefined (type *App has no field or method SetMempool)
1373-1373: app.SetPreBlocker undefined (type *App has no field or method SetPreBlocker)
1374-1374: app.SetBeginBlocker undefined (type *App has no field or method SetBeginBlocker)
1375-1375: app.SetEndBlocker undefined (type *App has no field or method SetEndBlocker)
1376-1376: app.SetPrecommiter undefined (type *App has no field or method SetPrecommiter)
1377-1377: app.SetPrepareCheckStater undefined (type *App has no field or method SetPrepareCheckStater)
1381-1381: app.SetPrepareProposal undefined (type *App has no field or method SetPrepareProposal)
1382-1382: app.SetProcessProposal undefined (type *App has no field or method SetProcessProposal)
1392-1392: app.AddRunTxRecoveryHandler undefined (type *App has no field or method AddRunTxRecoveryHandler)
1395-1395: app.setupUpgradeHandlers undefined (type *App has no field or method setupUpgradeHandlers)
1396-1396: app.setupUpgradeStoreLoaders undefined (type *App has no field or method setupUpgradeStoreLoaders)
1403-1403: app.LoadLatestVersion undefined (type *App has no field or method LoadLatestVersion)
1411-1411: app.Logger undefined (type *App has no field or method Logger)
1430-1430: app.Logger undefined (type *App has no field or method Logger)
1564-1564: app.SetExtendVoteHandler undefined (type *App has no field or method SetExtendVoteHandler)
1565-1565: app.SetVerifyVoteExtensionHandler undefined (type *App has no field or method SetVerifyVoteExtensionHandler)
1573-1573: app.ChainID undefined (type *App has no field or method ChainID)
1650-1650: app.scheduleForkUpgrade undefined (type *App has no field or method scheduleForkUpgrade)
1694-1694: undefined: GenesisState
1716-1716: app.LoadVersion undefined (type *App has no field or method LoadVersion)
1796-1796: app.Query undefined (type *App has no field or method Query)
1802-1802: app.GRPCQueryRouter undefined (type *App has no field or method GRPCQueryRouter)
1812-1812: undefined: NewAnteHandler
1813-1813: undefined: HandlerOptions
1838-1838: app.SetAnteHandler undefined (type *App has no field or method SetAnteHandler, but does have setAnteHandler)
Additional comments not posted (6)
protocol/lib/metrics/metric_keys.go (1)
71-73
: The new metric keys for gRPC operations are well-defined and follow the established naming conventions.protocol/streaming/grpc/grpc_streaming_manager.go (5)
Line range hint
23-46
: The addition of alogger
field inGrpcStreamingManagerImpl
and its initialization inNewGrpcStreamingManager
enhances debugging capabilities and is a good use of dependency injection for better testability.
55-60
: TheEmitMetrics
method correctly updates theGrpcStreamSubscriberCount
metric based on the number of active subscriptions. This is a crucial metric for monitoring the usage of gRPC streams.
86-86
: CallingEmitMetrics
at the end of theSubscribe
method ensures that metrics are updated immediately after a new subscription is added, which is good for real-time monitoring.
133-137
: Refactoring the update sending logic intosendStreamUpdate
inSendOrderbookUpdates
method improves modularity and reusability of the code.
172-177
: UsingsendStreamUpdate
inSendOrderbookFillUpdates
ensures consistency in how updates are sent across different types of data, which is beneficial for maintaining a uniform interface for update dispatch.
@@ -1931,7 +1931,7 @@ func getGrpcStreamingManagerFromOptions( | |||
) (manager streamingtypes.GrpcStreamingManager) { | |||
if appFlags.GrpcStreamingEnabled { | |||
logger.Info("GRPC streaming is enabled") | |||
return streaming.NewGrpcStreamingManager() | |||
return streaming.NewGrpcStreamingManager(logger) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure proper initialization of GrpcStreamingManager
with logger.
- return streaming.NewGrpcStreamingManager(logger)
+ return streaming.NewGrpcStreamingManager(logger, someOtherNecessaryParams)
Committable suggestion was skipped due low confidence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- protocol/streaming/grpc/grpc_streaming_manager.go (9 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/streaming/grpc/grpc_streaming_manager.go
Adds metrics on:
Summary by CodeRabbit
New Features
Refactor
Improvements
Subscribe()
method to update metrics, improving visibility into subscription activities.