fix(server/v2/api/telemetry): avoid panic due to no telemetry address #23022
Conversation
mmsqe
requested review from
kocubinski,
julienrbrt,
a team and
hieuvubk
as code owners
📝 WalkthroughWalkthroughThe pull request introduces enhancements to the telemetry configuration system in the Cosmos SDK. It adds new configuration options for the telemetry server, allowing more flexible initialization and control. The changes include creating a new configuration option type, updating the server constructor to accept configuration options, and modifying the testnet command to integrate telemetry server initialization with a default port. Changes
Sequence DiagramsequenceDiagram
participant User
participant TelemetryConfig
participant TelemetryServer
User->>TelemetryConfig: Create configuration options
User->>TelemetryServer: Initialize with config options
TelemetryServer->>TelemetryConfig: Apply configuration
TelemetryServer-->>User: Return configured server
Possibly related PRs
Suggested labels
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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
Documentation and Community
|
github-actions
bot
added
C:server/v2
julienrbrt
changed the title
julienrbrt
added
the
backport/v0.52.x
maybe try TestValidatorDoubleSign, when add a new node |
|
Yes, thanks! I can reproduce, |
![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
🧹 Nitpick comments (5)
simapp/v2/simdv2/cmd/testnet.go (3)
187-191: Consider short comments explaining these port constants.
Adding these port constants is a beneficial step for clarity. It might be helpful to add short inline comments describing each port’s purpose, especially for new contributors.
202-202: Check if telemetry configuration should be customizable.
Calling telemetry.DefaultConfig() is straightforward; however, if the testnet environment needs custom telemetry settings (e.g., enable/disable, different address), consider making them configurable via command flags, similar to the other components (gRPC, REST).
227-231: Recommended to validate user-input address before assigning.
This code sets a local IP address with a port offset. You might consider verifying whether the resulting address is valid, especially when the user sets a custom address, to prevent subtle errors.server/v2/api/telemetry/server.go (1)
87-93: Consider caching the final merged configuration.
When Config() is called repeatedly, you are re-building a new default config each time. You might store that merged config in s.config after applying cfgOptions to avoid reapplying them on each call.server/v2/api/telemetry/config.go (1)
62-78: Enable advanced customization through CfgOption.
These helper functions (OverwriteDefaultConfig and Disable) nicely separate default setting and dynamic overrides. One minor suggestion: if the user calls Disable(), consider setting “Address” to an empty string or adding a note that the port remains allocated even when telemetry is disabled.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
server/v2/api/telemetry/config.go(1 hunks)server/v2/api/telemetry/server.go(3 hunks)simapp/v2/simdv2/cmd/testnet.go(5 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
server/v2/api/telemetry/config.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
simapp/v2/simdv2/cmd/testnet.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/api/telemetry/server.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (5)
simapp/v2/simdv2/cmd/testnet.go (2)
25-25: Good use of the telemetry import.
The addition of "telemetry" import is aligned with the broader server configurability changes. Ensure that all references to this import are indeed necessary and used.
367-368: Telemetry server integration looks consistent.
You properly configured telemetry as one of the components in “serverv2.NewServer”. Ensure that the rest of the stack can gracefully handle cases when the telemetry server fails to start or is disabled.
server/v2/api/telemetry/server.go (3)
24-28: Well-structured fields in the Server struct.
The introduction of cfgOptions provides a flexible approach to supply configuration overrides. This design choice aligns well with the rest of the server’s modular approach.
Line range hint 32-41: Ensure consistent error handling and concurrency.
The code sets srv.cfgOptions but does not apply them if the server’s config is already populated. Double-check concurrency scenarios or repeated calls to New() from different goroutines are managed safely, especially if server creation is invoked dynamically.
71-79: Good approach to partial server creation for config manipulation.
The “NewWithConfigOptions” method is handy for retrieving and customizing the telemetry configuration without fully starting the server. Documenting that it’s not fully operational should help reduce confusion.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
server/v2/api/telemetry/config.go (1)
66-71: Consider adding validation for the new configurationWhile the implementation is correct, consider adding validation to ensure the new configuration meets minimum requirements.
func OverwriteDefaultConfig(newCfg *Config) CfgOption { return func(cfg *Config) { + if newCfg == nil { + return + } *cfg = *newCfg } }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
server/v2/api/telemetry/config.go(1 hunks)server/v2/api/telemetry/server.go(3 hunks)simapp/v2/simdv2/cmd/testnet.go(5 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
server/v2/api/telemetry/config.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
simapp/v2/simdv2/cmd/testnet.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/api/telemetry/server.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (9)
server/v2/api/telemetry/server.go (4)
24-28: LGTM: Good addition of configuration options
The addition of cfgOptions field to the Server struct enables flexible configuration management.
Line range hint 32-41: LGTM: Constructor properly handles configuration options
The constructor correctly accepts and assigns configuration options while maintaining backward compatibility.
71-78: LGTM: Well-documented config-only constructor
The function is well-documented about its limitations and provides a clean way to create a server instance for configuration purposes only.
86-93: LGTM: Robust configuration handling
The method properly handles nil config cases and correctly applies configuration options, effectively preventing panics due to missing telemetry address.
server/v2/api/telemetry/config.go (2)
63-64: LGTM: Well-defined configuration option type
The CfgOption type provides a type-safe way to modify configurations.
73-78: LGTM: Clean implementation of telemetry disable function
The function provides a simple way to disable telemetry functionality.
simapp/v2/simdv2/cmd/testnet.go (3)
187-191: LGTM: Well-organized port definitions
The telemetry port is properly defined alongside other service ports.
228-231: LGTM: Proper telemetry configuration for single machine setup
The telemetry configuration correctly handles port offsets for multiple instances on a single machine.
367-368: LGTM: Clean integration of telemetry server
The telemetry server is properly initialized and integrated with other server components.
…ort #23022) (#23033) Co-authored-by: mmsqe <[email protected]> Co-authored-by: Julien Robert <[email protected]>
Description
Closes #23021
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
Bug Fixes
Documentation