using zap logger #22
using zap logger #22
Conversation
WalkthroughThe pull request introduces significant changes to the logging and error handling mechanisms across multiple files in the codebase. The primary focus is on replacing the Changes
Possibly related PRs
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
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: 17
🧹 Outside diff range and nitpick comments (28)
common/zaplogger/logger.go (5)
1-7: Add package documentationConsider adding a package documentation comment that describes the purpose and usage of this logging utility package.
+// Package zaplogger provides a centralized logging utility based on zap logger. +// It manages a global logger instance and provides context-aware logging capabilities. package zaplogger
9-9: Consider making the logger configuration environment-awareThe current implementation hardcodes
Development: true, which enables development mode logging. This might not be suitable for production environments as it can impact performance and log verbosity.Consider:
- Making the configuration configurable through environment variables or flags
- Adding different preset configurations for development and production environments
- Making the logger variable private and providing thread-safe initialization
Example implementation:
var ( defaultOpts = &zap.Options{ Development: false, // Default to production mode } loggerInstance atomic.Value ) func init() { // Read from environment or flags if os.Getenv("LOG_DEV_MODE") == "true" { defaultOpts.Development = true } loggerInstance.Store(zap.New(zap.UseFlagOptions(defaultOpts))) }
11-17: Add function documentationThe implementation looks good, but consider adding godoc comments for better API documentation.
+// GetLogger returns the global logger instance func GetLogger() logr.Logger { return logger } +// WithLogger returns a new context with the provided logger +// It's safe to pass a nil logger, in which case a no-op logger will be used func WithLogger(ctx context.Context, logger logr.Logger) context.Context { return logr.NewContext(ctx, logger) }
19-26: Enhance error handling and add context validationWhile the implementation provides good fallback behavior, it could be improved with better error handling and context validation.
Consider this enhanced implementation:
func FromContext(ctx context.Context) logr.Logger { + if ctx == nil { + return logger + } l, err := logr.FromContext(ctx) if err != nil { + // Log the reason for falling back to default logger + logger.V(1).Info("using default logger", "reason", err.Error()) return logger } return l }Also, consider adding documentation:
+// FromContext extracts a logger from the given context. +// If the context is nil or doesn't contain a logger, +// it returns the default global logger. func FromContext(ctx context.Context) logr.Logger {
1-26: Consider adding log sanitization and level configurationAs this logger will be used throughout the application, consider implementing:
- Log level configuration to control verbosity in different environments
- Log sanitization helpers to prevent sensitive data exposure
- Structured logging guidelines to ensure consistent log format
Consider adding helper functions for common logging patterns:
// Example helper functions func WithSanitized(key string, value interface{}) logr.Logger { // Implement sanitization logic return logger.WithValues(key, sanitize(value)) } func sanitize(value interface{}) interface{} { // Implement sanitization logic for sensitive data // e.g., email addresses, tokens, etc. }module_tunnels/koupleless_http_tunnel/ark_service/ark_service.go (2)
22-41: Consider adding debug/info level logs for successful operationsWhile error logging is well implemented, adding debug or info level logs for successful operations would improve observability. Consider adding:
- Debug log before making the HTTP request
- Info log on successful installation
func (h *Service) InstallBiz(ctx context.Context, req InstallBizRequest, baseIP string, arkletPort int) (response ArkResponse, err error) { logger := zaplogger.FromContext(ctx) + logger.Debug("initiating biz installation", "baseIP", baseIP, "arkletPort", arkletPort) resp, err := h.client.R(). SetContext(ctx). SetBody(req). Post(fmt.Sprintf("http://%s:%d/installBiz", baseIP, arkletPort)) // ... error handling ... + logger.Info("successfully installed biz", "baseIP", baseIP) return response, nil
41-41: Standardize error message formattingError messages have inconsistent formatting. Consider standardizing:
- Capitalize method names consistently
- Use consistent message structure
- logger.Error(err, fmt.Sprintf("Unmarshal InstallBiz response: %s", resp.Body())) + logger.Error(err, fmt.Sprintf("failed to unmarshal install_biz response: %s", resp.Body())) - logger.Error(err, fmt.Sprintf("Unmarshal UnInstallBiz response: %s", resp.Body())) + logger.Error(err, fmt.Sprintf("failed to unmarshal uninstall_biz response: %s", resp.Body())) - logger.Error(err, fmt.Sprintf("Unmarshal QueryAllBiz response: %s", resp.Body())) + logger.Error(err, fmt.Sprintf("failed to unmarshal query_all_biz response: %s", resp.Body())) - logger.Error(err, fmt.Sprintf("Unmarshal Health response: %s", resp.Body())) + logger.Error(err, fmt.Sprintf("failed to unmarshal health response: %s", resp.Body()))Also applies to: 68-68, 95-95, 122-122
report_server/server.go (3)
Line range hint
82-84: Standardize logging approach across the fileThe file uses a mix of standard
logpackage andzaplogger, which is inconsistent with the PR's objective of transitioning to zaplogger.Replace standard log.Println calls with zaplogger:
- log.Println("Error sending to webhook:", err.Error()) + zaplogger.GetLogger().Error(err, "Error sending to webhook") - log.Println("Starting server on :8080") + zaplogger.GetLogger().Info("Starting server", "port", ":8080")Also applies to: 112-112
🧰 Tools
🪛 golangci-lint (1.62.2)
8-8: SA1019: "io/ioutil" has been deprecated since Go 1.19: As of Go 1.16, the same functionality is now provided by package [io] or package [os], and those implementations should be preferred in new code. See the specific function documentation for details.
(staticcheck)
Line range hint
106-113: Consider adding graceful shutdownThe server initialization lacks graceful shutdown handling, which could lead to connection interruptions during deployments or shutdowns.
Consider:
- Accepting a context parameter in InitReportServer
- Using http.Server with Shutdown method
- Implementing signal handling for graceful termination
Would you like me to provide an example implementation?
Line range hint
106-113: Add configuration validationThe server starts without validating if any webhooks are configured. This could lead to a running server that can't fulfill its primary purpose.
Consider adding validation:
func InitReportServer() { reportHooks := utils.GetEnv("REPORT_HOOKS", "") webhooks = strings.Split(reportHooks, ";") + + if len(webhooks) == 0 || (len(webhooks) == 1 && webhooks[0] == "") { + zaplogger.GetLogger().Warn("No webhooks configured, server will not forward any logs") + } http.HandleFunc("/log", logHandler)suite/http/suite_test.go (1)
Line range hint
88-94: Consider revising the error handling approach.While the error handling is comprehensive, consider these improvements:
- In a test suite, failing the test might be more appropriate than panicking
- The error message duplicates the tunnel key information
Consider this alternative implementation:
err = httpTunnel.Start(clientID, env) if err != nil { log.G(ctx).WithError(err).Error("failed to start tunnel", httpTunnel.Key()) - panic(fmt.Sprintf("failed to start tunnel %s", httpTunnel.Key())) + Fail(fmt.Sprintf("failed to start tunnel: %v", err)) } else { log.G(ctx).Info("Tunnel started: ", httpTunnel.Key()) }module_tunnels/koupleless_mqtt_tunnel/mqtt/mqtt.go (2)
56-64: Standardize logging approach across handlersWhile the transition to zaplogger is correct, the logging approach is inconsistent:
- Line 56: Uses fmt.Sprintf
- Line 60: Uses direct string
- Line 64: Uses fmt.Sprintf
Consider standardizing to use structured logging for better performance and consistency:
- zaplogger.GetLogger().Info(fmt.Sprintf("Received message: %s from topic: %s\n", msg.Payload(), msg.Topic())) + zaplogger.GetLogger().Info("Received message", + "payload", msg.Payload(), + "topic", msg.Topic()) - zaplogger.GetLogger().Info("Connected") + zaplogger.GetLogger().Info("Connected") // This is fine as is - zaplogger.GetLogger().Info(fmt.Sprintf("Connection lost %v\n", err)) + zaplogger.GetLogger().Info("Connection lost", + "error", err)
163-171: Standardize logging approach in subscription methodsSimilar to the handlers, consider using structured logging for consistency and better performance:
- zaplogger.GetLogger().Info(fmt.Sprintf("Subscribing to topic: %s\n", topic)) + zaplogger.GetLogger().Info("Subscribing to topic", + "topic", topic) - zaplogger.GetLogger().Info(fmt.Sprintf("Unsubscribing from topic: %s\n", topic)) + zaplogger.GetLogger().Info("Unsubscribing from topic", + "topic", topic)suite/http/mock_http_base.go (3)
Line range hint
91-91: Standardize logging patterns across the codebaseThe code currently mixes different logging approaches:
- Context-aware logging with zap:
log.G(ctx).Info()- Global logging with logrus:
logrus.Errorf()This inconsistency makes it harder to maintain and monitor logs effectively.
Standardize all logging calls to use the context-aware zap logger. Here's how to refactor the logging calls:
// In error handling for request body -logrus.Errorf("error reading body: %s", err) +log.G(ctx).Error("error reading body", "error", err) // In heartbeat error handling -logrus.Errorf("error calling heartbeat: %s", err) +log.G(ctx).Error("error calling heartbeat", "error", err) // In biz installation/uninstallation -logrus.Infof("install biz %s from http base", identity) +log.G(ctx).Info("installing biz from http base", "identity", identity) -logrus.Infof("uninstall biz %s from http base", identity) +log.G(ctx).Info("uninstalling biz from http base", "identity", identity)Note: This requires propagating the context parameter to the
processInstallBizandprocessUnInstallBizmethods.Also applies to: 111-111, 127-127, 130-130, 139-139, 156-156, 214-214, 238-238
Line range hint
83-117: Consider architectural improvements for context propagationThe current HTTP handler implementation makes it challenging to use context-aware logging consistently. The handlers and business processing methods (
processInstallBiz,processUnInstallBiz) don't have access to the request context.Consider these improvements:
- Update method signatures to accept context:
-func (b *MockHttpBase) processInstallBiz(msg []byte) []byte { +func (b *MockHttpBase) processInstallBiz(ctx context.Context, msg []byte) []byte { -func (b *MockHttpBase) processUnInstallBiz(msg []byte) []byte { +func (b *MockHttpBase) processUnInstallBiz(ctx context.Context, msg []byte) []byte {
- Pass request context in HTTP handlers:
mux.HandleFunc("/installBiz", func(w http.ResponseWriter, r *http.Request) { if b.reachable { w.WriteHeader(http.StatusOK) w.Header().Set("Content-Type", "application/json") defer r.Body.Close() body, err := io.ReadAll(r.Body) if err != nil { - logrus.Errorf("error reading body: %s", err) + log.G(r.Context()).Error("error reading body", "error", err) return } - w.Write(b.processInstallBiz(body)) + w.Write(b.processInstallBiz(r.Context(), body)) } else { w.WriteHeader(http.StatusBadGateway) } })Also applies to: 204-248
Line range hint
83-117: Improve error handling in HTTP handlersWhile not directly related to the logging transition, consider these improvements:
- Check for errors when writing responses
- Use proper error response structures
Example improvement:
mux.HandleFunc("/installBiz", func(w http.ResponseWriter, r *http.Request) { if b.reachable { - w.WriteHeader(http.StatusOK) - w.Header().Set("Content-Type", "application/json") + w.Header().Set("Content-Type", "application/json") defer r.Body.Close() body, err := io.ReadAll(r.Body) if err != nil { - logrus.Errorf("error reading body: %s", err) + w.WriteHeader(http.StatusBadRequest) + json.NewEncoder(w).Encode(map[string]string{"error": "failed to read request body"}) return } - w.Write(b.processInstallBiz(body)) + w.WriteHeader(http.StatusOK) + if err := json.NewEncoder(w).Encode(b.processInstallBiz(body)); err != nil { + log.G(r.Context()).Error("failed to write response", "error", err) + } } else { w.WriteHeader(http.StatusBadGateway) } })cmd/module-controller/main.go (4)
21-21: Consider cleaning up logging-related importsThe code still maintains imports for both the old logging system (logrus, logruslogger) and the new zaplogger. Consider removing the unused logging imports after completing the migration to zaplogger.
Also applies to: 43-43, 44-44
103-104: Address TODO and consider parameterizing QPS/Burst valuesThe QPS and Burst values are hardcoded. Consider:
- Moving these values to configuration
- Making them configurable via environment variables, similar to other parameters
Example:
-kubeConfig.QPS = 100 -kubeConfig.Burst = 200 +kubeConfig.QPS = float32(utils.GetEnvAsInt("KUBE_QPS", 100)) +kubeConfig.Burst = utils.GetEnvAsInt("KUBE_BURST", 200)Would you like me to help implement this change or create an issue to track this improvement?
Line range hint
190-192: Migrate remaining logrus error handling to zaploggerThere's still an instance of old logging pattern. Consider updating to zaplogger:
-log.G(ctx).WithError(err).Error("failed to parse HTTP_TUNNEL_LISTEN_PORT, set default port 7777") +zlogger.Error(err, "failed to parse HTTP_TUNNEL_LISTEN_PORT, set default port 7777")
Line range hint
159-166: Complete logger migration for remaining error handlersThere are still instances of the old logging pattern. Consider updating these to use zaplogger:
if err := mgr.AddHealthzCheck("healthz", healthz.Ping); err != nil { - log.G(ctx).Error(err, "unable to set up health check") + zlogger.Error(err, "unable to set up health check") os.Exit(1) } if err := mgr.AddReadyzCheck("readyz", healthz.Ping); err != nil { - log.G(ctx).Error(err, "unable to set up ready check") + zlogger.Error(err, "unable to set up ready check") os.Exit(1) } -log.G(ctx).Info("Module controller running") +zlogger.Info("Module controller running") err = mgr.Start(signals.SetupSignalHandler()) if err != nil { - log.G(ctx).WithError(err).Error("failed to start manager") + zlogger.Error(err, "failed to start manager") }Also applies to: 171-173
common/utils/utils.go (2)
189-189: Improve logging efficiency and debug-abilityConsider these improvements:
- Use structured logging instead of string formatting
- Consider using debug level for parsing errors
- Add context fields for better debugging
-zaplogger.GetLogger().Info(fmt.Sprintf("failed to parse change time %s", record.ChangeTime)) +zaplogger.GetLogger().Debug("Failed to parse change time", + "changeTime", record.ChangeTime, + "state", state)
206-217: LGTM: Good structured logging implementation with minor suggestionsThe logging implementation follows good practices with structured fields and appropriate log levels. Consider these minor enhancements:
- Add more context fields like cluster name
- Make error messages more descriptive
-logger.Error(deleteErr, "delete base node failed") +logger.Error(deleteErr, "Failed to delete base node from API server")module_tunnels/koupleless_http_tunnel/http_tunnel.go (2)
176-179: Handle Write errors in HTTP handlersThe error return value from
w.Writeis not being checked. While write errors in HTTP handlers are often not critical (as the connection might be already broken), it's good practice to log them for debugging purposes.- w.Write([]byte(err.Error())) + if _, writeErr := w.Write([]byte(err.Error())); writeErr != nil { + logger.Error(writeErr, "failed to write error response") + }🧰 Tools
🪛 golangci-lint (1.62.2)
178-178: Error return value of
w.Writeis not checked(errcheck)
Line range hint
264-267: Improve error handling patternConsider defining error variables for common errors and providing more descriptive error messages. This would improve error handling consistency and debugging experience.
+var ( + ErrNetworkInfoNotFound = errors.New("base network info not found for node") +) - return errors.New("network info not found") + return fmt.Errorf("%w: nodeID=%s", ErrNetworkInfoNotFound, nodeID)Also applies to: 299-302
controller/module_deployment_controller/module_deployment_controller.go (3)
222-227: Consider enhancing the error messageWhile the logging is implemented correctly, the error message could be more descriptive to aid in troubleshooting.
Consider this enhancement:
- logger.Info(fmt.Sprintf("failed to get cluster name of node %s", node.Name)) + logger.Info(fmt.Sprintf("failed to get cluster name of node %s: cluster label not found", node.Name))
299-299: Fix typo in error messageThere's a typo in the error message ("relicas" instead of "replicas").
Apply this fix:
- logger.Error(err, fmt.Sprintf("failed to get nodes of cluster %s, skip to update relicas", clusterName)) + logger.Error(err, fmt.Sprintf("failed to get nodes of cluster %s, skip to update replicas", clusterName))
Line range hint
1-391: Consider standardizing error handling patternsWhile the logging implementation is good, consider standardizing error handling patterns across the controller. Some suggestions:
- Define standard error message templates for common operations
- Consider implementing structured logging fields consistently
- Consider adding log level guidelines for different types of events (Info vs. Error)
module_tunnels/koupleless_mqtt_tunnel/mqtt_tunnel.go (1)
Line range hint
179-188: Avoid using shared context in anonymous functionsIn the
queryBaselineMsgCallback, the anonymous function should receive the context explicitly to prevent unintended behaviors, especially if the context may change or be canceled.Pass
ctxto the goroutine:-go func() { +go func(ctx context.Context) { // function body... -}() +}(ctx)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (16)
cmd/module-controller/main.go(8 hunks)cmd/self-test/main.go(1 hunks)common/utils/utils.go(4 hunks)common/zaplogger/logger.go(1 hunks)controller/module_deployment_controller/module_deployment_controller.go(14 hunks)go.mod(1 hunks)module_tunnels/koupleless_http_tunnel/ark_service/ark_service.go(2 hunks)module_tunnels/koupleless_http_tunnel/http_tunnel.go(22 hunks)module_tunnels/koupleless_mqtt_tunnel/mqtt/mqtt.go(3 hunks)module_tunnels/koupleless_mqtt_tunnel/mqtt_tunnel.go(15 hunks)report_server/server.go(2 hunks)suite/http/mock_http_base.go(1 hunks)suite/http/suite_test.go(2 hunks)suite/mqtt/mock_mqtt_base.go(1 hunks)suite/mqtt/module_mqtt_lifecycle_test.go(1 hunks)suite/mqtt/suite_test.go(2 hunks)
✅ Files skipped from review due to trivial changes (3)
- cmd/self-test/main.go
- suite/mqtt/mock_mqtt_base.go
- suite/mqtt/module_mqtt_lifecycle_test.go
🧰 Additional context used
🪛 GitHub Check: unit-test
controller/module_deployment_controller/module_deployment_controller.go
[failure] 198-198:
fmt.Sprintf format % is missing verb at end of string
🪛 golangci-lint (1.62.2)
controller/module_deployment_controller/module_deployment_controller.go
198-198: printf: fmt.Sprintf format % is missing verb at end of string
(govet)
module_tunnels/koupleless_http_tunnel/http_tunnel.go
178-178: Error return value of w.Write is not checked
(errcheck)
292-292: Error return value of h.QueryAllBizStatusData is not checked
(errcheck)
🔇 Additional comments (20)
module_tunnels/koupleless_http_tunnel/ark_service/ark_service.go (1)
8-8: LGTM: Clean import change for zaplogger
The import change aligns with the PR objective of transitioning to zaplogger.
report_server/server.go (1)
7-7: LGTM on zaplogger import
The import aligns with the PR objective of transitioning to zaplogger.
go.mod (1)
Line range hint 1-100: Consider removing logrus as direct dependency
Since the PR's objective is to transition from logrus to zaplogger, consider removing github.com/sirupsen/logrus from direct dependencies if it's no longer needed. This will prevent potential confusion and ensure consistent logging practices.
suite/http/suite_test.go (3)
14-15: LGTM: Import changes align with logging transition.
The new logging imports are correctly structured and properly aliased.
86-86: LGTM: Context propagation improvement.
The addition of context parameter to NewHttpTunnel aligns with Go best practices for context propagation.
Line range hint 88-94: Verify logging pattern consistency across the codebase.
Let's ensure the logging pattern used here is consistent with other files that have been transitioned to the new logging system.
✅ Verification successful
Based on the search results, I can now generate the final response as the logging pattern verification is complete.
Logging pattern is consistent across the codebase
The error logging pattern log.G(ctx).WithError(err).Error() is consistently used across the codebase, including:
- Similar tunnel start error handling in
suite/mqtt/suite_test.go - Manager start error handling in multiple files
- HTTP tunnel port parsing in
cmd/module-controller/main.go
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar error logging patterns to ensure consistency
rg -A 2 'log\.G\(ctx\)\.WithError\(err\)\.Error\(' --type go
Length of output: 1401
module_tunnels/koupleless_mqtt_tunnel/mqtt/mqtt.go (1)
7-7: LGTM: Clean import addition
The zaplogger import is correctly added and aligns with the PR objective of transitioning to zap logger.
suite/mqtt/suite_test.go (2)
115-116: LGTM: Context handling changes look consistent
The changes follow a good pattern of:
- Passing context during initialization
- Removing context from method parameters where it's stored in the struct
Let's verify this pattern is consistent across other tunnel implementations:
✅ Verification successful
LGTM: Context handling pattern is consistently implemented
The verification confirms that both tunnel implementations (HTTP and MQTT) follow the same consistent pattern:
- Both
HttpTunnelandMqttTunnelstructs storecontext.Contextas a field - Both
NewHttpTunnelandNewMqttTunnelaccept context as their first parameter - Both implementations have removed context from their
Start(clientID, env string)method signatures
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other tunnel implementations to verify context handling pattern
ast-grep --pattern 'type $tunnel struct {
$$$
}'
# Search for NewXxxTunnel functions to verify context parameter
ast-grep --pattern 'func New$_Tunnel($$$) {
$$$
}'
Length of output: 107
Script:
#!/bin/bash
# Let's try a more specific search for tunnel implementations
rg -l "Tunnel" --type go
# Then let's examine the content of these files
rg "type.*Tunnel.*struct|func New.*Tunnel|Start\(" --type go -A 3
Length of output: 8452
14-15: Verify logger initialization consistency
The code sets up both zap logger (logf.SetLogger(zap.New(...))) and logrus logger (log.L = logruslogger.FromLogrus(...)). This might lead to inconsistent logging behavior.
Let's check if this dual logger setup is intentional or if there are other files with similar patterns:
✅ Verification successful
Let me gather more context about the logging setup to understand if this is a test-specific pattern or a broader issue.
Dual logger setup is intentional for test environment
The dual logger initialization is a deliberate pattern used specifically in test files, where:
logf.SetLogger(zap)is used for controller-runtime logging in testslog.L = logruslogger.FromLogrus()is used for virtual-kubelet logging in tests
This is consistent across test suites and matches the production setup in cmd/module-controller/main.go where both loggers are also initialized. No action is needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar dual logger initialization patterns
rg -U "SetLogger.*zap.*\n.*logruslogger.FromLogrus"
Length of output: 451
Script:
#!/bin/bash
# Check for logger initialization patterns in non-test files
rg -g '!*_test.go' "SetLogger|logruslogger.FromLogrus"
# Check imports of logging packages in test files
rg -g '*_test.go' "\"github.com/virtual-kubelet/virtual-kubelet/log\"|logf \"sigs.k8s.io/controller-runtime/pkg/log\""
Length of output: 718
cmd/module-controller/main.go (2)
89-90: LGTM! Error handling properly updated
The error handling has been correctly migrated to use zaplogger with appropriate error context and default value handling.
Also applies to: 95-96
203-210: 🛠️ Refactor suggestion
Consider graceful shutdown instead of panic
While the logging is properly updated, using panic for tunnel startup failures might not be the best approach. Consider implementing a graceful shutdown:
if startFailedCount > 0 {
- panic(errors.New(fmt.Sprintf("failed to start %d tunnels", startFailedCount)))
+ zlogger.Error(fmt.Errorf("failed to start %d tunnels", startFailedCount), "shutting down")
+ cancel() // trigger graceful shutdown
+ return nil
} else if successTunnelCount == 0 {
- panic(errors.New(fmt.Sprintf("successfully started 0 tunnels")))
+ zlogger.Error(fmt.Errorf("no tunnels started successfully"), "shutting down")
+ cancel() // trigger graceful shutdown
+ return nil
}Likely invalid or redundant comment.
common/utils/utils.go (1)
6-6: LGTM: Import statement is correctly placed
module_tunnels/koupleless_http_tunnel/http_tunnel.go (3)
29-29: LGTM: Good context management implementation
The addition of the context field and its proper initialization in the constructor follows best practices for context propagation.
Also applies to: 55-63
327-329: LGTM: Well-structured logging implementation
Good use of structured logging with proper field enrichment using WithValues. This provides better context for debugging and monitoring.
Also applies to: 359-361
Line range hint 1-377: Verify context propagation in dependent packages
The context implementation looks good, but we should verify that dependent packages properly handle the propagated context.
✅ Verification successful
Context propagation implementation is correctly handled
The verification shows that context is properly propagated throughout the codebase:
- The
HttpTunnelstruct correctly passes its context to allarkServicemethod calls - The
ark_service.Servicemethods properly:- Accept context as their first parameter
- Use context in HTTP requests via
SetContext(ctx) - Utilize context for logging via
zaplogger.FromContext(ctx)
- All HTTP client calls are context-aware through resty client's context support
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if other packages properly handle the propagated context
ast-grep --pattern 'func $_(ctx context.Context, $$$) {
$$$
h.arkService.$_($$$)
$$$
}'
Length of output: 95
Script:
#!/bin/bash
# Let's check for context propagation in ark_service package
rg -A 5 "func.*\(ctx context.Context" module_tunnels/koupleless_http_tunnel/ark_service/
Length of output: 2589
Script:
#!/bin/bash
# Check how context is used in method calls to arkService
rg -A 3 "arkService\." module_tunnels/koupleless_http_tunnel/
Length of output: 1447
Script:
#!/bin/bash
# Check the Service struct and its methods in ark_service
fd -e go . module_tunnels/koupleless_http_tunnel/ark_service/ -x cat {}
Length of output: 4410
controller/module_deployment_controller/module_deployment_controller.go (3)
46-46: LGTM: Proper context-based logging implementation
The logging implementation correctly uses context and includes relevant request information.
Line range hint 62-164: LGTM: Comprehensive logging implementation in setup process
The logging implementation provides good visibility into the controller setup process with appropriate error handling and context usage.
350-357: LGTM: Well-implemented error logging
The error logging provides good context and properly uses the zaplogger with context.
module_tunnels/koupleless_mqtt_tunnel/mqtt_tunnel.go (2)
11-11: Import zaplogger for structured logging
The zaplogger package is imported to replace the previous logging library. Ensure all logging statements are updated to use zaplogger for consistency and improved logging capabilities.
87-88: 🛠️ Refactor suggestion
Pass context explicitly in utility functions
When calling utility functions like utils.OnBaseUnreachable, pass the context explicitly rather than using a field from the struct. This maintains consistent context propagation throughout the application.
Modify the call:
-utils.OnBaseUnreachable(mqttTunnel.ctx, nodeName, mqttTunnel.kubeClient)
+utils.OnBaseUnreachable(ctx, nodeName, mqttTunnel.kubeClient)Ensure ctx is passed to the OnNodeNotReady method.
Likely invalid or redundant comment.
| if resp == nil { | ||
| err = errors.New("received nil response from the server") | ||
| log.G(ctx).WithError(err).Errorf("installBiz request failed") | ||
| logger.Error(err, "installBiz request failed") | ||
| return | ||
| } | ||
|
|
||
| if err = json.Unmarshal(resp.Body(), &response); err != nil { | ||
| log.G(ctx).WithError(err).Errorf("Unmarshal InstallBiz response: %s", resp.Body()) | ||
| logger.Error(err, fmt.Sprintf("Unmarshal InstallBiz response: %s", resp.Body())) | ||
| return | ||
| } | ||
|
|
||
| return response, nil | ||
| } | ||
|
|
||
| func (h *Service) UninstallBiz(ctx context.Context, req UninstallBizRequest, baseIP string, arkletPort int) (response ArkResponse, err error) { | ||
| logger := zaplogger.FromContext(ctx) | ||
|
|
||
| resp, err := h.client.R(). | ||
| SetContext(ctx). | ||
| SetBody(req). | ||
| Post(fmt.Sprintf("http://%s:%d/uninstallBiz", baseIP, arkletPort)) | ||
|
|
||
| if err != nil { | ||
| log.G(ctx).WithError(err).Errorf("uninstall request failed") | ||
| logger.Error(err, "uninstall request failed") | ||
| return | ||
| } | ||
|
|
||
| if resp == nil { | ||
| err = errors.New("received nil response from the server") | ||
| log.G(ctx).WithError(err).Errorf("uninstall request failed") | ||
| logger.Error(err, "uninstall request failed") | ||
| return | ||
| } | ||
|
|
||
| if err = json.Unmarshal(resp.Body(), &response); err != nil { | ||
| log.G(ctx).WithError(err).Errorf("Unmarshal UnInstallBiz response: %s", resp.Body()) | ||
| logger.Error(err, fmt.Sprintf("Unmarshal UnInstallBiz response: %s", resp.Body())) | ||
| return | ||
| } | ||
|
|
||
| return response, nil | ||
| } | ||
|
|
||
| func (h *Service) QueryAllBiz(ctx context.Context, baseIP string, port int) (response QueryAllBizResponse, err error) { | ||
| logger := zaplogger.FromContext(ctx) | ||
|
|
||
| resp, err := h.client.R(). | ||
| SetContext(ctx). | ||
| SetBody(`{}`). | ||
| Post(fmt.Sprintf("http://%s:%d/queryAllBiz", baseIP, port)) | ||
|
|
||
| if err != nil { | ||
| log.G(ctx).WithError(err).Errorf("queryAllBiz request failed") | ||
| logger.Error(err, "queryAllBiz request failed") | ||
| return | ||
| } | ||
|
|
||
| if resp == nil { | ||
| err = errors.New("received nil response from the server") | ||
| log.G(ctx).WithError(err).Errorf("queryAllBiz request failed") | ||
| logger.Error(err, "queryAllBiz request failed") | ||
| return | ||
| } | ||
|
|
||
| if err = json.Unmarshal(resp.Body(), &response); err != nil { | ||
| log.G(ctx).WithError(err).Errorf("Unmarshal QueryAllBiz response: %s", resp.Body()) | ||
| logger.Error(err, fmt.Sprintf("Unmarshal QueryAllBiz response: %s", resp.Body())) | ||
| return | ||
| } | ||
|
|
||
| return | ||
| } | ||
|
|
||
| func (h *Service) Health(ctx context.Context, baseIP string, arkletPort int) (response HealthResponse, err error) { | ||
| logger := zaplogger.FromContext(ctx) | ||
|
|
||
| resp, err := h.client.R(). | ||
| SetContext(ctx). | ||
| SetBody(`{}`). | ||
| Post(fmt.Sprintf("http://%s:%d/health", baseIP, arkletPort)) | ||
|
|
||
| if err != nil { | ||
| log.G(ctx).WithError(err).Errorf("health request failed") | ||
| logger.Error(err, "health request failed") | ||
| return | ||
| } | ||
|
|
||
| if resp == nil { | ||
| err = errors.New("received nil response from the server") | ||
| log.G(ctx).WithError(err).Errorf("health request failed") | ||
| logger.Error(err, "health request failed") | ||
| return | ||
| } | ||
|
|
||
| if err = json.Unmarshal(resp.Body(), &response); err != nil { | ||
| log.G(ctx).WithError(err).Errorf("Unmarshal Health response: %s", resp.Body()) | ||
| logger.Error(err, fmt.Sprintf("Unmarshal Health response: %s", resp.Body())) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring common HTTP client operations
All methods follow the same pattern for HTTP requests and error handling. Consider extracting this into a helper method to reduce code duplication and improve maintainability.
+func (h *Service) doRequest(ctx context.Context, endpoint string, body interface{}, response interface{}) error {
+ logger := zaplogger.FromContext(ctx)
+
+ resp, err := h.client.R().
+ SetContext(ctx).
+ SetBody(body).
+ Post(endpoint)
+
+ if err != nil {
+ logger.Error(err, "request failed")
+ return err
+ }
+
+ if resp == nil {
+ err = errors.New("received nil response from the server")
+ logger.Error(err, "request failed")
+ return err
+ }
+
+ if err = json.Unmarshal(resp.Body(), response); err != nil {
+ logger.Error(err, fmt.Sprintf("unmarshal response failed: %s", resp.Body()))
+ return err
+ }
+
+ return nil
+}
func (h *Service) InstallBiz(ctx context.Context, req InstallBizRequest, baseIP string, arkletPort int) (response ArkResponse, err error) {
- logger := zaplogger.FromContext(ctx)
-
- resp, err := h.client.R().
- SetContext(ctx).
- SetBody(req).
- Post(fmt.Sprintf("http://%s:%d/installBiz", baseIP, arkletPort))
-
- if err != nil {
- logger.Error(err, "installBiz request failed")
- return
- }
- // ... rest of error handling
+ err = h.doRequest(
+ ctx,
+ fmt.Sprintf("http://%s:%d/installBiz", baseIP, arkletPort),
+ req,
+ &response,
+ )
return response, err
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| logger := zaplogger.FromContext(ctx) | |
| resp, err := h.client.R(). | |
| SetContext(ctx). | |
| SetBody(req). | |
| Post(fmt.Sprintf("http://%s:%d/installBiz", baseIP, arkletPort)) | |
| if err != nil { | |
| log.G(ctx).WithError(err).Errorf("installBiz request failed") | |
| logger.Error(err, "installBiz request failed") | |
| return | |
| } | |
| if resp == nil { | |
| err = errors.New("received nil response from the server") | |
| log.G(ctx).WithError(err).Errorf("installBiz request failed") | |
| logger.Error(err, "installBiz request failed") | |
| return | |
| } | |
| if err = json.Unmarshal(resp.Body(), &response); err != nil { | |
| log.G(ctx).WithError(err).Errorf("Unmarshal InstallBiz response: %s", resp.Body()) | |
| logger.Error(err, fmt.Sprintf("Unmarshal InstallBiz response: %s", resp.Body())) | |
| return | |
| } | |
| return response, nil | |
| } | |
| func (h *Service) UninstallBiz(ctx context.Context, req UninstallBizRequest, baseIP string, arkletPort int) (response ArkResponse, err error) { | |
| logger := zaplogger.FromContext(ctx) | |
| resp, err := h.client.R(). | |
| SetContext(ctx). | |
| SetBody(req). | |
| Post(fmt.Sprintf("http://%s:%d/uninstallBiz", baseIP, arkletPort)) | |
| if err != nil { | |
| log.G(ctx).WithError(err).Errorf("uninstall request failed") | |
| logger.Error(err, "uninstall request failed") | |
| return | |
| } | |
| if resp == nil { | |
| err = errors.New("received nil response from the server") | |
| log.G(ctx).WithError(err).Errorf("uninstall request failed") | |
| logger.Error(err, "uninstall request failed") | |
| return | |
| } | |
| if err = json.Unmarshal(resp.Body(), &response); err != nil { | |
| log.G(ctx).WithError(err).Errorf("Unmarshal UnInstallBiz response: %s", resp.Body()) | |
| logger.Error(err, fmt.Sprintf("Unmarshal UnInstallBiz response: %s", resp.Body())) | |
| return | |
| } | |
| return response, nil | |
| } | |
| func (h *Service) QueryAllBiz(ctx context.Context, baseIP string, port int) (response QueryAllBizResponse, err error) { | |
| logger := zaplogger.FromContext(ctx) | |
| resp, err := h.client.R(). | |
| SetContext(ctx). | |
| SetBody(`{}`). | |
| Post(fmt.Sprintf("http://%s:%d/queryAllBiz", baseIP, port)) | |
| if err != nil { | |
| log.G(ctx).WithError(err).Errorf("queryAllBiz request failed") | |
| logger.Error(err, "queryAllBiz request failed") | |
| return | |
| } | |
| if resp == nil { | |
| err = errors.New("received nil response from the server") | |
| log.G(ctx).WithError(err).Errorf("queryAllBiz request failed") | |
| logger.Error(err, "queryAllBiz request failed") | |
| return | |
| } | |
| if err = json.Unmarshal(resp.Body(), &response); err != nil { | |
| log.G(ctx).WithError(err).Errorf("Unmarshal QueryAllBiz response: %s", resp.Body()) | |
| logger.Error(err, fmt.Sprintf("Unmarshal QueryAllBiz response: %s", resp.Body())) | |
| return | |
| } | |
| return | |
| } | |
| func (h *Service) Health(ctx context.Context, baseIP string, arkletPort int) (response HealthResponse, err error) { | |
| logger := zaplogger.FromContext(ctx) | |
| resp, err := h.client.R(). | |
| SetContext(ctx). | |
| SetBody(`{}`). | |
| Post(fmt.Sprintf("http://%s:%d/health", baseIP, arkletPort)) | |
| if err != nil { | |
| log.G(ctx).WithError(err).Errorf("health request failed") | |
| logger.Error(err, "health request failed") | |
| return | |
| } | |
| if resp == nil { | |
| err = errors.New("received nil response from the server") | |
| log.G(ctx).WithError(err).Errorf("health request failed") | |
| logger.Error(err, "health request failed") | |
| return | |
| } | |
| if err = json.Unmarshal(resp.Body(), &response); err != nil { | |
| log.G(ctx).WithError(err).Errorf("Unmarshal Health response: %s", resp.Body()) | |
| logger.Error(err, fmt.Sprintf("Unmarshal Health response: %s", resp.Body())) | |
| func (h *Service) doRequest(ctx context.Context, endpoint string, body interface{}, response interface{}) error { | |
| logger := zaplogger.FromContext(ctx) | |
| resp, err := h.client.R(). | |
| SetContext(ctx). | |
| SetBody(body). | |
| Post(endpoint) | |
| if err != nil { | |
| logger.Error(err, "request failed") | |
| return err | |
| } | |
| if resp == nil { | |
| err = errors.New("received nil response from the server") | |
| logger.Error(err, "request failed") | |
| return err | |
| } | |
| if err = json.Unmarshal(resp.Body(), response); err != nil { | |
| logger.Error(err, fmt.Sprintf("unmarshal response failed: %s", resp.Body())) | |
| return err | |
| } | |
| return nil | |
| } | |
| func (h *Service) InstallBiz(ctx context.Context, req InstallBizRequest, baseIP string, arkletPort int) (response ArkResponse, err error) { | |
| err = h.doRequest( | |
| ctx, | |
| fmt.Sprintf("http://%s:%d/installBiz", baseIP, arkletPort), | |
| req, | |
| &response, | |
| ) | |
| return response, err | |
| } | |
| func (h *Service) UninstallBiz(ctx context.Context, req UninstallBizRequest, baseIP string, arkletPort int) (response ArkResponse, err error) { | |
| err = h.doRequest( | |
| ctx, | |
| fmt.Sprintf("http://%s:%d/uninstallBiz", baseIP, arkletPort), | |
| req, | |
| &response, | |
| ) | |
| return response, err | |
| } | |
| func (h *Service) QueryAllBiz(ctx context.Context, baseIP string, port int) (response QueryAllBizResponse, err error) { | |
| err = h.doRequest( | |
| ctx, | |
| fmt.Sprintf("http://%s:%d/queryAllBiz", baseIP, port), | |
| struct{}{}, | |
| &response, | |
| ) | |
| return response, err | |
| } | |
| func (h *Service) Health(ctx context.Context, baseIP string, arkletPort int) (response HealthResponse, err error) { | |
| err = h.doRequest( | |
| ctx, | |
| fmt.Sprintf("http://%s:%d/health", baseIP, arkletPort), | |
| struct{}{}, | |
| &response, | |
| ) | |
| return response, err | |
| } |
| @@ -5,8 +5,7 @@ | |||
| "encoding/json" | |||
| "errors" | |||
| "fmt" | |||
|
|
|||
| "github.com/koupleless/virtual-kubelet/common/log" | |||
| "github.com/koupleless/module_controller/common/zaplogger" | |||
There was a problem hiding this comment.
💡 Codebase verification
Old logging dependencies found in test files
The verification reveals that while the main application code has been migrated to zaplogger, there are still old logging dependencies in test files:
- Test files (suite/mqtt/suite_test.go, suite/http/suite_test.go) still import and use
github.com/sirupsen/logrus - Test files use old logging patterns like
log.G(ctx).WithError()
These should be updated to use the new zaplogger implementation for consistency.
🔗 Analysis chain
Verify complete removal of old logging implementation
Let's ensure all old logging imports and usages have been removed.
Let's verify the actual usage context of these logging references to determine if they are legitimate dependencies or remnants of old logging.
Let's check one final thing to verify if the old logging is actually being imported and used in the test files:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining instances of old logging implementations
echo "Checking for old logging imports..."
rg -l "github.com/koupleless/virtual-kubelet/common/log|logrus|logr"
echo "Checking for old logging patterns..."
rg -l "WithError|G\(ctx\)"
Length of output: 614
Script:
#!/bin/bash
# Check the actual usage context in the found files
echo "Checking logging imports in go.mod..."
rg "logrus|logr" go.mod -A 2 -B 2
echo "Checking logging usage in test files..."
rg "WithError|G\(ctx\)" suite/mqtt/suite_test.go suite/http/suite_test.go -A 2 -B 2
echo "Checking zaplogger implementation..."
cat common/zaplogger/logger.go
echo "Checking main.go logging usage..."
rg "WithError|G\(ctx\)" cmd/module-controller/main.go -A 2 -B 2
Length of output: 4432
Script:
#!/bin/bash
# Check the imports in test files to see which logging package they're using
echo "Checking imports in test files..."
rg "^import \(" -A 10 suite/mqtt/suite_test.go suite/http/suite_test.go
echo "Checking imports in main.go..."
rg "^import \(" -A 10 cmd/module-controller/main.go
Length of output: 1994
| @@ -109,6 +109,6 @@ func InitReportServer() { | |||
| http.HandleFunc("/log", logHandler) | |||
| log.Println("Starting server on :8080") | |||
| if err := http.ListenAndServe(":8080", nil); err != nil { | |||
| logrus.Errorf("Server failed: %v", err) | |||
| zaplogger.GetLogger().Error(err, "Server failed: %v") | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error logging format
The error is being logged twice in the format string. The zaplogger's Error method already handles the error object, so including it in the format string is redundant.
- zaplogger.GetLogger().Error(err, "Server failed: %v")
+ zaplogger.GetLogger().Error(err, "Server failed")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| zaplogger.GetLogger().Error(err, "Server failed: %v") | |
| zaplogger.GetLogger().Error(err, "Server failed") |
go.mod
Outdated
| @@ -10,11 +10,12 @@ require ( | |||
| github.com/go-resty/resty/v2 v2.11.0 | |||
| github.com/google/uuid v1.6.0 | |||
| github.com/koupleless/arkctl v0.2.2 | |||
| github.com/koupleless/virtual-kubelet v0.3.5-0.20241125132309-00fba45cbde1 | |||
| github.com/koupleless/virtual-kubelet v0.3.5-0.20241126075206-2ab835c98a84 | |||
There was a problem hiding this comment.
💡 Codebase verification
Virtual-kubelet version updates need attention due to logging conflicts
The verification reveals several concerns:
- Both logrus and zap logging libraries are present in the codebase
- Logrus is still actively used in multiple files (
cmd/module-controller/main.go,suite/mqtt/suite_test.go, etc.) - The updated virtual-kubelet dependencies don't introduce any breaking changes in logging, but the current implementation maintains both logging systems
Action items:
- The logrus dependency and its imports should be removed as part of the transition to zaplogger
- Files still using logrus need to be updated to use the new zaplogger implementation
- Consider removing the direct logrus dependency from go.mod after all usages are migrated
🔗 Analysis chain
Verify the virtual-kubelet version updates
The changes include updating koupleless/virtual-kubelet and adding virtual-kubelet/virtual-kubelet. Please ensure these specific versions are compatible with the new zaplogger implementation.
Also applies to: 18-18
Let me gather more information about the logging-related dependencies to verify the compatibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for logging-related changes in the virtual-kubelet dependencies
# Check the changelog or commit diff for the koupleless/virtual-kubelet update
gh api repos/koupleless/virtual-kubelet/compare/00fba45cbde1...2ab835c98a84 --jq '.commits[].commit.message'
# Check the v1.11.0 release notes of virtual-kubelet/virtual-kubelet
gh api repos/virtual-kubelet/virtual-kubelet/releases/tags/v1.11.0 --jq '.body'
Length of output: 4794
Script:
#!/bin/bash
# Check for logging-related imports and dependencies
# Check for logrus imports in the codebase
echo "=== Checking logrus imports ==="
rg "github.com/sirupsen/logrus" -l
# Check for zap imports in the codebase
echo -e "\n=== Checking zap imports ==="
rg "go.uber.org/zap" -l
# Check go.mod for logging-related dependencies
echo -e "\n=== Checking go.mod for logging dependencies ==="
cat go.mod | grep -E "logrus|zap|zapr"
Length of output: 611
| @@ -152,23 +152,23 @@ func (c *Client) Connect() error { | |||
|
|
|||
| // Pub publishes a message to a specified topic | |||
| func (c *Client) Pub(topic string, qos byte, msg []byte) error { | |||
| logrus.Infof("Publishing message: %s to topic: %s\n", msg, topic) | |||
| zaplogger.GetLogger().Info(fmt.Sprintf("Publishing message: %s to topic: %s\n", msg, topic)) | |||
There was a problem hiding this comment.
Use safe byte slice handling in log messages
The current logging of the message payload using %s format specifier might not be safe for all byte slices, especially if they contain non-UTF8 data.
- zaplogger.GetLogger().Info(fmt.Sprintf("Publishing message: %s to topic: %s\n", msg, topic))
+ zaplogger.GetLogger().Info("Publishing message",
+ "payload_size", len(msg),
+ "topic", topic)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| zaplogger.GetLogger().Info(fmt.Sprintf("Publishing message: %s to topic: %s\n", msg, topic)) | |
| zaplogger.GetLogger().Info("Publishing message", | |
| "payload_size", len(msg), | |
| "topic", topic) |
| @@ -24,6 +23,7 @@ import ( | |||
| var _ tunnel.Tunnel = &MqttTunnel{} | |||
|
|
|||
| type MqttTunnel struct { | |||
| ctx context.Context | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid storing context.Context within struct fields
Storing context.Context inside a struct (MqttTunnel.ctx) is not recommended as it can lead to unintended consequences, especially in concurrent code. Contexts are meant to be passed explicitly to functions that need them. This ensures that cancellation and deadlines are propagated correctly.
Apply this diff to remove the ctx field and pass context explicitly:
type MqttTunnel struct {
- ctx context.Context
sync.Mutex
// other fields...
}
-func NewMqttTunnel(ctx context.Context, env string, kubeClient client.Client, moduleDeploymentController *module_deployment_controller.ModuleDeploymentController) MqttTunnel {
- return MqttTunnel{
- ctx: ctx,
+func NewMqttTunnel(env string, kubeClient client.Client, moduleDeploymentController *module_deployment_controller.ModuleDeploymentController) MqttTunnel {
+ return MqttTunnel{
env: env,
kubeClient: kubeClient,
moduleDeploymentController: moduleDeploymentController,
}
}Also applies to: 43-45
| @@ -117,20 +119,20 @@ | |||
| ClientKeyPath: c.MqttClientKeyPath, | |||
| CleanSession: true, | |||
| OnConnectHandler: func(client paho.Client) { | |||
| log.G(ctx).Info("MQTT client connected :", clientID) | |||
| zlogger.Info(fmt.Sprintf("MQTT client connected: %s", clientID)) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Leverage structured logging instead of fmt.Sprintf
The log messages use fmt.Sprintf for string formatting, which is unnecessary with structured logging. Replace these with structured fields to improve log parsing and querying.
For example, change:
zlogger.Info(fmt.Sprintf("MQTT client connected: %s", clientID))To:
zlogger.Info("MQTT client connected", zap.String("clientID", clientID))Apply similar changes to other logging statements at lines 146, 166, 197, 222, 247, and 270.
Also applies to: 146-146, 166-166, 197-197, 222-222, 247-247, 270-270
| client.Subscribe(fmt.Sprintf(model.BaseHeartBeatTopic, mqttTunnel.env), mqtt.Qos1, mqttTunnel.heartBeatMsgCallback) | ||
| client.Subscribe(fmt.Sprintf(model.BaseQueryBaselineTopic, mqttTunnel.env), mqtt.Qos1, mqttTunnel.queryBaselineMsgCallback) | ||
| }, | ||
| }) | ||
|
|
||
| err = mqttTunnel.mqttClient.Connect() | ||
| if err != nil { | ||
| log.G(ctx).WithError(err).Error("mqtt connect error") | ||
| zlogger.Error(err, "mqtt connect error") |
There was a problem hiding this comment.
Correct usage of zaplogger.Error method
The Error method should include the error as a field rather than a parameter. Adjust the logging statements to correctly utilize zap.Error.
Replace instances like:
zlogger.Error(err, "Error unmarshalling heart beat data")With:
zlogger.Error("Error unmarshalling heart beat data", zap.Error(err))Apply this correction to all error logging statements at lines 130, 151, 172, 188, 203, 228, 253, and 276.
Also applies to: 151-151, 172-172, 188-188, 203-203, 228-228, 253-253, 276-276
| zlogger := zaplogger.FromContext(mqttTunnel.ctx).WithValues("bizName", bizModel.BizName, "bizVersion", bizModel.BizVersion) | ||
| zlogger.Info("InstallModule") |
There was a problem hiding this comment.
Use With and structured fields for logger context
The method WithValues is not part of the zap logger API. Use With in combination with structured fields to add context to logs.
Replace:
zlogger := zaplogger.FromContext(mqttTunnel.ctx).WithValues("bizName", bizModel.BizName, "bizVersion", bizModel.BizVersion)With:
zlogger := zaplogger.FromContext(mqttTunnel.ctx).With(
zap.String("bizName", bizModel.BizName),
zap.String("bizVersion", bizModel.BizVersion),
)Apply similar changes to lines 324-325.
Also applies to: 324-325
| <-mqttTunnel.ctx.Done() | ||
| mqttTunnel.mqttClient.Disconnect() | ||
| }() | ||
| mqttTunnel.ready = true |
There was a problem hiding this comment.
Ensure proper context handling in goroutines
When using context.Context within goroutines, passing the context as a parameter is safer than accessing a struct field. This prevents potential data races and ensures that context cancellations are handled correctly.
Modify the goroutine to accept ctx explicitly:
go func(ctx context.Context) {
<-ctx.Done()
mqttTunnel.mqttClient.Disconnect()
-}()
+}(ctx)Committable suggestion skipped: line range outside the PR's diff.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #22 +/- ##
==========================================
- Coverage 81.29% 78.33% -2.96%
==========================================
Files 12 13 +1
Lines 1834 1939 +105
==========================================
+ Hits 1491 1519 +28
- Misses 287 351 +64
- Partials 56 69 +13 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
cmd/module-controller/main.go (4)
21-21: Clean up logging-related importsThe code imports both the new
zaploggerand the old logging libraries (logrus,logruslogger). Since we're migrating tozaplogger, consider removing the old logging imports once the migration is complete.-"github.com/sirupsen/logrus" -"github.com/virtual-kubelet/virtual-kubelet/log" -logruslogger "github.com/virtual-kubelet/virtual-kubelet/log/logrus"Also applies to: 43-44
Line range hint
159-169: Complete logger migration for health checksThe health check error logging still uses the old logging pattern (
log.G(ctx)). Update these to use the newzaploggerfor consistency.if err := mgr.AddHealthzCheck("healthz", healthz.Ping); err != nil { - log.G(ctx).Error(err, "unable to set up health check") + zlogger.Error(err, "unable to set up health check") os.Exit(1) } if err := mgr.AddReadyzCheck("readyz", healthz.Ping); err != nil { - log.G(ctx).Error(err, "unable to set up ready check") + zlogger.Error(err, "unable to set up ready check") os.Exit(1) }
Line range hint
190-193: Update HTTP tunnel error loggingThe HTTP tunnel port parsing error still uses the old logging pattern (
log.G(ctx)). Update this to use the newzaploggerfor consistency.if err != nil { - log.G(ctx).WithError(err).Error("failed to parse HTTP_TUNNEL_LISTEN_PORT, set default port 7777") + zlogger.Error(err, "failed to parse HTTP_TUNNEL_LISTEN_PORT, set default port 7777") httpTunnelListenPort = 7777 }
Line range hint
214-219: Consider more graceful error handling for tunnel startupInstead of using
panicwithfmt.Sprintf, consider:
- Using structured error types
- Logging the error with zaplogger before exiting
- Using
os.Exit(1)for a more graceful shutdownif startFailedCount > 0 { - panic(errors.New(fmt.Sprintf("failed to start %d tunnels", startFailedCount))) + zlogger.Error(fmt.Errorf("failed to start %d tunnels", startFailedCount), "tunnel startup failed") + os.Exit(1) } else if successTunnelCount == 0 { - panic(errors.New(fmt.Sprintf("successfully started 0 tunnels"))) + zlogger.Error(fmt.Errorf("successfully started 0 tunnels"), "no tunnels started") + os.Exit(1) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (16)
cmd/module-controller/main.go(8 hunks)cmd/self-test/main.go(1 hunks)common/utils/utils.go(4 hunks)common/zaplogger/logger.go(1 hunks)controller/module_deployment_controller/module_deployment_controller.go(14 hunks)go.mod(1 hunks)module_tunnels/koupleless_http_tunnel/ark_service/ark_service.go(2 hunks)module_tunnels/koupleless_http_tunnel/http_tunnel.go(22 hunks)module_tunnels/koupleless_mqtt_tunnel/mqtt/mqtt.go(3 hunks)module_tunnels/koupleless_mqtt_tunnel/mqtt_tunnel.go(15 hunks)report_server/server.go(2 hunks)suite/http/mock_http_base.go(1 hunks)suite/http/suite_test.go(2 hunks)suite/mqtt/mock_mqtt_base.go(1 hunks)suite/mqtt/module_mqtt_lifecycle_test.go(1 hunks)suite/mqtt/suite_test.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- common/zaplogger/logger.go
🚧 Files skipped from review as they are similar to previous changes (12)
- cmd/self-test/main.go
- common/utils/utils.go
- controller/module_deployment_controller/module_deployment_controller.go
- go.mod
- module_tunnels/koupleless_http_tunnel/ark_service/ark_service.go
- module_tunnels/koupleless_mqtt_tunnel/mqtt/mqtt.go
- report_server/server.go
- suite/http/mock_http_base.go
- suite/http/suite_test.go
- suite/mqtt/mock_mqtt_base.go
- suite/mqtt/module_mqtt_lifecycle_test.go
- suite/mqtt/suite_test.go
| @@ -56,7 +57,7 @@ | |||
| return utils.GetBizIdentity(container.Name, utils.GetBizVersionFromContainer(container)) | |||
| } | |||
|
|
|||
| func (mqttTunnel *MqttTunnel) RegisterNode(ctx context.Context, nodeInfo vkModel.NodeInfo) { | |||
| func (mqttTunnel *MqttTunnel) RegisterNode(nodeInfo vkModel.NodeInfo) { | |||
| nodeID := utils2.ExtractNodeIDFromNodeName(nodeInfo.Metadata.Name) | |||
| mqttTunnel.mqttClient.Sub(fmt.Sprintf(model.BaseHealthTopic, mqttTunnel.env, nodeID), mqtt.Qos1, mqttTunnel.healthMsgCallback) | |||
There was a problem hiding this comment.
Handle error return value from mqttClient.Sub
The error return value from mqttClient.Sub is not being checked. This could lead to silent failures in subscription.
Apply this diff to handle the error:
-mqttTunnel.mqttClient.Sub(fmt.Sprintf(model.BaseHealthTopic, mqttTunnel.env, nodeID), mqtt.Qos1, mqttTunnel.healthMsgCallback)
+if err := mqttTunnel.mqttClient.Sub(fmt.Sprintf(model.BaseHealthTopic, mqttTunnel.env, nodeID), mqtt.Qos1, mqttTunnel.healthMsgCallback); err != nil {
+ zlogger.Error("Failed to subscribe to health topic", zap.Error(err))
+ return
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| mqttTunnel.mqttClient.Sub(fmt.Sprintf(model.BaseHealthTopic, mqttTunnel.env, nodeID), mqtt.Qos1, mqttTunnel.healthMsgCallback) | |
| if err := mqttTunnel.mqttClient.Sub(fmt.Sprintf(model.BaseHealthTopic, mqttTunnel.env, nodeID), mqtt.Qos1, mqttTunnel.healthMsgCallback); err != nil { | |
| zlogger.Error("Failed to subscribe to health topic", zap.Error(err)) | |
| return | |
| } |
🧰 Tools
🪛 golangci-lint (1.62.2)
62-62: Error return value of mqttTunnel.mqttClient.Sub is not checked
(errcheck)
| corev1 "k8s.io/api/core/v1" | ||
| "sigs.k8s.io/controller-runtime/pkg/client" | ||
| ) | ||
|
|
||
| var _ tunnel.Tunnel = &HttpTunnel{} | ||
|
|
||
| type HttpTunnel struct { | ||
| ctx context.Context |
There was a problem hiding this comment.
Avoid storing context.Context in struct fields
It is recommended in Go to pass context.Context explicitly to each function that needs it, rather than storing it within a struct. Storing ctx in the HttpTunnel struct can lead to unintended behaviors, especially with context cancellations and timeouts.
Consider removing the ctx field from the HttpTunnel struct and passing ctx explicitly to methods that require it.
| if data.Response.Code == "SUCCESS" { | ||
| if data.Command == model.CommandInstallBiz { | ||
| // not update here, update in all biz response callback | ||
| return | ||
| } | ||
| } else { | ||
| // operation failed, log | ||
| logrus.Errorf("biz operation failed: %s\n%s\n%s", data.Response.Message, data.Response.ErrorStackTrace, data.Response.Data.Message) | ||
| logger.Error(nil, "biz operation failed: %s\n%s\n%s", data.Response.Message, data.Response.ErrorStackTrace, data.Response.Data.Message) |
There was a problem hiding this comment.
Use structured logging with logger.Error instead of formatted strings
The logger.Error method is designed for structured logging and does not support formatted strings in the message parameter. Passing a formatted string may not produce the intended log output.
Update the log statement to use key-value pairs for additional context:
-logger.Error(nil, "biz operation failed: %s\n%s\n%s", data.Response.Message, data.Response.ErrorStackTrace, data.Response.Data.Message)
+logger.Error(nil, "biz operation failed",
+ "message", data.Response.Message,
+ "errorStackTrace", data.Response.ErrorStackTrace,
+ "dataMessage", data.Response.Data.Message)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| logger.Error(nil, "biz operation failed: %s\n%s\n%s", data.Response.Message, data.Response.ErrorStackTrace, data.Response.Data.Message) | |
| logger.Error(nil, "biz operation failed", | |
| "message", data.Response.Message, | |
| "errorStackTrace", data.Response.ErrorStackTrace, | |
| "dataMessage", data.Response.Data.Message) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
common/utils/utils.go (1)
177-189: Consider adding input validation for robustnessThe function logic is correct and more straightforward. However, consider adding input validation for the records slice.
func GetLatestState(records []ark.ArkBizStateRecord) (time.Time, string, string) { + if len(records) == 0 { + return time.Time{}, "", "" + } latestStateTime := int64(0) reason := "" message := ""common/utils/utils_test.go (2)
Line range hint
354-369: Remove or uncomment the PodKey field.The commented out
PodKeyfield suggests incomplete changes. Either remove it if it's no longer needed or uncomment it if it's still required. This will improve code clarity.
Line range hint
633-635: Enhance TestOnBaseUnreachable coverage.The test for
OnBaseUnreachableis minimal and doesn't verify the actual behavior. Consider adding assertions to verify:
- Expected client calls
- Error handling
- State changes
func TestOnBaseUnreachable(t *testing.T) { - OnBaseUnreachable(context.Background(), "test-node-name", fake.NewFakeClient()) + // Setup + client := fake.NewFakeClient() + ctx := context.Background() + nodeName := "test-node-name" + + // Execute + OnBaseUnreachable(ctx, nodeName, client) + + // Verify expected behavior + // Add assertions here to verify the function's effects }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
common/model/model.go(2 hunks)common/utils/utils.go(4 hunks)common/utils/utils_test.go(10 hunks)go.mod(1 hunks)module_tunnels/koupleless_mqtt_tunnel/mqtt_tunnel.go(14 hunks)suite/http/mock_http_base.go(2 hunks)suite/mqtt/mock_mqtt_base.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- go.mod
- suite/http/mock_http_base.go
- suite/mqtt/mock_mqtt_base.go
🔇 Additional comments (14)
common/model/model.go (2)
4-4: LGTM: Import addition is appropriate
The new import for the ark package is correctly specified and necessary for the new LatestStateRecord field type.
57-60: Verify compatibility with existing code
The struct fields are well-formatted with consistent alignment and clear documentation. However, since we're adding a new field to an existing struct, we should verify:
- JSON serialization compatibility with existing clients
- All code locations that create or modify this struct are updated
Let's verify the struct usage:
✅ Verification successful
Let's search for more specific patterns to understand the usage and potential impacts of the changes.
Field addition is safe and compatible
Based on the verification results, the addition of the LatestStateRecord field is safe because:
- The struct is used in a limited scope, primarily in test code and mock implementations
- The only production usage is in
TranslateSimpleBizDataToArkBizInfowhich has been properly updated to handle the new field - No direct JSON marshaling/unmarshaling operations were found, reducing compatibility concerns
- The field is added with proper JSON tags following the existing convention
The changes maintain backward compatibility as JSON deserializers will ignore unknown fields, and the new field is properly handled in all identified usages.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for struct instantiations and field access
rg -t go "ArkSimpleBizInfoData" -A 5
# Search for JSON marshaling/unmarshaling of this struct
ast-grep --pattern 'json.Marshal($_)' | rg "ArkSimpleBizInfoData"
ast-grep --pattern 'json.Unmarshal($_)' | rg "ArkSimpleBizInfoData"
Length of output: 2630
Script:
#!/bin/bash
# Search for any code that creates or modifies ArkSimpleBizInfoData
ast-grep --pattern 'ArkSimpleBizInfoData{$$$}'
# Search for any code that uses the struct in JSON operations
rg -t go "json\.(Marshal|Unmarshal).*(ArkSimpleBizInfoData|ArkSimpleAllBizInfoData)"
# Search for any code that accesses the struct fields
rg -t go "\.(Name|Version|State|LatestStateRecord)\s*="
Length of output: 248
common/utils/utils.go (5)
6-6: LGTM: Import change aligns with PR objective
The addition of the zaplogger import is consistent with the PR's goal of transitioning to zap logger.
133-133: LGTM: Improved state retrieval logic
The change simplifies state retrieval by using the GetLatestState function while maintaining the core functionality.
169-172: LGTM: Correct field mapping implementation
The function correctly maps the simple data fields to the ArkBizInfo struct while maintaining proper field alignment.
197-208: LGTM: Well-structured logging implementation
The logging implementation follows best practices with proper context and field structuring.
218-218:
Improve error logging and default value handling
The current implementation passes nil as error and uses string formatting instead of structured logging.
-zaplogger.GetLogger().Error(nil, fmt.Sprintf("failed to parse port %s from node info", portStr))
+zaplogger.GetLogger().Error(err, "Failed to parse port from node info",
+ "port", portStr,
+ "defaultPort", 1238)
-port = 1238
+const defaultTunnelPort = 1238 // Add at top of file
+port = defaultTunnelPortmodule_tunnels/koupleless_mqtt_tunnel/mqtt_tunnel.go (4)
27-27: Avoid storing context.Context in struct fields
Storing context.Context inside a struct is not recommended as it can lead to unintended consequences in concurrent code.
123-123: Use structured logging instead of string formatting
Replace fmt.Sprintf with structured logging fields for better log parsing and querying.
Also applies to: 147-147, 167-167, 198-198, 223-223, 248-248, 271-271
131-131: Use zap.Error for error logging
The error should be included as a field rather than a parameter in the logging statements.
Also applies to: 152-152, 173-173, 189-189, 204-204, 229-229, 254-254, 277-277
340-341: Use zap.With for structured logging fields
Replace WithValues with With and appropriate zap field constructors.
Also applies to: 350-351
common/utils/utils_test.go (3)
418-440: LGTM! Well-structured test with clear assertions.
The test case properly validates the timestamp conversion and follows good testing practices with explicit expected values.
Line range hint 458-502: LGTM! Good coverage of edge cases.
The test cases properly handle edge scenarios for timestamps:
- Zero timestamps
- Negative timestamps
- No matching records
Line range hint 600-623: LGTM! Improved test readability with testify/assert.
The switch to using testify/assert library makes the test assertions more readable and provides better error messages.
| if data.Data.Command == model.CommandInstallBiz { | ||
| if data.Data.Response.Code == "SUCCESS" { | ||
| mqttTunnel.onOneBizDataArrived(nodeName, vkModel.BizStatusData{ | ||
| Key: utils.GetBizIdentity(data.Data.BizName, data.Data.BizVersion), | ||
| Name: data.Data.BizName, | ||
| State: string(vkModel.BizStateActivated), | ||
| ChangeTime: time.UnixMilli(data.PublishTimestamp), | ||
| Reason: fmt.Sprintf("%s:%s %s succeed", data.Data.BizName, data.Data.BizVersion, data.Data.Command), | ||
| Message: data.Data.Response.Data.Message, | ||
| }) | ||
| } else { | ||
| mqttTunnel.onOneBizDataArrived(nodeName, vkModel.BizStatusData{ | ||
| Key: utils.GetBizIdentity(data.Data.BizName, data.Data.BizVersion), | ||
| Name: data.Data.BizName, | ||
| State: string(vkModel.BizStateBroken), | ||
| ChangeTime: time.UnixMilli(data.PublishTimestamp), | ||
| Reason: fmt.Sprintf("%s:%s %s failed", data.Data.BizName, data.Data.BizVersion, data.Data.Command), | ||
| Message: data.Data.Response.Data.Message, | ||
| }) | ||
| } | ||
| } else if data.Data.Command == model.CommandUnInstallBiz { | ||
| if data.Data.Response.Code == "SUCCESS" { | ||
| mqttTunnel.onOneBizDataArrived(nodeName, vkModel.BizStatusData{ | ||
| Key: utils.GetBizIdentity(data.Data.BizName, data.Data.BizVersion), | ||
| Name: data.Data.BizName, | ||
| State: string(vkModel.BizStateStopped), | ||
| ChangeTime: time.UnixMilli(data.PublishTimestamp), | ||
| Reason: fmt.Sprintf("%s:%s %s succeed", data.Data.BizName, data.Data.BizVersion, data.Data.Command), | ||
| Message: data.Data.Response.Data.Message, | ||
| }) | ||
| } else { | ||
| mqttTunnel.onOneBizDataArrived(nodeName, vkModel.BizStatusData{ | ||
| Key: utils.GetBizIdentity(data.Data.BizName, data.Data.BizVersion), | ||
| Name: data.Data.BizName, | ||
| State: string(vkModel.BizStateBroken), | ||
| ChangeTime: time.UnixMilli(data.PublishTimestamp), | ||
| Reason: fmt.Sprintf("%s:%s %s failed", data.Data.BizName, data.Data.BizVersion, data.Data.Command), | ||
| Message: data.Data.Response.Data.Message, | ||
| }) | ||
| } | ||
| } else { | ||
| unsupported := fmt.Sprintf("unsupport command: %s", data.Data.Command) | ||
| zlogger.Error(errors.New(unsupported), unsupported) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor command handling to reduce duplication
The command handling logic contains significant duplication in status creation. Consider:
- Creating a helper function to construct BizStatusData
- Using constants for state values instead of string literals
- Implementing a command pattern to handle different operations
Example refactor:
+func createBizStatusData(data *model.BizOperationResponse, timestamp int64, state vkModel.BizState) vkModel.BizStatusData {
+ return vkModel.BizStatusData{
+ Key: utils.GetBizIdentity(data.BizName, data.BizVersion),
+ Name: data.BizName,
+ State: string(state),
+ ChangeTime: time.UnixMilli(timestamp),
+ Reason: fmt.Sprintf("%s:%s %s %s", data.BizName, data.BizVersion, data.Command,
+ map[bool]string{true: "succeed", false: "failed"}[data.Response.Code == "SUCCESS"]),
+ Message: data.Response.Data.Message,
+ }
+}
-if data.Data.Command == model.CommandInstallBiz {
- if data.Data.Response.Code == "SUCCESS" {
- mqttTunnel.onOneBizDataArrived(nodeName, vkModel.BizStatusData{
- Key: utils.GetBizIdentity(data.Data.BizName, data.Data.BizVersion),
- // ... repeated code
- })
- } else {
- // ... similar repeated code
- }
-} else if data.Data.Command == model.CommandUnInstallBiz {
- // ... similar repeated code
-}
+state := map[string]map[bool]vkModel.BizState{
+ model.CommandInstallBiz: {true: vkModel.BizStateActivated, false: vkModel.BizStateBroken},
+ model.CommandUnInstallBiz: {true: vkModel.BizStateStopped, false: vkModel.BizStateBroken},
+}[data.Data.Command]
+
+if state == nil {
+ zlogger.Error("Unsupported command", zap.String("command", data.Data.Command))
+ return
+}
+
+mqttTunnel.onOneBizDataArrived(nodeName,
+ createBizStatusData(&data.Data, data.PublishTimestamp,
+ state[data.Data.Response.Code == "SUCCESS"]))Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (8)
module_tunnels/koupleless_mqtt_tunnel/mqtt_tunnel.go (2)
Line range hint
61-71: Add error handling for MQTT subscriptionsThe subscription calls to
mqttClient.Subdon't check for errors, which could lead to silent failures.Add error handling for each subscription call. For example:
-mqttTunnel.mqttClient.Sub(fmt.Sprintf(model.BaseHealthTopic, mqttTunnel.env, nodeID), mqtt.Qos1, mqttTunnel.healthMsgCallback) +if err := mqttTunnel.mqttClient.Sub(fmt.Sprintf(model.BaseHealthTopic, mqttTunnel.env, nodeID), mqtt.Qos1, mqttTunnel.healthMsgCallback); err != nil { + zlogger.Error("Failed to subscribe to health topic", zap.Error(err)) + return +}🧰 Tools
🪛 golangci-lint (1.62.2)
63-63: Error return value of
mqttTunnel.mqttClient.Subis not checked(errcheck)
Line range hint
180-191: Add error handling for JSON marshalingThe error from
json.Marshalis ignored, which could lead to silent failures.Add error handling for the marshal operation:
- baselineBytes, _ := json.Marshal(baselineBizs) + baselineBytes, err := json.Marshal(baselineBizs) + if err != nil { + zlogger.Error("Failed to marshal baseline bizs", zap.Error(err)) + return + }common/utils/utils_test.go (6)
Line range hint
354-398: Consider using more realistic timestamp values in test data.The hardcoded timestamp
1234is not representative of real-world Unix millisecond timestamps. Consider using timestamps that reflect actual dates (e.g.,1699564800000for "2023-11-10") to make the tests more realistic and maintainable.- ChangeTime: 1234, + ChangeTime: 1699564800000, // 2023-11-10
Line range hint
418-450: Enhance test readability with named constants and comments.Consider using named constants for timestamps and adding comments to clarify the test scenario. This would make the test more maintainable and self-documenting.
+ // Define timestamps for better readability + const ( + oldestTime = 1234 // Oldest record + middleTime = 12345 // Middle record + newestTime = 123456 // Most recent record + ) + records := []ark.ArkBizStateRecord{ { State: "state1", - ChangeTime: 1234, + ChangeTime: oldestTime, // First state change Reason: "Reason1", Message: "Message1", }, { State: "state2", - ChangeTime: 12345, + ChangeTime: middleTime, // Second state change Reason: "Reason2", Message: "Message2", }, { State: "state1", - ChangeTime: 123456, + ChangeTime: newestTime, // Most recent state change Reason: "Reason3", Message: "Message3", }, }
Line range hint
458-477: Replace negative timestamp with a valid Unix millisecond value.Using a negative timestamp (
-1234) is not realistic for Unix timestamps. Consider using a valid timestamp value, even for error cases.- ChangeTime: -1234, + ChangeTime: 0, // Use 0 or another valid timestamp for error cases
Line range hint
486-513: Add test documentation to clarify the purpose.The test name and implementation could be more explicit about what constitutes an "invalid" change time and what behavior is being verified.
+// TestGetLatestStateInvalidChangeTime verifies that when encountering a record +// with a zero timestamp (invalid), the function correctly selects the record +// with the next valid timestamp. func TestGetLatestStateInvalidChangeTime(t *testing.T) {
600-601: Improve assertion error message.The error message in the assertion doesn't clearly indicate what's being compared. Consider making it more specific to the length comparison.
- assert.Equal(t, len(result), len(tc.expected), fmt.Errorf("ConvertBaseMetadataToBaselineQuery(%v) = %v; want %v", tc.input, result, tc.expected)) + assert.Equal(t, len(result), len(tc.expected), fmt.Errorf("Length mismatch: got %d items, want %d items", len(result), len(tc.expected)))
Line range hint
614-622: Rename test to follow Go naming conventions.The test name uses underscores which is not idiomatic Go. Consider using a more descriptive camelCase name.
-func TestGetLatestState_ChangeTimeLt3(t *testing.T) { +func TestGetLatestStateWithSingleRecord(t *testing.T) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
common/model/model.go(2 hunks)common/utils/utils.go(4 hunks)common/utils/utils_test.go(10 hunks)go.mod(1 hunks)module_tunnels/koupleless_mqtt_tunnel/mqtt_tunnel.go(14 hunks)suite/http/mock_http_base.go(2 hunks)suite/mqtt/mock_mqtt_base.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- common/model/model.go
- go.mod
- suite/http/mock_http_base.go
- suite/mqtt/mock_mqtt_base.go
🧰 Additional context used
🪛 GitHub Check: codecov/patch
common/utils/utils.go
[warning] 203-203: common/utils/utils.go#L203
Added line #L203 was not covered by tests
[warning] 208-208: common/utils/utils.go#L208
Added line #L208 was not covered by tests
🔇 Additional comments (7)
module_tunnels/koupleless_mqtt_tunnel/mqtt_tunnel.go (6)
27-27: Storing context in struct field is not recommended
The previous review comment about avoiding context storage in struct fields is still valid. Contexts should be passed explicitly to functions that need them.
123-123: Use structured logging instead of string formatting
The previous review comment about using structured logging instead of fmt.Sprintf is still valid. Replace string formatting with structured fields for better log parsing and querying.
Also applies to: 147-147, 167-167, 198-198, 223-223, 248-248, 271-271
131-131: Correct usage of zaplogger.Error method
The previous review comment about correct usage of zaplogger.Error is still valid. The error should be included as a field rather than a parameter.
Also applies to: 152-152, 173-173, 189-189, 204-204, 229-229, 254-254, 277-277
282-324: Refactor command handling to reduce duplication
The previous review comment about refactoring command handling logic is still valid. The code contains significant duplication in status creation and could benefit from helper functions and constants.
340-341: Use With and structured fields for logger context
The previous review comment about using With instead of WithValues is still valid. Use structured fields to add context to logs.
Also applies to: 350-351
136-139:
Ensure proper context handling in goroutine
The goroutine uses the stored context from the struct, which could lead to issues with cancellation propagation.
Pass the context explicitly to the goroutine:
-go func() {
- <-mqttTunnel.ctx.Done()
+go func(ctx context.Context) {
+ <-ctx.Done()
mqttTunnel.mqttClient.Disconnect()
-}()
+}(ctx)Likely invalid or redundant comment.
common/utils/utils.go (1)
218-218:
[Duplicate] Improve error logging and default port handling
This issue has been previously flagged and remains unresolved:
- Do not pass
nilas the error parameter inlogger.Error; pass the actual errorerr. - Use structured logging instead of string formatting for better log parsing and consistency.
- Consider defining and using a constant for the default port value.
Apply the following diff to address these issues:
- zaplogger.GetLogger().Error(nil, fmt.Sprintf("failed to parse port %s from node info", portStr))
+ zaplogger.GetLogger().Error(err, "Failed to parse port from node info",
+ "port", portStr,
+ "defaultPort", defaultTunnelPort)Define a constant for the default port at the top of the file:
+const defaultTunnelPort = 1238And update the assignment:
- port = 1238
+ port = defaultTunnelPort| func NewMqttTunnel(ctx context.Context, env string, kubeClient client.Client, moduleDeploymentController *module_deployment_controller.ModuleDeploymentController) MqttTunnel { | ||
| return MqttTunnel{ | ||
| ctx: ctx, |
There was a problem hiding this comment.
Constructor initializes stored context
This is related to the context storage issue. The constructor now accepts and stores a context, which could lead to issues with context cancellation and deadlines.
Consider refactoring to pass context explicitly to methods that need it, rather than storing it in the struct.
| logger.Error(deleteErr, "delete base node failed") | ||
| } | ||
| } else if apiErrors.IsNotFound(err) { | ||
| logger.Info("Node not found, skipping delete operation") | ||
| } else { | ||
| logger.WithError(err).Error("Failed to get node, cannot delete") | ||
| logger.Error(err, "Failed to get node, cannot delete") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Increase test coverage for error handling in OnBaseUnreachable
The error handling paths at lines 203 and 208 are not covered by tests, as indicated by static analysis tools. Ensuring these paths are tested is important for the reliability of your system.
Consider adding unit tests to cover these scenarios:
- When deleting the node fails (
deleteErris notniland not aNotFounderror). - When fetching the node fails with an error different from
NotFound.
Would you like assistance in generating the unit tests or opening a GitHub issue to track this task?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 203-203: common/utils/utils.go#L203
Added line #L203 was not covered by tests
[warning] 208-208: common/utils/utils.go#L208
Added line #L208 was not covered by tests
| func GetLatestState(records []ark.ArkBizStateRecord) (time.Time, string, string) { | ||
| latestStateTime := int64(0) | ||
| reason := "" | ||
| message := "" | ||
| for _, record := range records { | ||
| if record.State != state { | ||
| continue | ||
| } | ||
| if len(record.ChangeTime) < 3 { | ||
| continue | ||
| } | ||
| changeTime, err := time.Parse("2006-01-02 15:04:05", record.ChangeTime[:len(record.ChangeTime)-4]) | ||
| if err != nil { | ||
| logrus.Errorf("failed to parse change time %s", record.ChangeTime) | ||
| continue | ||
| } | ||
| if changeTime.UnixMilli() > latestStateTime.UnixMilli() { | ||
| changeTime := record.ChangeTime | ||
| if changeTime > latestStateTime { | ||
| latestStateTime = changeTime | ||
| reason = record.Reason | ||
| message = record.Message | ||
| } | ||
| } | ||
| return latestStateTime, reason, message | ||
| return time.UnixMilli(latestStateTime), reason, message |
There was a problem hiding this comment.
Handle empty records in GetLatestState function
If the records slice is empty, the function returns time.UnixMilli(0), which corresponds to the Unix epoch (1970-01-01). This might not be the intended behavior and could lead to incorrect timestamps elsewhere in your application.
Consider adding a check to handle empty records appropriately:
func GetLatestState(records []ark.ArkBizStateRecord) (time.Time, string, string) {
+ if len(records) == 0 {
+ return time.Time{}, "", ""
+ }
latestStateTime := int64(0)
reason := ""
message := ""
for _, record := range records {
changeTime := record.ChangeTime
if changeTime > latestStateTime {
latestStateTime = changeTime
reason = record.Reason
message = record.Message
}
}
return time.UnixMilli(latestStateTime), reason, message
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func GetLatestState(records []ark.ArkBizStateRecord) (time.Time, string, string) { | |
| latestStateTime := int64(0) | |
| reason := "" | |
| message := "" | |
| for _, record := range records { | |
| if record.State != state { | |
| continue | |
| } | |
| if len(record.ChangeTime) < 3 { | |
| continue | |
| } | |
| changeTime, err := time.Parse("2006-01-02 15:04:05", record.ChangeTime[:len(record.ChangeTime)-4]) | |
| if err != nil { | |
| logrus.Errorf("failed to parse change time %s", record.ChangeTime) | |
| continue | |
| } | |
| if changeTime.UnixMilli() > latestStateTime.UnixMilli() { | |
| changeTime := record.ChangeTime | |
| if changeTime > latestStateTime { | |
| latestStateTime = changeTime | |
| reason = record.Reason | |
| message = record.Message | |
| } | |
| } | |
| return latestStateTime, reason, message | |
| return time.UnixMilli(latestStateTime), reason, message | |
| func GetLatestState(records []ark.ArkBizStateRecord) (time.Time, string, string) { | |
| if len(records) == 0 { | |
| return time.Time{}, "", "" | |
| } | |
| latestStateTime := int64(0) | |
| reason := "" | |
| message := "" | |
| for _, record := range records { | |
| changeTime := record.ChangeTime | |
| if changeTime > latestStateTime { | |
| latestStateTime = changeTime | |
| reason = record.Reason | |
| message = record.Message | |
| } | |
| } | |
| return time.UnixMilli(latestStateTime), reason, message | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
cmd/module-controller/main.go (3)
89-89: Enhance error messages for better debuggingThe error messages could be more descriptive by including the actual values that failed parsing.
-zlogger.Error(err, "failed to parse WORKLOAD_MAX_LEVEL, will be set to 3 default") +zlogger.Error(err, "failed to parse WORKLOAD_MAX_LEVEL", "raw_value", utils.GetEnv("WORKLOAD_MAX_LEVEL", "3"), "default_value", 3) -zlogger.Error(err, "failed to parse VNODE_WORKER_NUM, will be set to 8 default") +zlogger.Error(err, "failed to parse VNODE_WORKER_NUM", "raw_value", utils.GetEnv("VNODE_WORKER_NUM", "8"), "default_value", 8)Also applies to: 95-95
Line range hint
170-172: Fix inconsistent logging patternThis error handling still uses the old logging pattern while the rest of the file uses zaplogger.
- log.G(ctx).WithError(err).Error("failed to start manager") + zlogger.Error(err, "failed to start manager")
Line range hint
190-193: Fix inconsistent error handling in HTTP tunnel setupThe HTTP tunnel port parsing error is still using the old logging pattern.
- log.G(ctx).WithError(err).Error("failed to parse HTTP_TUNNEL_LISTEN_PORT, set default port 7777") + zlogger.Error(err, "failed to parse HTTP_TUNNEL_LISTEN_PORT", "default_port", 7777)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
cmd/module-controller/main.go(8 hunks)go.mod(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
🔇 Additional comments (1)
cmd/module-controller/main.go (1)
Line range hint 77-82: Remove redundant logger initialization
The code initializes both logrus and zaplogger, which creates unnecessary overhead and potential confusion.
Since we're migrating to zaplogger, the logrus initialization at line 77 should be removed:
-log.L = logruslogger.FromLogrus(logrus.NewEntry(logrus.StandardLogger()))| zlogger := zaplogger.GetLogger() | ||
| ctx = zaplogger.WithLogger(ctx, zlogger) |
There was a problem hiding this comment.
💡 Codebase verification
Update logger usage to use consistent zap logger pattern
The verification reveals inconsistent logger usage across the codebase:
- Several files still use the old
log.G(ctx)pattern, particularly in test files and main.go - The zap logger is properly initialized and propagated in most production code
- Context propagation with
zaplogger.FromContext(ctx)is consistently used in controllers and tunnels
Action items:
- Update remaining
log.G(ctx)instances in:suite/http/suite_test.gosuite/mqtt/suite_test.gosuite/http/mock_http_base.gosuite/mqtt/mock_mqtt_base.gosuite/mqtt/module_mqtt_lifecycle_test.gocmd/self-test/main.gocmd/module-controller/main.go
🔗 Analysis chain
Verify logger configuration and performance settings
While the logger initialization and context propagation look correct, we should verify the logger's configuration and performance settings.
Also applies to: 105-106
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify logger configuration and usage across the codebase
# Check for any remaining old logger patterns
echo "Checking for remaining old logger patterns..."
rg "log\.G\(" --type go
# Check for consistent zap logger initialization
echo "Checking for consistent zap logger initialization..."
rg "zaplogger\.GetLogger\(\)" --type go
# Check for proper context propagation
echo "Checking for proper context propagation..."
rg "zaplogger\.FromContext\(ctx\)" --type go
Length of output: 4303
old logger format
new logger format
Summary by CodeRabbit
Release Notes
New Features
zaplogger) for improved logging consistency across various modules.HttpTunnelandMqttTunnelimplementations.Bug Fixes
Documentation
Refactor
LatestStateRecordto theArkSimpleBizInfoDatastruct for storing the latest state record.These updates enhance the overall logging mechanisms and improve the clarity of operations within the application.