Conversation
Pyr33x
added
enhancement
|
Warning Rate limit exceeded@Pyr33x has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 21 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughAdds a config package with loader/defaults/validation, refactors internal MQTT client to accept *config.Config and return enriched errors, introduces a builder-style Bench API with concurrent connection runner, adds a slog-based logger, extends the error model, adds a conn CLI command, and adds a golangci-lint workflow. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI / Cobra
participant Config as pkg/config
participant Logger as pkg/logger
participant Bench as internal/bench
participant MQTT as internal/mqtt
participant ER as pkg/er
Note over CLI,Config: Startup — load and validate config
CLI->>Config: InitializeCfg()
alt config ok
Config-->>CLI: *Config (defaults applied)*
CLI->>Logger: InitGlobalLogger(preset based on Env)
CLI->>Bench: NewBenchmark(cfg *Config, opts...)
Bench-->>CLI: *Bench*
CLI->>Bench: RunConnections()
par per client (concurrent)
Bench->>MQTT: mqtt.NewClient(cfg *Config)
MQTT->>MQTT: build options from cfg.Server / cfg.Client
MQTT->>MQTT: Connect()
alt connect success
MQTT-->>Bench: *Adapter*
Bench->>Logger: LogClientConnection(...)
else connect fail
MQTT->>ER: return er.Error{Package, Func, Message, Raw: token.Error()}
ER-->>Bench: er.Error
Bench->>Logger: Log error
end
end
Bench-->>CLI: all goroutines complete (elapsed)
else config fail
Config-->>CLI: er.Error (validation/read/unmarshal)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pkg/config/config.go (1)
32-46: Do not swallow config errors; make path configurable and validateCurrently logs and returns zero-value config, which leads to confusing downstream failures.
Option A (non-breaking, better logging + env override + inline validation):
-func InitializeCfg() *Config { - rawCfg, err := os.ReadFile("config.yml") - if err != nil { - log.Println("Failed to read the configuration file") - } - - var cfg Config - err = yaml.Unmarshal(rawCfg, &cfg) - if err != nil { - log.Println("Failed to unmarshal the yaml configuration file") - } - - return &cfg -} +func InitializeCfg() *Config { + path := os.Getenv("BENCHMQ_CONFIG") + if path == "" { + path = "config.yml" + } + rawCfg, err := os.ReadFile(path) + if err != nil { + log.Printf("Failed to read configuration file %q: %v", path, err) + return &Config{} // keep current signature; avoid nil + } + + var cfg Config + if err := yaml.Unmarshal(rawCfg, &cfg); err != nil { + log.Printf("Failed to unmarshal YAML config %q: %v", path, err) + return &Config{} + } + + // Minimal validation + if cfg.Server.Host == "" || cfg.Server.Port == 0 { + log.Printf("Invalid config %q: server.host and server.port must be set", path) + } + return &cfg +}Option B (preferred, breaking: return errors and validate):
-func InitializeCfg() *Config { - ... - return &cfg -} +func InitializeCfg(path string) (*Config, error) { + if path == "" { + if env := os.Getenv("BENCHMQ_CONFIG"); env != "" { + path = env + } else { + path = "config.yml" + } + } + rawCfg, err := os.ReadFile(path) + if err != nil { + return nil, fmt.Errorf("read config %q: %w", path, err) + } + var cfg Config + if err := yaml.Unmarshal(rawCfg, &cfg); err != nil { + return nil, fmt.Errorf("unmarshal YAML %q: %w", path, err) + } + if cfg.Server.Host == "" || cfg.Server.Port == 0 { + return nil, fmt.Errorf("invalid config %q: server.host and server.port must be set", path) + } + return &cfg, nil +}If you choose Option B, add:
import "fmt"to the imports.
internal/mqtt/mqtt.go (2)
22-22: Broker URL formatting breaks for IPv6; use net.JoinHostPortfmt with "%s:%d" yields invalid URLs for IPv6 literals. Use net.JoinHostPort to bracket IPv6 correctly.
Apply:
-opts.AddBroker(fmt.Sprintf("tcp://%s:%d", cfg.Server.Host, cfg.Server.Port)) +hostPort := net.JoinHostPort(cfg.Server.Host, strconv.Itoa(int(cfg.Server.Port))) +opts.AddBroker("tcp://" + hostPort)Add imports:
import ( "fmt" "sync" "time" "net" "strconv" mq "github.com/eclipse/paho.mqtt.golang" "github.com/pyr33x/benchmq/pkg/config" "github.com/pyr33x/benchmq/pkg/er" )
37-45: Surface the underlying connection error for debuggabilityCurrently the root cause is dropped.
- if token := a.client.Connect(); token.Wait() && token.Error() != nil { - return &er.Error{ - Context: "MQTT Adapter", - Message: er.ErrMqttConnectionFailed, - } - } + if token := a.client.Connect(); token.Wait() && token.Error() != nil { + return &er.Error{ + Context: "MQTT Adapter", + Message: fmt.Sprintf("%s: %v", er.ErrMqttConnectionFailed, token.Error()), + } + }
🧹 Nitpick comments (4)
pkg/config/config.go (1)
24-30: Consider extending client config (timeout/TLS) for reliability and benchmarksOptional fields that help a “connection benchmark”: connect timeout, protocol version, TLS toggle/params.
Example additions (YAML keys):
- connect_timeout_secs: int
- protocol_version: int (3=3.1, 4=3.1.1, 5=5.0)
- tls_enabled: bool; tls_insecure_skip_verify: bool; tls_ca_file: string
internal/mqtt/mqtt.go (3)
22-29: Add a connect timeout to avoid indefinite waitsBenchmarks shouldn’t hang indefinitely on unreachable brokers.
Minimal addition:
opts := mq.NewClientOptions() ... opts.SetPassword(cfg.Client.Password) +opts.SetConnectTimeout(10 * time.Second) // consider making configurable opts.SetProtocolVersion(4) // Default set to MQTT 3.1.1
13-16: Drop or properly use the WaitGroup
WaitGroup’s Wait is called in Disconnect (internal/mqtt/mqtt.go:49), but no Add or Done calls exist, so it never actually waits. Either removewgand itsWait()call, or callwg.Add(…)before spawning goroutines andwg.Done()when they finish.
18-18: Passing cfg config.Config by value is acceptable; pointer refactor is optional
The Config struct (nested server and client fields) is small, so copying is cheap and there are no existing callers to update the signature.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/mqtt/mqtt.go(2 hunks)pkg/config/config.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/mqtt/mqtt.go (1)
pkg/config/config.go (1)
Config(11-15)
🔇 Additional comments (3)
pkg/config/config.go (2)
1-1: Package move to pkg/config looks goodShrinks public surface and clarifies layering.
13-14: No downstream references to config.Server or config.Client detected – unexported types change is safe.internal/mqtt/mqtt.go (1)
9-9: Import path update to pkg/config: LGTM
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
pkg/config/config.go (1)
61-78: Validation looks good; trim host to avoid whitespace edge casesThis satisfies the earlier “minimal host/port validation” ask. As a small robustness boost, trim whitespace before checks/defaults.
Apply:
func (c *Config) Validate() error { - if c.Server.Host == "" { + if strings.TrimSpace(c.Server.Host) == "" { return &er.Error{ Package: "Config", Func: "Validate", Message: er.ErrEmptyServerHost, } } if c.Server.Port == 0 { return &er.Error{ Package: "Config", Func: "Validate", Message: er.ErrInvalidServerPort, } } return nil }func (c *Config) SetDefaults() { - if c.Server.Host == "" { + c.Server.Host = strings.TrimSpace(c.Server.Host) + if c.Server.Host == "" { c.Server.Host = "localhost" } if c.Server.Port == 0 { c.Server.Port = 1883 } if c.Client.KeepAlive == 0 { c.Client.KeepAlive = 60 } }Also applies to: 80-91
🧹 Nitpick comments (10)
.github/workflows/lint.yml (2)
22-26: Enable module cache to speed up runs.Go module caching cuts CI time and network churn.
Apply:
- name: "Setup Go" uses: actions/setup-go@v5 with: go-version: ${{ matrix.go }} + cache: true
16-16: Reduce OS matrix for a lint-only job.Linting is OS-agnostic; running on macOS adds cost/time with little benefit. Consider Ubuntu only.
- os: [ubuntu-latest, macos-latest] + os: [ubuntu-latest]pkg/er/er.go (2)
16-20: Add small constructor helpers to reduce repetition.Creating
&er.Error{Package: "...", Func: "...", Message: ...}everywhere is noisy. ProvideNew/Wraphelpers.Add these to this file:
// New constructs a contextual error. func New(pkg, fn string, msg error) error { return &Error{Package: pkg, Func: fn, Message: msg} } // Wrap wraps a cause with a sentinel while preserving Is/Unwrap chains. func Wrap(pkg, fn string, sentinel, cause error) error { return &Error{Package: pkg, Func: fn, Message: fmt.Errorf("%w: %w", sentinel, cause)} }
22-24: Consider a log-friendly format.Minor:
package=mqtt func=Connect: mqtt connection failedreads tighter in logs than comma-separated labels.-return fmt.Sprintf("package: %s, func: %s, error: %v", e.Package, e.Func, e.Message) +return fmt.Sprintf("package=%s func=%s: %v", e.Package, e.Func, e.Message)internal/mqtt/mqtt.go (2)
22-29: Set connect timeout and enable auto-reconnect.Avoid indefinite waits and improve resiliency by configuring timeouts and reconnect.
opts.SetClientID(cfg.Client.ClientID) opts.SetKeepAlive(time.Duration(cfg.Client.KeepAlive) * time.Second) + // Ensure Connect respects a finite timeout (tweak or make configurable). + opts.SetConnectTimeout(10 * time.Second) + // Automatically try to re-establish the connection. + opts.SetAutoReconnect(true) opts.SetCleanSession(cfg.Client.CleanSession) opts.SetUsername(cfg.Client.Username) opts.SetPassword(cfg.Client.Password) opts.SetProtocolVersion(4) // Default set to MQTT 3.1.1
48-51: Make quiesce period configurable (or slightly higher).200ms may be too short to flush in-flight packets under load; consider 500–1000ms or a config knob.
-func (a *Adapter) Disconnect() { - a.client.Disconnect(200) +func (a *Adapter) Disconnect() { + // Allow a bit more time or read from config/env if available. + a.client.Disconnect(500) a.wg.Wait() }pkg/config/config.go (4)
10-15: Unexported nested types limit programmatic construction outside the packageUsing unexported
server/clienttypes prevents external packages/tests from constructingconfig.Configliterals. Consider exporting these types or providing a small constructor (e.g.,New()orNewWith(server, client)) to ease testing and reuse.
34-41: Support configurable path and .yaml fallbackAllow overriding the config path via env and fall back to common filenames. This improves DX and CI portability.
Apply:
- rawCfg, err := os.ReadFile("config.yml") + path := os.Getenv("BENCHMQ_CONFIG") + if path == "" { + if _, statErr := os.Stat("config.yml"); statErr == nil { + path = "config.yml" + } else { + path = "config.yaml" + } + } + rawCfg, err := os.ReadFile(path) if err != nil { return nil, &er.Error{ Package: "Config", Func: "InitializeCfg", Message: er.ErrConfigReadFailed, } }
32-41: Preserve root cause in returned errorsCurrent returns drop the underlying
err. Ifer.Errorsupports wrapping/Unwrap, include the cause so callers canerrors.Is/Asand for better logs.Would you like me to adapt this to the actual
er.ErrorAPI (e.g., adding aCausefield or using a helper likeer.Wrapif present)?Also applies to: 43-51
24-30: Secrets in config: consider env overrides and redactionUsernames/passwords in YAML are convenient but risky. Consider: env var overrides for creds, optional .env loading, and ensuring any config logging redacts
Password.If desired, I can add a small helper to apply env overrides (e.g.,
BENCHMQ_USERNAME,BENCHMQ_PASSWORD) with redaction onString().
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/lint.yml(1 hunks)internal/mqtt/mqtt.go(3 hunks)pkg/config/config.go(1 hunks)pkg/er/er.go(1 hunks)
🔇 Additional comments (5)
.github/workflows/lint.yml (2)
7-10: Permissions look good.Least-privilege
contents: readandpull-requests: readare appropriate here.
28-28: Confirm golangci-lint-action@v8 is valid
golangci/golangci-lint-action@v8corresponds to the current v8.0.0 release (May 4 2025). No change needed. (github.com)pkg/er/er.go (1)
10-13: Good addition of specific sentinel errors.Clear, focused error vars improve
errors.Ischecks and messages.internal/mqtt/mqtt.go (1)
18-18: Signature change: verify all call sites.Ensure every
mqtt.NewClientcaller was updated to passconfig.Config.Run:
#!/bin/bash # Find mqtt.NewClient call sites and show context rg -nP -C2 '\bmqtt\.NewClient\s*\('pkg/config/config.go (1)
53-56: Good sequencing: defaults before validationSetting defaults then validating is correct and avoids false negatives on missing-but-defaulted fields.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (12)
pkg/er/er.go (4)
10-13: Add doc comments for exported errors to satisfy linters and improve API claritygolangci-lint will likely flag these exported vars for missing comments. Brief, identifier-prefixed comments also clarify scope (config vs MQTT).
var ( ErrMqttConnectionFailed = errors.New("mqtt connection failed") - ErrEmptyServerHost = errors.New("server host cannot be empty") - ErrInvalidServerPort = errors.New("server port is invalid") - ErrUnmarshalFailed = errors.New("failed to unmarshal config file") - ErrConfigReadFailed = errors.New("failed to read config file") + // ErrEmptyServerHost indicates the server/broker host field is missing in config. + ErrEmptyServerHost = errors.New("server host cannot be empty") + // ErrInvalidServerPort indicates the configured server/broker port is outside 1–65535. + ErrInvalidServerPort = errors.New("server port is invalid") + // ErrUnmarshalFailed indicates decoding the config file into a struct failed. + ErrUnmarshalFailed = errors.New("failed to unmarshal config file") + // ErrConfigReadFailed indicates reading the config file from disk failed. + ErrConfigReadFailed = errors.New("failed to read config file") )
16-21: Consider a constructor to enforce invariants and reduce duplication when building er.ErrorA helper keeps Message non-nil and makes Package/Func consistent at call sites.
// New builds an Error with optional raw cause. func New(pkg, fn string, msg, raw error) *Error { if msg == nil { msg = ErrMqttConnectionFailed // or a generic sentinel; choose what fits best } return &Error{Package: pkg, Func: fn, Message: msg, Raw: raw} }
23-25: Avoid printing “rawError: ” and handle nil Message gracefully in Error()This trims noisy logs and avoids “error: ” when fields are unset.
-func (e *Error) Error() string { - return fmt.Sprintf("package: %s, func: %s, error: %v, rawError: %v", e.Package, e.Func, e.Message, e.Raw) -} +func (e *Error) Error() string { + msg := "<nil>" + if e.Message != nil { + msg = e.Message.Error() + } + if e.Raw != nil { + return fmt.Sprintf("package: %s, func: %s, error: %s, rawError: %v", e.Package, e.Func, msg, e.Raw) + } + return fmt.Sprintf("package: %s, func: %s, error: %s", e.Package, e.Func, msg) +}
27-29: Expose both Message and Raw in the error chain (Go 1.20+) so errors.Is/As match eitherRight now Unwrap() returns only Message; callers cannot match/inspect Raw. If you’re on Go 1.20+, switch to multi-unwrap.
-func (e *Error) Unwrap() error { - return e.Message -} +// Go 1.20+: allow matching both the classification (Message) and the underlying cause (Raw). +func (e *Error) Unwrap() []error { + errs := make([]error, 0, 2) + if e.Message != nil { + errs = append(errs, e.Message) + } + if e.Raw != nil { + errs = append(errs, e.Raw) + } + return errs +}If Go < 1.20, keep Unwrap() error and add an Is method to match either:
// Fallback for older Go versions: func (e *Error) Is(target error) bool { return errors.Is(e.Message, target) || errors.Is(e.Raw, target) }Run to confirm Go version and find sites relying on errors.Is/As:
#!/bin/bash set -euo pipefail echo "Go version in go.mod:" && rg -n '^go\s+\d+\.\d+' go.mod || true echo -e "\nCall sites using errors.Is/As:" && rg -n --glob '!**/vendor/**' 'errors\.(Is|As)\(' -C2 || trueinternal/bench/bench.go (8)
71-75: Use Go naming: clientID, not clientId.Aligns with common initialism rules.
-func WithClientID(clientId string) Option { +func WithClientID(clientID string) Option { return func(b *Bench) { - b.ClientID = clientId + b.ClientID = clientID } }
26-35: Avoid defaults duplication with pkg/config; centralize to one source of truth.These defaults overlap config defaults introduced in pkg/config. Drift is likely.
- Option 1: Import pkg/config defaults here.
- Option 2: Provide NewBenchmarkFromConfig(cfg) that maps from config.Config, with minimal local defaults.
- Option 3: Embed a config.Config inside Bench for connection fields and only add bench-specific fields here.
Also applies to: 37-48
50-54: Nil-check on options is redundant.Functional options are under your control; passing nil is a misuse. Dropping the check simplifies the loop.
- for _, option := range options { - if option != nil { - option(&bench) - } - } + for _, option := range options { + option(&bench) + }
6-6: Prefer time.Duration for Delay to make units explicit.int is ambiguous and error-prone. time.Duration communicates intent and prevents unit mistakes.
-import "errors" +import ( + "errors" + "time" +) @@ - Delay int + Delay time.Duration @@ - DefaultDelay = 100 + DefaultDelay = 100 * time.MillisecondThis change will ripple; only adopt if callers are easy to update.
Also applies to: 26-26
3-3: Add GoDoc comments for exported identifiers.GolangCI (revive/exported) commonly flags missing comments. Brief one-liners unblock docs and lint.
Example:
- type QoSLevel uint8 +// QoSLevel is the MQTT Quality of Service (0, 1, or 2). +type QoSLevel uint8 @@ - type Bench struct { +// Bench holds parameters for the MQTT connection benchmark. +type Bench struct { @@ - func NewBenchmark(options ...Option) *Bench { +// NewBenchmark returns a Bench initialized with defaults, then applies options. +func NewBenchmark(options ...Option) *Bench {Replicate for constants and option helpers as needed.
Also applies to: 5-15, 17-17, 19-23, 25-35, 37-37
101-111: Port validation/clamping is missing; avoid 0 (ephemeral) or out-of-range values.While Validate() addresses this, consider an early guard in the setter to fail fast.
func WithPort(port uint16) Option { return func(b *Bench) { - b.Port = port + if port == 0 { + b.Port = DefaultPort + return + } + b.Port = port } }
25-35: Consider convenience options for common QoS and client ID generation.Minor usability wins for the CLI/bench command.
- WithQoS0/1/2 helpers:
+func WithQoS0() Option { return func(b *Bench) { b.QoS = QoS0 } } +func WithQoS1() Option { return func(b *Bench) { b.QoS = QoS1 } } +func WithQoS2() Option { return func(b *Bench) { b.QoS = QoS2 } }
- Optionally add WithRandomClientID(prefix string) using time.Now().UnixNano() or a UUID (if you already depend on one).
I can wire these up and add unit tests if you want.
89-93: Make QoS option type-safe (use QoSLevel) or add explicit range check.Taking uint16 invites invalid values; the type already exists. Either switch to QoSLevel or validate here.
Option A (type-safe):
-func WithQoS(qos uint16) Option { - return func(b *Bench) { - b.QoS = QoSLevel(qos) - } -} +func WithQoS(qos QoSLevel) Option { + return func(b *Bench) { + b.QoS = qos + } +}Option B (keep signature, validate):
func WithQoS(qos uint16) Option { return func(b *Bench) { - b.QoS = QoSLevel(qos) + if qos > uint16(QoS2) { + b.QoS = DefaultQoS + return + } + b.QoS = QoSLevel(qos) } }If you pick Option A, check/adjust call sites:
#!/bin/bash rg -n --hidden -S '\bWithQoS\(' -C2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/lint.yml(1 hunks)internal/bench/bench.go(1 hunks)internal/mqtt/mqtt.go(3 hunks)pkg/config/config.go(1 hunks)pkg/er/er.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/mqtt/mqtt.go
- .github/workflows/lint.yml
- pkg/config/config.go
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
internal/mqtt/mqtt.go (1)
38-45: Wrap the sentinel with the underlying token error so errors.Is works.Right now the root cause is only in Raw. Prefer wrapping the sentinel to keep Is/As semantics intact.
Apply:
if token := a.client.Connect(); token.Wait() && token.Error() != nil { - tErr := token.Error() - return &er.Error{ - Package: "MQTT", - Func: "Connect", - Message: er.ErrMqttConnectionFailed, - Raw: tErr, - } + tErr := token.Error() + return &er.Error{ + Package: "MQTT", + Func: "Connect", + Message: fmt.Errorf("%w: %v", er.ErrMqttConnectionFailed, tErr), + Raw: tErr, // keep for logging/telemetry + } }
🧹 Nitpick comments (12)
pkg/er/er.go (2)
33-35: Preserve the full error chain by joining Message and Raw in UnwrapReturning only Message drops the underlying Raw cause from errors.Is/As traversal.
func (e *Error) Unwrap() error { - return e.Message + if e.Raw != nil { + return errors.Join(e.Message, e.Raw) + } + return e.Message }
29-31: Avoid printing “rawError” when nil to reduce noise (and potential leakage)Keep logs cleaner and don’t print
<nil>. When Raw exists, include it; otherwise omit.func (e *Error) Error() string { - return fmt.Sprintf("package: %s, func: %s, error: %v, rawError: %v", e.Package, e.Func, e.Message, e.Raw) + if e.Raw != nil { + return fmt.Sprintf("package: %s, func: %s, error: %v, rawError: %v", e.Package, e.Func, e.Message, e.Raw) + } + return fmt.Sprintf("package: %s, func: %s, error: %v", e.Package, e.Func, e.Message) }internal/bench/conn.go (2)
27-27: Make the final log line self-describingInclude clients count and a label for readability.
- log.Println(time.Since(now)) + log.Printf("connections: %d, elapsed: %s", b.clients, time.Since(now))
11-28: Optional: support paced connection ramp-up if a delay option existsIf a per-connection delay is present in bench/config (ErrInvalidDelay exists), consider sleeping between goroutine spawns to shape load.
pkg/config/config.go (4)
37-41: Include the OS read error in Raw to aid diagnosisPopulate Raw so callers can see the underlying failure reason.
return nil, &er.Error{ Package: "Config", Func: "InitializeCfg", Message: er.ErrConfigReadFailed, + Raw: err, }
48-52: Include the YAML decode error in RawThis preserves the exact unmarshal failure for troubleshooting.
return nil, &er.Error{ Package: "Config", Func: "InitializeCfg", Message: er.ErrUnmarshalFailed, + Raw: err, }
3-9: Treat whitespace-only hostnames as emptyTrim spaces before validating to avoid accepting " ".
import ( "bytes" "os" + "strings" "github.com/pyr33x/benchmq/pkg/er" "gopkg.in/yaml.v3" )- if c.Server.Host == "" { + if strings.TrimSpace(c.Server.Host) == "" { return &er.Error{ Package: "Config", Func: "Validate", Message: er.ErrEmptyServerHost, } }Also applies to: 65-71
33-61: Optional: allow a configurable path; keep InitializeCfg for defaultThis improves usability for tests and multiple environments.
Add this helper (outside the shown ranges):
// InitializeCfgFrom reads the config from a given path. func InitializeCfgFrom(path string) (*Config, error) { rawCfg, err := os.ReadFile(path) if err != nil { return nil, &er.Error{Package: "Config", Func: "InitializeCfgFrom", Message: er.ErrConfigReadFailed, Raw: err} } var cfg Config dec := yaml.NewDecoder(bytes.NewReader(rawCfg)) dec.KnownFields(true) if err = dec.Decode(&cfg); err != nil { return nil, &er.Error{Package: "Config", Func: "InitializeCfgFrom", Message: er.ErrUnmarshalFailed, Raw: err} } cfg.SetDefaults() if err := cfg.Validate(); err != nil { return nil, err } return &cfg, nil }Then change InitializeCfg to call it:
func InitializeCfg() (*Config, error) { return InitializeCfgFrom("config.yml") }internal/mqtt/mqtt.go (1)
22-29: Optional: expose transport and connect timeout via config.
- Allow TLS by switching scheme based on cfg (e.g., ssl:// when enabled).
- Consider setting opts.SetConnectTimeout(...) from cfg to avoid hanging dials.
internal/bench/bench.go (3)
29-33: Fix minor typo in QoS comment.- QoS1 QoSLevel = 1 // QoS At Lease Once + QoS1 QoSLevel = 1 // QoS At Least Once
35-45: Clarify a comment for readability.- DefaultDelay = 100 // Default delay between connection + DefaultDelay = 100 // Default delay between connections
12-25: Optional: add GoDoc on exported types/consts for discoverability.Short comments on QoSLevel, Bench, Option, and defaults help pkg users.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
internal/bench/bench.go(1 hunks)internal/bench/conn.go(1 hunks)internal/mqtt/mqtt.go(3 hunks)pkg/config/config.go(1 hunks)pkg/er/er.go(1 hunks)
🔇 Additional comments (5)
pkg/config/config.go (1)
45-47: Nice: strict YAML decoding with KnownFieldsinternal/mqtt/mqtt.go (2)
9-11: Config import + signature change look good.Using pkg/config and a pointer receiver for NewClient aligns the module boundaries and avoids copies.
Also applies to: 18-18
50-53: Confirm WaitGroup lifecycle to avoid hangs on Disconnect.Ensure all goroutines add/done against a.wg or this may block unexpectedly.
internal/bench/bench.go (2)
75-126: Validation logic is solid and covers key fields.Good checks for clients, delay, host/topic, port, and QoS bounds.
75-126: Ensure er.Error participates in Go error chains.If er.Error doesn’t implement Unwrap to expose Message (and/or Raw), errors.Is/As on returned validation errors won’t work as intended.
Run:
#!/bin/bash # Verify er.Error supports error chaining rg -nC2 'type\s+Error\s+struct' pkg/er || true rg -nC2 'func\s+\(\*?Error\)\s+Unwrap\(\)\s+error' -g 'pkg/er/**' || true rg -nC2 'errors\.Is\(|errors\.As\('Expected:
- er.Error has Unwrap() error or equivalent.
- Call sites using errors.Is/As will then behave correctly.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (11)
pkg/logger/logger.go (4)
32-44: Remove or implement unused Config fields (ShowCaller, TimeFormat).They’re never applied. If needed, wire via ReplaceAttr and AddSource; otherwise drop to avoid confusion.
Example wiring:
opts := &slog.HandlerOptions{ Level: convertLevel(config.Level), AddSource: config.AddSource, -} + ReplaceAttr: func(groups []string, a slog.Attr) slog.Attr { + if a.Key == slog.TimeKey && config.TimeFormat != "" { + if t, ok := a.Value.Time(); ok { + return slog.String(slog.TimeKey, t.Format(config.TimeFormat)) + } + } + return a + }, +}
115-140: Default service name is inconsistent (“goqtt”).This repo is BenchMQ; consider “benchmq” or read from app config to keep metadata consistent.
- Service: "goqtt", + Service: "benchmq",(Apply to both DevelopmentConfig and ProductionConfig.)
257-260: Emit duration with an explicit unit and integer type.Current key “time” is ambiguous and casts to float. Prefer milliseconds as int and clearer key.
-func TrackTime(t time.Time) slog.Attr { - return slog.Float64("time", float64(time.Since(t).Milliseconds())) -} +func TrackTime(t time.Time) slog.Attr { + return slog.Int64("elapsed_ms", time.Since(t).Milliseconds()) +}
235-245: Use helper for client_id for consistency.Minor consistency nit: reuse ClientID() here.
- slog.String("client_id", clientID), + ClientID(clientID),pkg/config/config.go (4)
39-44: Preserve underlying read error via Raw for better diagnosticsInclude the original
errin the returneder.Error. This greatly improves observability without changing behavior.- return nil, &er.Error{ + return nil, &er.Error{ Package: "Config", Func: "InitializeCfg", Message: er.ErrConfigReadFailed, - } + Raw: err, + }
49-55: Also attach Raw on YAML decode failureSame rationale as above; return the root cause.
- if err = dec.Decode(&cfg); err != nil { - return nil, &er.Error{ + if err = dec.Decode(&cfg); err != nil { + return nil, &er.Error{ Package: "Config", Func: "InitializeCfg", Message: er.ErrUnmarshalFailed, - } + Raw: err, + }
65-82: Harden validation: trim host and reject whitespace-only valuesRejecting
" "avoids subtle misconfigurations. Optional: keep current port rule (uint16 already bounds upper limit).func (c *Config) Validate() error { - if c.Server.Host == "" { + if strings.TrimSpace(c.Server.Host) == "" { return &er.Error{ Package: "Config", Func: "Validate", Message: er.ErrEmptyServerHost, } }Additional change (imports):
import ( "bytes" "os" + "strings"
84-101: Consider explicit defaulting for CleanSession (bools can’t distinguish “unset”)If you intend CleanSession to default to true when omitted in YAML (to mirror Bench defaults), switch to a pointer and default only when nil. Otherwise, configs that omit the field will silently default to false.
Example:
type Client struct { ClientID string `yaml:"client_id"` KeepAlive uint16 `yaml:"keep_alive"` - CleanSession bool `yaml:"clean_session"` + CleanSession *bool `yaml:"clean_session"` Username string `yaml:"username"` Password string `yaml:"password"` } func (c *Config) SetDefaults() { + if c.Client.CleanSession == nil { + def := true + c.Client.CleanSession = &def + }internal/bench/bench.go (3)
31-35: Fix typo in QoS comment (“At Lease Once” → “At Least Once”)- QoS1 QoSLevel = 1 // QoS At Lease Once + QoS1 QoSLevel = 1 // QoS At Least Once
37-47: Tighten wording in defaults’ comments (plural and units)- DefaultDelay = 1000 // Default delay between connection - DefaultClients = 100 // Default clients to connect + DefaultDelay = 1000 // Default delay between connections (ms) + DefaultClients = 100 // Default number of clients to connect
171-175: Prefer typed QoS in the option to catch invalid values at compile timeAccept
QoSLevelinstead ofuint16. CLI parsing can cast toQoSLevelat the callsite; validation still enforces the range.-func WithQoS(qos uint16) Option { +func WithQoS(qos QoSLevel) Option { return func(b *Bench) { - b.qos = QoSLevel(qos) + b.qos = qos } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cmd/root.go(2 hunks)config.yml(1 hunks)internal/bench/bench.go(1 hunks)internal/bench/conn.go(1 hunks)pkg/config/config.go(1 hunks)pkg/logger/logger.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/bench/conn.go
🔇 Additional comments (3)
pkg/logger/logger.go (1)
51-95: Solid slog setup and attribute enrichment.Handler options, JSON/text selection, and grouped component handler look good. The attr preloading for env/service/version is clean.
config.yml (1)
2-2: Verify these keys are actually consumed by the loader.Confirm
versionandenvironmentmap to struct tags and are used downstream (e.g., propagated into logger metadata). If not, either wire them or drop to avoid drift.Run:
#!/bin/bash # Verify yaml tags and usage rg -nC2 -g '!**/vendor/**' -e 'type\s+Config\s+struct' -e 'yaml:"version"' -e 'yaml:"environment"' rg -nC2 -g '!**/vendor/**' 'InitializeCfg|SetDefaults|Environment|Version'Also applies to: 4-4
cmd/root.go (1)
13-17: Nice CLI synopsis.Short and Long descriptions are clear and concise.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cmd/root.go (1)
26-53: Config load + logger init error-path handling looks good (resolves prior review).You now guard InitializeCfg errors and propagate metadata into the logger. Nice.
🧹 Nitpick comments (6)
cmd/root.go (2)
28-33: Include service/env/version metadata in fallback logger as well.The error path initializes a basic dev logger but misses Service/Version/Environment fields, so log records differ vs. the success path.
Apply:
- if err != nil || cfg == nil { - logger.InitGlobalLogger(logger.DevelopmentConfig()) - logger.Error("Failed to initialize config", logger.ErrorAttr(err)) - Cfg = cfg - return - } + if err != nil || cfg == nil { + lcfg := logger.DevelopmentConfig() + lcfg.Service = "benchmq" + lcfg.Version = "unknown" + lcfg.Environment = "development" + logger.InitGlobalLogger(lcfg) + logger.Error("Failed to initialize config", logger.ErrorAttr(err)) + Cfg = cfg + return + }
3-9: Normalize environment for the switch to avoid case/spacing surprises.Ensures "Production", "PRODUCTION", etc. map correctly.
Apply:
import ( "os" + "strings" "github.com/pyr33x/benchmq/pkg/config" "github.com/pyr33x/benchmq/pkg/logger" "github.com/spf13/cobra" )- var lcfg logger.Config - switch Cfg.Environment { + var lcfg logger.Config + env := strings.ToLower(Cfg.Environment) + switch env { case "production": lcfg = logger.ProductionConfig() case "development": lcfg = logger.DevelopmentConfig() default: lcfg = logger.DevelopmentConfig() }Also applies to: 36-44
cmd/conn.go (2)
14-22: Polish user-facing help text.Replace placeholders with concise, accurate descriptions.
Apply:
Use: "conn", - Short: "A brief description of your command", - Long: `A longer description that spans multiple lines and likely contains examples -and usage of using your command. For example: - -Cobra is a CLI library for Go that empowers applications. -This application is a tool to generate the needed files -to quickly create a Cobra application.`, + Short: "Run a connection benchmark against the configured MQTT broker.", + Long: "Opens N concurrent MQTT connections (from config or flags) to measure connection throughput, failures, and timing.",
1-3: Remove or update placeholder copyright.Ship-ready CLIs shouldn’t include “NAME HERE ”.
Apply:
-/* -Copyright © 2025 NAME HERE <EMAIL ADDRESS> -*/ +// BenchMQ connection benchmark command.internal/bench/bench.go (2)
32-35: Fix typo in QoS comment (“At Lease Once” → “At Least Once”).Apply:
- QoS1 QoSLevel = 1 // QoS At Lease Once + QoS1 QoSLevel = 1 // QoS At Least Once
24-25: Minor comment cleanups for clarity.Apply:
- wg sync.WaitGroup // Wait Group + wg sync.WaitGroup // WaitGroup- DefaultDelay = 1000 // Default delay between connection + DefaultDelay = 1000 // Default delay between connections (ms)Also applies to: 38-45
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/conn.go(1 hunks)cmd/root.go(2 hunks)internal/bench/bench.go(1 hunks)pkg/logger/logger.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/logger/logger.go
🔇 Additional comments (1)
internal/bench/bench.go (1)
47-84: Constructor + validation look solid.Nil cfg guard, cfg-sourced host/port, options application, and fail-fast validation are all on point.
Also applies to: 86-137
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cmd/conn.go (1)
53-59: Return on constructor error and guard nil before RunConnections()Otherwise a nil deref panic is likely.
Apply:
- if err != nil { - logger.Error("Benchmark failed", logger.ErrorAttr(err)) - } - - // Run benchmark - b.RunConnections() + if err != nil { + logger.Error("failed to initialize benchmark", logger.ErrorAttr(err)) + return + } + if b == nil { + logger.Error("benchmark initialization returned nil without error") + return + } + // Run benchmark + b.RunConnections()
🧹 Nitpick comments (9)
cmd/conn.go (2)
66-66: Fix default clientID typo to match defaults elsewhere (“benchmq-client”)Prevents confusing drift between CLI and Bench defaults.
- connCmd.Flags().StringP("clientID", "i", "bencmq-client", "Client ID for MQTT connections") + connCmd.Flags().StringP("clientID", "i", "benchmq-client", "Client ID for MQTT connections")
68-68: Clarify units and align delay default with Bench defaultBench.DefaultDelay is 1000; CLI sets 10. Either change the default or explicitly document units.
- connCmd.Flags().IntP("delay", "d", 10, "Delay between each client connection") + connCmd.Flags().IntP("delay", "d", 1000, "Delay between each client connection in milliseconds")If delay is actually seconds, keep 10 but change the help text to “in seconds”.
internal/bench/bench.go (4)
33-35: Fix typo: “At Lease Once” → “At Least Once”Minor doc polish.
- QoS1 QoSLevel = 1 // QoS At Lease Once + QoS1 QoSLevel = 1 // QoS At Least Once
38-45: Polish constant comments (grammar/clarity)Use plural “connections”.
- DefaultDelay = 1000 // Default delay between connection + DefaultDelay = 1000 // Default delay between connections (ms)
136-139: Use the constant for default clientID to avoid driftKeeps one source of truth.
- if b.clientID == "" { - b.clientID = "benchmq-client" - } + if b.clientID == "" { + b.clientID = DefaultClientID + }
173-177: Type-safety for QoS option (optional)Consider an overload that accepts QoSLevel directly to avoid silent narrowing.
func WithQoS(qos uint16) Option { return func(b *Bench) { b.qos = QoSLevel(qos) } } + +func WithQoSLevel(q QoSLevel) Option { + return func(b *Bench) { + b.qos = q + } +}pkg/logger/logger.go (3)
262-270: Avoid colliding with slog’s built-in “time” field; rename attributeUse a more specific key to prevent confusion in logs.
-func TrackTime(t time.Time) slog.Attr { - return slog.Float64("time", float64(time.Since(t).Seconds())) +func TrackTime(t time.Time) slog.Attr { + return slog.Float64("elapsed_s", float64(time.Since(t).Seconds())) } -// TrackRaw creates a raw attribute +// TrackRawTime returns elapsed seconds func TrackRawTime(t time.Time) float64 { return float64(time.Since(t).Seconds()) }
120-145: Set Service to “benchmq” to match this projectCurrent value “goqtt” is misleading in emitted logs.
- Service: "goqtt", + Service: "benchmq",(Apply in both DevelopmentConfig and ProductionConfig.)
31-43: Unused Config fields (ShowCaller, TimeFormat)These are currently no-ops; either wire them (custom handler) or drop to reduce API surface.
Would you like a minimal custom handler to honor TimeFormat and a boolean to include caller info beyond AddSource?
Also applies to: 54-58
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cmd/conn.go(1 hunks)cmd/root.go(2 hunks)internal/bench/bench.go(1 hunks)internal/bench/conn.go(1 hunks)pkg/logger/logger.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/bench/conn.go
- cmd/root.go
🧰 Additional context used
🧬 Code graph analysis (2)
cmd/conn.go (3)
pkg/logger/logger.go (2)
Error(189-191)ErrorAttr(293-295)internal/bench/bench.go (6)
NewBenchmark(48-84)WithClients(149-153)WithDelay(143-147)WithCleanSession(167-171)WithKeepAlive(179-183)WithClientID(155-159)cmd/root.go (1)
Cfg(12-12)
internal/bench/bench.go (2)
pkg/logger/logger.go (4)
Config(32-43)Logger(25-29)Error(189-191)NewBenchmarkLogger(216-227)pkg/er/er.go (8)
Error(23-28)ErrNilConfig(20-20)ErrInvalidClients(15-15)ErrInvalidDelay(16-16)ErrEmptyHost(18-18)ErrEmptyTopic(19-19)ErrInvalidPort(17-17)ErrInvalidQoS(14-14)
🔇 Additional comments (3)
cmd/conn.go (1)
15-43: Flag parsing path looks goodEarly returns on parse failures and structured error logging are correct.
internal/bench/bench.go (1)
47-84: Bench builder, cfg sync, and validation look solidGood nil-cfg guard, cfg→host/port sourcing, options application, and explicit validation with structured errors.
Also applies to: 86-141, 185-201
pkg/logger/logger.go (1)
96-111: LGTM: fixed race in GetGlobalLogger with double-checked lockingThread-safe, readable, and avoids write-under-RLock.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
cmd/conn.go (1)
53-59: Return on constructor error and guard nil benchmark to prevent panicPrior review noted this; shadowing is fixed, but the nil guard/early return is still missing.
if err != nil { - logger.Error("Benchmark failed", logger.ErrorAttr(err)) + logger.Error("failed to initialize benchmark", logger.ErrorAttr(err)) + return } - // Run benchmark - b.RunConnections() + if b == nil { + logger.Error("benchmark is nil after initialization") + return + } + + // Run benchmark + b.RunConnections()
🧹 Nitpick comments (4)
internal/bench/bench.go (1)
153-157: Keep cfg in sync when overriding ClientID (optional)Helps downstream code that may read cfg.Client.ClientID.
func WithClientID(clientID string) Option { return func(b *Bench) { b.clientID = clientID + if b.cfg != nil { + b.cfg.Client.ClientID = clientID + } } }internal/bench/conn.go (3)
25-31: Log the actual error and defer disconnect after successful connectImproves diagnostics and avoids disconnecting an unconnected client.
b.logger.Info("Connecting Client", logger.ClientID(cfg.Client.ClientID), logger.State("connecting")) if err := client.Connect(); err != nil { - b.logger.Error("Couldn't establish client", logger.ClientID(cfg.Client.ClientID), logger.State("failed")) + b.logger.Error("Couldn't establish client", logger.ClientID(cfg.Client.ClientID), logger.State("failed"), logger.ErrorAttr(err)) return } + defer client.Disconnect() b.logger.LogClientConnection(cfg.Client.ClientID)
32-33: Skip trailing sleep after last spawnRemoves an extra delay after launching the final goroutine.
- time.Sleep(time.Duration(b.delay) * time.Millisecond) + if i+1 < b.clients { + time.Sleep(time.Duration(b.delay) * time.Millisecond) + }
13-13: Use RFC3339 timestamp instead of casting UnixNano to intAvoids potential int overflow and is more readable.
- b.logger.Info("Started connection benchmark", logger.Int("time", int(start.UnixNano()))) + b.logger.Info("Started connection benchmark", logger.String("started_at", start.Format(time.RFC3339Nano)))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/conn.go(1 hunks)internal/bench/bench.go(1 hunks)internal/bench/conn.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
internal/bench/conn.go (4)
internal/bench/bench.go (1)
Bench(14-27)pkg/logger/logger.go (6)
Info(179-181)Int(278-280)ClientID(253-255)State(258-260)String(273-275)TrackRawTime(268-270)pkg/config/config.go (1)
Client(27-33)internal/mqtt/mqtt.go (1)
NewClient(18-35)
cmd/conn.go (3)
pkg/logger/logger.go (2)
Error(189-191)ErrorAttr(293-295)internal/bench/bench.go (6)
NewBenchmark(48-83)WithClients(148-152)WithDelay(142-146)WithCleanSession(166-170)WithKeepAlive(178-182)WithClientID(154-158)cmd/root.go (1)
Cfg(12-12)
internal/bench/bench.go (2)
pkg/logger/logger.go (4)
Config(32-43)Logger(25-29)Error(189-191)NewBenchmarkLogger(216-227)pkg/er/er.go (8)
Error(23-28)ErrNilConfig(20-20)ErrInvalidClients(15-15)ErrInvalidDelay(16-16)ErrEmptyHost(18-18)ErrEmptyTopic(19-19)ErrInvalidPort(17-17)ErrInvalidQoS(14-14)
🔇 Additional comments (2)
internal/bench/bench.go (2)
47-56: Good nil-config guardConstructor fails fast with a structured error. LGTM.
85-140: Validation covers core constraintsClients/delay/host/topic/port/QoS checks look solid; defaulting clientID is sensible.
1 participant
Summary by CodeRabbit
New Features
Chores