feat(server/v2): wire telemetry + server refactors #21746
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes involve multiple updates across various files in the server's API, primarily focusing on the telemetry functionality. Key modifications include the introduction of new configuration options, restructuring of existing functions, and enhancements to type assertions. The telemetry server is now integrated into the command initialization process, reflecting a more cohesive setup for monitoring and metrics. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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
requested review from
kocubinski,
a team and
hieuvubk
as code owners
|
@julienrbrt your pull request is missing a changelog! |
|
We just need to update confix for the config migration. |
server/v2/api/telemetry/config.go
Outdated
| func DefaultConfig() *Config { | ||
| return &Config{ | ||
| Enable: true, | ||
| Address: "localhost:1338", |
There was a problem hiding this comment.
In v2, as discussed yesterday, the telemetry endpoint will be in another port than grpc gateway.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
server/v2/api/grpc/config.go (1)
7-8: Consider reinstating field comments for better documentation.While the functionality remains unchanged, the removal of comments describing the default values for
Address,MaxRecvMsgSize, andMaxSendMsgSizemight impact code readability and maintainability. Consider adding inline comments to provide context for these configuration values, especially forMaxRecvMsgSizeandMaxSendMsgSize, as their default values are not immediately obvious.Example:
return &Config{ Enable: true, Address: "localhost:9090", // Default gRPC server address MaxRecvMsgSize: 1024 * 1024 * 10, // 10 MB MaxSendMsgSize: math.MaxInt32, // Maximum possible value }server/v2/api/telemetry/config.go (1)
Line range hint
19-72: Config struct is well-defined, but has some inconsistenciesThe
Configstruct is well-structured and follows Go naming conventions. The field comments are clear and informative, providing good documentation for users of this configuration.However, there are a few points to address:
Breaking change: The
Enabledfield has been renamed toEnable. This is a breaking change that should be documented and possibly reconsidered if backward compatibility is important.Inconsistent tag values:
- The
MetricsSinkfield has atypemapstructure tag, which doesn't match its field name.- The
StatsdAddrfield has astats-addrtoml tag, which doesn't match its field name.Consider the following changes:
- If backward compatibility is required, revert the field name change from
EnabletoEnabled.- Update the inconsistent tags:
- MetricsSink string `mapstructure:"type" toml:"metrics-sink" comment:"MetricsSink defines the type of metrics backend to use. Default is in memory"` + MetricsSink string `mapstructure:"metrics-sink" toml:"metrics-sink" comment:"MetricsSink defines the type of metrics backend to use. Default is in memory"` - StatsdAddr string `mapstructure:"statsd-addr" toml:"stats-addr" comment:"StatsdAddr defines the address of a statsd server to send metrics to. Only utilized if MetricsSink is set to \"statsd\" or \"dogstatsd\"."` + StatsdAddr string `mapstructure:"statsd-addr" toml:"statsd-addr" comment:"StatsdAddr defines the address of a statsd server to send metrics to. Only utilized if MetricsSink is set to \"statsd\" or \"dogstatsd\"."`These changes will ensure consistency between field names and their corresponding tags.
simapp/v2/simdv2/cmd/commands.go (1)
85-85: LGTM: Telemetry initialization added correctly.The addition of
telemetry.New[T]()to theserverv2.AddCommandsfunction call correctly integrates telemetry functionality into the server setup. This change aligns with the PR objectives and addresses the requirements outlined in issue #21732.Consider wrapping the entire
serverv2.AddCommandscall in a separate error check for improved error handling and clarity. For example:if err := serverv2.AddCommands( rootCmd, newApp, logger, initServerConfig(), cometbft.New( &genericTxDecoder[T]{txConfig}, initCometOptions[T](), initCometConfig(), ), grpc.New[T](), store.New[T](newApp), telemetry.New[T](), ); err != nil { logger.Error("failed to add commands", "error", err) os.Exit(1) }This approach provides more context in case of an error and allows for proper logging before exiting.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (6)
- server/v2/api/grpc/config.go (1 hunks)
- server/v2/api/grpcgateway/server.go (1 hunks)
- server/v2/api/telemetry/config.go (1 hunks)
- server/v2/api/telemetry/metrics.go (2 hunks)
- server/v2/api/telemetry/server.go (1 hunks)
- simapp/v2/simdv2/cmd/commands.go (2 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
server/v2/api/grpc/config.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/api/grpcgateway/server.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/api/telemetry/config.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/api/telemetry/metrics.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.simapp/v2/simdv2/cmd/commands.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (7)
server/v2/api/grpc/config.go (1)
Line range hint
1-53: Overall structure and documentation look good.The file maintains a clear structure with well-documented types and functions. The
Configstruct and its fields have descriptive comments, which is excellent for maintainability. The helper functionsOverwriteDefaultConfigandDisableprovide useful configuration options. The code adheres to the Uber Go Style Guide, with consistent naming conventions and structure.server/v2/api/telemetry/config.go (1)
3-17: LGTM: DefaultConfig function is well-structuredThe
DefaultConfig()function is clear, concise, and follows Go naming conventions. It provides a good set of default values for theConfigstruct.Could you please verify if "localhost:1338" is the intended default address for the metrics server? This seems to be a non-standard port, so we should ensure it's the correct choice.
server/v2/api/grpcgateway/server.go (1)
20-23: LGTM! Good improvement in type assertions and formatting.The changes in the variable declaration section are well-implemented:
- The use of a variable block with parentheses for grouping related declarations is consistent with the Uber Golang style guide.
- Adding the type assertion for
serverv2.HasConfigenhances type safety by ensuringGRPCGatewayServerimplements this interface.- The formatting and alignment of the type assertions improve code readability.
These changes contribute to better code organization and type checking without introducing any issues.
server/v2/api/telemetry/metrics.go (3)
61-61: Improved function signature and error handling.The changes to the function signature and error handling are good improvements:
- The function name
NewMetricsis more descriptive and avoids stuttering (good alignment with Uber Go Style Guide).- Using a pointer for the config parameter (
*Config) can be more efficient, especially for larger structs.- The return signature is simplified by removing the blank identifier.
- Renaming
rerrtoerris more idiomatic in Go.
88-91: Consistent error handling in deferred function.The deferred function now correctly checks for
errinstead ofrerr, maintaining consistency with the error variable renaming.
61-61: Verify impact of function signature changes.The changes to the function signature (name and parameter type) are improvements, but they may affect other parts of the codebase:
- The function name change from
NewtoNewMetricswill require updates in all places where this function is called.- Changing the parameter type to a pointer (
*Config) may require updates in the calling code to pass a pointer instead of a value.Please ensure that all usages of this function have been updated accordingly throughout the codebase.
Run the following script to check for any remaining usages of the old function signature:
simapp/v2/simdv2/cmd/commands.go (1)
18-18: LGTM: Import statement for telemetry package added correctly.The new import statement for the telemetry package is properly formatted and consistent with the existing import style. This addition is necessary for the telemetry functionality being introduced.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Metrics were previously sent to the comet metric server. This is a good change but we should default to sending to comet if an endpoint is not shared. |
How so? It looks like it was on grpc gateway: Line 559 in dcf00cf |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
server/v2/api/grpc/server.go (1)
151-151: Unnecessary change detected.The modification on this line appears to be unnecessary as it doesn't alter the existing logic. The condition
s.config == nil || *s.config == (Config{})remains the same as before.Consider reverting this change to maintain code clarity and avoid confusion. If there was a specific reason for this modification, please provide context in a comment.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (4)
- server/v2/api/grpc/server.go (1 hunks)
- server/v2/api/grpcgateway/server.go (2 hunks)
- server/v2/api/telemetry/server.go (1 hunks)
- server/v2/store/server.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/v2/api/telemetry/server.go
🧰 Additional context used
📓 Path-based instructions (3)
server/v2/api/grpc/server.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/api/grpcgateway/server.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/store/server.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (3)
server/v2/store/server.go (1)
Line range hint
71-76: Verify the impact of the Config() method signature changeThe AI-generated summary mentions that the
Config()method signature has been updated tofunc (g *StoreComponent[T]) Config() any. This change is not visible in the provided code snippet, but it's important to consider its implications.Please verify the following:
- Confirm that the method signature has indeed been changed to return
any.- If confirmed, ensure that all call sites of this method are updated to handle the
anyreturn type safely, potentially requiring type assertions.- Consider if this change aligns with the project's design principles regarding type safety.
You can use the following script to check for usage of the
Config()method:This will help identify areas that might need updates due to the signature change.
✅ Verification successful
Verification Successful: The
Config()Method Signature Change is Properly ImplementedThe
Config()method inserver/v2/store/server.gohas been successfully updated to returnany. A thorough search revealed no usages of this method outside its definition, ensuring that there are no places in the codebase requiring adjustments to handle the new return type.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of the Config() method rg --type go 'Config\(\)' -C 3Length of output: 110977
Script:
#!/bin/bash # Verify the signature of the Config() method in server/v2/store/server.go rg --type go '^func\s+\(.*\)\s+Config\(\)\s+any' server/v2/store/server.goLength of output: 119
Script:
#!/bin/bash # Search for all usages of the Config() method excluding its definition rg --type go 'Config\(\)' --invert-match 'func\s+\(.*\)\s+Config\(\)\s+any' -l | while read -r file; do echo "Checking file: $file" rg -n 'Config\(\)' "$file" | while read -r line; do line_number=$(echo "$line" | cut -d':' -f2) usage=$(echo "$line" | cut -d':' -f3-) # Check if the usage includes a type assertion or is assigned to an interface{} if ! echo "$usage" | grep -q '(\w+),' && ! echo "$usage" | grep -q 'any'; then echo "Potential type handling issue at $file:$line_number" echo " Usage: $usage" fi done doneLength of output: 168
Script:
#!/bin/bash # Search for all usages of the Config() method excluding its definition # and check for proper type handling. # Find all files that call Config() but are not defining it files=$(rg --type go 'Config\(\)' --invert-match 'func\s+\(.*\)\s+Config\(\)\s+any' -l) if [ -z "$files" ]; then echo "No usages of Config() method found outside its definition." exit 0 fi # Iterate over each file and check each usage for file in $files; do echo "Checking file: $file" # Find lines where Config() is called rg -n 'Config\(\)' "$file" | while read -r line; do line_number=$(echo "$line" | cut -d':' -f2) usage=$(echo "$line" | cut -d':' -f3-) # Check if the usage includes a type assertion or is assigned to 'any' if ! echo "$usage" | grep -q 'config\s*:=\s*g\.Config()\s*\.(\w+)' && \ ! echo "$usage" | grep -q '.*Config\(\)\s*\.\w\+'; then echo "Potential type handling issue at $file:$line_number" echo " Usage: $usage" fi done doneLength of output: 315
server/v2/api/grpcgateway/server.go (2)
20-23: LGTM: Improved type assertions forGRPCGatewayServerThe grouping of type assertions into a
varblock enhances readability and aligns with the Uber Golang style guide. The addition of theserverv2.HasConfigassertion correctly indicates thatGRPCGatewayServernow implements this interface, which is consistent with the changes in theConfigmethod.
75-75: LGTM: Improved nil check inConfigmethodThe modification to dereference the
configpointer before comparison with an emptyConfigstruct is correct and more accurate. This change ensures that the method properly handles both nil pointers and non-nil pointers to empty structs, aligning with Go best practices for pointer comparisons.
julienrbrt
force-pushed
the
julien/wire-telemetry
branch
from
5c7a6b6
to
11b33bb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
server/v2/cometbft/server.go (1)
274-274: LGTM with a suggestion for improvement.The change improves the condition by making it more specific and avoiding comparison to a zero struct, which aligns with the Uber Go Style Guide. However, consider if other fields of
AppTomlConfigare also important for determining whether a new default configuration should be created. If so, you might want to add checks for those fields as well.If other fields are relevant, consider creating a method on
AppTomlConfigto check if it's effectively empty:func (c *AppTomlConfig) IsEffectivelyEmpty() bool { return c == nil || (c.Address == "" /* && check other relevant fields */) }Then use it in the
Config()method:if s.config.AppTomlConfig.IsEffectivelyEmpty() { // ... create new default configuration }This would encapsulate the logic for determining if a new configuration is needed and make it easier to maintain if the criteria change in the future.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (5)
- server/v2/api/grpc/server.go (1 hunks)
- server/v2/api/grpcgateway/server.go (2 hunks)
- server/v2/api/telemetry/server.go (1 hunks)
- server/v2/cometbft/server.go (1 hunks)
- server/v2/store/server.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- server/v2/api/grpc/server.go
- server/v2/api/grpcgateway/server.go
- server/v2/api/telemetry/server.go
- server/v2/store/server.go
🧰 Additional context used
📓 Path-based instructions (1)
server/v2/cometbft/server.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
julienrbrt
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (5)
server/v2/api/grpcgateway/config.go (1)
5-6: LGTM with a minor suggestionThe addition of the
Addressfield with a default value of "localhost:1317" is appropriate. This provides a sensible default for local development.Consider extracting the default address as a package-level constant for better maintainability:
+const DefaultAddress = "localhost:1317" func DefaultConfig() *Config { return &Config{ Enable: true, - Address: "localhost:1317", + Address: DefaultAddress, } }server/v2/store/server.go (1)
22-23: Update comment to reflect new struct nameThe struct has been renamed from
StoreComponenttoServer, but the comment above it still mentionsStoreComponent. Please update the comment to maintain consistency with the new struct name.Consider updating the comment as follows:
-// StoreComponent manages store config and contains prune & snapshot commands +// Server manages store config and contains prune & snapshot commandsserver/v2/api/grpcgateway/server.go (3)
26-33: LGTM: Server struct refactored with improved naming.The renaming of
GRPCGatewayServertoServersimplifies the struct name, enhancing readability. The addition of theserver *http.Serverfield suggests an improvement in server management.Consider adding a comment for the
serverfield to explain its purpose, similar to the other fields:server *http.Server // HTTP server for serving gRPC-Gateway endpoints
Line range hint
65-82: LGTM: Name and Config methods updated appropriately.The
NameandConfigmethods have been correctly updated to reflect the struct renaming. The simplification of theConfigmethod by removing the unnecessary check for an emptyConfigstruct is a good improvement.Consider adding a comment to explain the purpose of the
cfgOptionsslice and how it's used to override the default configuration:// Apply configuration options to override defaults for _, opt := range s.cfgOptions { opt(cfg) }
99-119: LGTM: Start method significantly improved.The
Startmethod has been well-refactored to create and start an HTTP server, aligning with the addition of thehttp.Serverfield in the struct. The error handling is more robust, correctly distinguishing between server startup errors and normal shutdown.Consider adding a context timeout for the
ListenAndServecall to ensure the server doesn't hang indefinitely on shutdown:go func() { if err := s.server.ListenAndServe(); err != nil && err != http.ErrServerClosed { s.logger.Error("failed to start gRPC-Gateway server", "error", err) } }() select { case <-ctx.Done(): return s.Stop(context.Background()) case <-time.After(time.Second): // Allow a short time for server to start return nil }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
server/v2/go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
- server/v2/api/grpc/server.go (4 hunks)
- server/v2/api/grpcgateway/config.go (1 hunks)
- server/v2/api/grpcgateway/server.go (5 hunks)
- server/v2/api/telemetry/config.go (1 hunks)
- server/v2/api/telemetry/server.go (1 hunks)
- server/v2/go.mod (0 hunks)
- server/v2/store/commands.go (1 hunks)
- server/v2/store/server.go (2 hunks)
- server/v2/store/snapshot.go (6 hunks)
💤 Files not reviewed due to no reviewable changes (1)
- server/v2/go.mod
🚧 Files skipped from review as they are similar to previous changes (1)
- server/v2/api/telemetry/config.go
🧰 Additional context used
📓 Path-based instructions (7)
server/v2/api/grpc/server.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/api/grpcgateway/config.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/api/grpcgateway/server.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.server/v2/store/commands.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/store/server.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/store/snapshot.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (31)
server/v2/api/grpcgateway/config.go (1)
13-15: LGTM: Well-documented addition to the Config structThe new
Addressfield is properly integrated into theConfigstruct with appropriate struct tags and a clear, descriptive comment. This addition enhances the configurability of the gRPC-gateway server.server/v2/store/server.go (6)
15-17: LGTM: Interface implementations updated correctlyThe interface implementations have been properly updated to use
Serverinstead ofStoreComponent, maintaining consistency with the type renaming. The use of blank identifier_ensures compile-time verification of interface implementation.
29-30: LGTM: Constructor function updated correctlyThe constructor function
Newhas been properly updated to return*Server[T]instead of*StoreComponent[T], maintaining consistency with the struct renaming. The implementation is correct and follows the Golang style guide.
33-34: LGTM: Init method updated correctlyThe
Initmethod has been properly updated with the new receiver type*Server[T]and parameter names. These changes are consistent with the struct renaming and improve code readability.
45-57: LGTM: Name, Start, and Stop methods updated correctlyThe
Name,Start, andStopmethods have been properly updated with the new receiver type*Server[T]and parameter names. These changes are consistent with the struct renaming and improve code readability. The method implementations remain unchanged, which is correct.
Line range hint
57-70: LGTM: CLICommands method updated correctlyThe
CLICommandsmethod has been properly updated with the new receiver type*Server[T]and parameter names. These changes are consistent with the struct renaming and improve code readability. The method implementation remains unchanged, which is correct.
71-76: LGTM: Config method updated and improvedThe
Configmethod has been properly updated with the new receiver type*Server[T]and parameter names. These changes are consistent with the struct renaming and improve code readability.Additionally, the condition for returning the default config has been improved to address the potential nil pointer dereference issue mentioned in the past review comments. The new condition
s.config == nil || s.config.AppDBBackend == ""is more robust as it checks both for a nil config and an empty AppDBBackend.Great job on addressing the previous feedback and improving the code safety!
server/v2/api/telemetry/server.go (10)
1-13: LGTM: Import statements are well-organized and necessary.The import statements are correctly ordered and include all necessary packages for the telemetry server implementation.
15-18: LGTM: Interface checks are correctly implemented.The interface checks ensure that the
Servertype correctly implements theserverv2.ServerComponentandserverv2.HasConfiginterfaces, providing compile-time verification.
20-20: LGTM: ServerName constant is well-defined.The
ServerNameconstant follows Go naming conventions and provides a centralized definition for the server name.
22-27: LGTM: Server struct is well-defined with appropriate fields.The
Serverstruct is correctly defined using generics, allowing flexibility for different transaction types. The struct fields cover all necessary components for the telemetry server.
29-32: LGTM: New function is correctly implemented.The
Newfunction correctly creates and returns a newServerinstance, properly utilizing the generic typeT.
34-37: LGTM: Name method is correctly implemented.The
Namemethod correctly returns theServerNameconstant, fulfilling theserverv2.ServerComponentinterface requirement.
39-45: LGTM: Config method is correctly implemented with improved checks.The
Configmethod correctly implements theserverv2.HasConfiginterface. The check for an emptyAddressfield is an improvement over the previous version, addressing the issue raised in a past review comment.
47-65: LGTM: Init method is comprehensive and well-implemented.The
Initmethod correctly initializes the server with the provided configuration and logger. It properly unmarshals the configuration and sets up the metrics. The error handling is appropriate, and the inclusion of metrics initialization in this method improves the overall structure.
93-100: LGTM: Stop method is correctly implemented.The
Stopmethod appropriately shuts down the HTTP server. It correctly checks if the server is enabled and initialized before attempting to shut down. The use of the provided context for shutdown allows for proper timeout control.
1-126: Overall implementation is solid and meets the PR objectives.The telemetry server implementation in this file is well-structured and follows Go best practices. It successfully integrates telemetry configuration and setup within the server/v2 API server, addressing the requirements outlined in the PR objectives. The use of generics, proper interface implementations, and comprehensive error handling contribute to a robust and flexible solution.
While some minor improvements have been suggested for error handling and server startup, the overall implementation is commendable and achieves the goals set out in the linked issue #21732.
server/v2/api/grpcgateway/server.go (4)
19-21: LGTM: Type assertions are correct and useful.The type assertions for
Server[transaction.Tx]implementingServerComponentandHasConfiginterfaces are well-placed. They provide compile-time checks, ensuring that theServerstruct adheres to the expected interfaces.
Line range hint
37-64: LGTM: Server initialization updated consistently.The
Newfunction has been correctly updated to return*Server[T], maintaining consistency with the struct renaming. The initialization logic remains largely unchanged, preserving existing behavior while incorporating the structural changes. The continued use of generics with[T transaction.Tx]maintains type safety.
121-127: LGTM: Stop method correctly updated for graceful shutdown.The
Stopmethod has been well-updated to use theShutdownmethod on the HTTP server, ensuring a graceful stoppage. The logging of the server stop enhances observability. These changes align well with the addition of the HTTP server and the overall struct renaming.
136-138: LGTM: Constant added for GRPC block height header.The addition of the
GRPCBlockHeightHeaderconstant improves code readability and maintainability. The constant name follows Go naming conventions, which is good for consistency.server/v2/api/grpc/server.go (2)
151-151: Improved configuration checkThe modification to check for both
nilconfig and emptyAddressprovides a more precise condition for determining if the configuration is effectively uninitialized. This change aligns with the Uber Go Style Guide by using more specific checks.
175-177: Improved server start logic and loggingThe changes in the
Startmethod offer several improvements:
- Added an informational log statement, enhancing observability.
- Simplified error handling by directly returning errors from
s.grpcSrv.Serve(listener).- Removed the unnecessary goroutine and error channel, reducing complexity.
These modifications align with the Uber Go Style Guide by simplifying error handling and improving logging practices.
server/v2/store/commands.go (1)
20-20: Method receiver type changed fromStoreComponent[T]toServer[T]The method signature for
PrunesCmdhas been updated to useServer[T]as the receiver type instead ofStoreComponent[T]. This change suggests a shift in the architectural design, potentially moving the pruning functionality to a higher-level component.While the change itself adheres to the Golang style guide, it's important to consider the following:
- Ensure that all calls to
PrunesCmdhave been updated to use theServer[T]type.- Verify that the
Server[T]type has access to all necessary data and methods previously available inStoreComponent[T].- Update any documentation or comments that reference this method to reflect the new receiver type.
To ensure this change doesn't introduce any issues, please run the following command to check for any remaining references to
StoreComponent[T].PrunesCmd:If this command returns any results, those locations may need to be updated to use the new
Server[T]type.server/v2/store/snapshot.go (7)
79-79: LGTM. Verify usage in other parts of the codebase.The change from
StoreComponent[T]toServer[T]as the receiver type is consistent with the refactoring. Ensure that all calls to this method are updated accordingly in other parts of the codebase.#!/bin/bash # Search for any remaining usage of StoreComponent.RestoreSnapshotCmd rg "StoreComponent\[.\]\.RestoreSnapshotCmd" --type go
142-142: LGTM. Verify usage in other parts of the codebase.The change from
StoreComponent[T]toServer[T]as the receiver type is consistent with the refactoring. Ensure that all calls to this method are updated accordingly in other parts of the codebase.#!/bin/bash # Search for any remaining usage of StoreComponent.DeleteSnapshotCmd rg "StoreComponent\[.\]\.DeleteSnapshotCmd" --type go
116-116: LGTM. Verify usage in other parts of the codebase.The change from
StoreComponent[T]toServer[T]as the receiver type is consistent with the refactoring. Ensure that all calls to this method are updated accordingly in other parts of the codebase.#!/bin/bash # Search for any remaining usage of StoreComponent.ListSnapshotsCmd rg "StoreComponent\[.\]\.ListSnapshotsCmd" --type go
Line range hint
28-263: Overall LGTM. Comprehensive codebase check recommended.The changes in this file consistently update the receiver type from
StoreComponent[T]toServer[T]for all snapshot-related functions. This refactoring appears to be part of a larger architectural change. While the individual function implementations remain unchanged, it's crucial to ensure that all references to these methods throughout the codebase are updated to use the newServer[T]type.#!/bin/bash # Search for any remaining usage of StoreComponent in the entire codebase rg "StoreComponent\[.\]" --type go # List all files that still contain StoreComponent rg -l "StoreComponent\[.\]" --type go # Check for any TODO or FIXME comments related to this refactoring rg "(TODO|FIXME).*StoreComponent" --type goConsider running these checks and updating any remaining references to
StoreComponentin the codebase to maintain consistency with this refactoring.
263-263: LGTM. Verify usage in other parts of the codebase.The change from
StoreComponent[T]toServer[T]as the receiver type is consistent with the refactoring. Ensure that all calls to this method are updated accordingly in other parts of the codebase.#!/bin/bash # Search for any remaining usage of StoreComponent.LoadArchiveCmd rg "StoreComponent\[.\]\.LoadArchiveCmd" --type go
170-170: LGTM. Verify usage in other parts of the codebase.The change from
StoreComponent[T]toServer[T]as the receiver type is consistent with the refactoring. Ensure that all calls to this method are updated accordingly in other parts of the codebase.#!/bin/bash # Search for any remaining usage of StoreComponent.DumpArchiveCmd rg "StoreComponent\[.\]\.DumpArchiveCmd" --type go
28-28: LGTM. Verify usage in other parts of the codebase.The change from
StoreComponent[T]toServer[T]as the receiver type is consistent with the refactoring. Ensure that all calls to this method are updated accordingly in other parts of the codebase.✅ Verification successful
Verification Successful: All usages of
StoreComponent.ExportSnapshotCmdhave been updated to useServer[T]. No issues found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining usage of StoreComponent.ExportSnapshotCmd rg "StoreComponent\[.\]\.ExportSnapshotCmd" --type goLength of output: 122
Script:
#!/bin/bash # Broad search for any usage of ExportSnapshotCmd regardless of receiver type rg "\.ExportSnapshotCmd\(" --type go #!/bin/bash # Use ast-grep to find all method calls to ExportSnapshotCmd ast-grep --lang go --pattern 'CallExpr (selector: "ExportSnapshotCmd")' .Length of output: 166
julienrbrt
added
the
backport/v0.52.x
(cherry picked from commit 7fe95fc) # Conflicts: # server/v2/api/grpc/config.go # server/v2/api/grpc/server.go # server/v2/api/grpcgateway/config.go # server/v2/api/grpcgateway/server.go # server/v2/api/telemetry/config.go # server/v2/api/telemetry/metrics.go # server/v2/api/telemetry/server.go # server/v2/go.mod # server/v2/go.sum # server/v2/store/commands.go # server/v2/store/server.go # server/v2/store/snapshot.go
Description
Closes: #21732
It will use another http server than grpc gateway (on v1) because we don't want to share the same http server between (unexisting) grpc gateway server components and metrics.
It will give a better UX to not have to pass http servers around.
Additionally, it makes all server consistent and does some preliminary boilerplate wiring for #20798
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
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor