RHINENG-22557; adding basic error handling for MQTT#459
Draft
kami-nashi wants to merge 1 commit into
Draft
Conversation
Reviewer's GuideAdds structured MQTT connection error classification (TLS, network, protocol, runtime), propagates these classified errors through broker connection creation and connection-lost handlers, and enhances logging/fatal handling with error codes, categories, and kinds for better observability and alerting. Sequence diagram for MQTT broker connection and error handlingsequenceDiagram
participant Consumer as MqttMessageConsumer
participant MqttPkg as mqtt_package
participant Paho as Paho_MQTT_Client
participant Classifier as MqttErrorClassifier
participant Logger as Logger
Consumer->>MqttPkg: CreateBrokerConnection(brokerUrl, options)
MqttPkg->>MqttPkg: NewBrokerOptions()
MqttPkg->>Paho: NewClient(connOpts)
MqttPkg->>Paho: Connect()
Paho-->>MqttPkg: ConnectToken (rc, error)
MqttPkg->>Classifier: classifyProtocolReturnCode(rc)
Classifier-->>MqttPkg: protoErr
alt Protocol_error
MqttPkg->>Logger: Error(MQTT CONNACK not accepted)
MqttPkg-->>Consumer: protoErr
else No_protocol_error
MqttPkg->>Classifier: classifyConnectError(token.Error())
Classifier-->>MqttPkg: connectErr
alt Connect_error
MqttPkg->>Logger: Error(Unable to connect, fields: code, kind, category)
MqttPkg-->>Consumer: connectErr
else Success
MqttPkg->>Logger: Info(Connected to MQTT broker)
MqttPkg-->>Consumer: mqttClient
end
end
alt Consumer_receives_error
Consumer->>Consumer: switch on error kind
Consumer->>Logger: logConnectFatal(..., mqtt_error_code, mqtt_error_category, mqtt_error_kind)
Consumer->>Consumer: process fatal exit
end
Sequence diagram for MQTT connection-lost classification and notificationsequenceDiagram
participant Broker as MQTT_Broker
participant Paho as Paho_MQTT_Client
participant Handler as notifyOnMqttConnectionLostHandler
participant Classifier as MqttErrorClassifier
participant Chan as mqttConnectionFailedChan
participant Logger as Logger
Broker-->>Paho: Connection lost (network/runtime failure)
Paho-->>Handler: OnConnectionLost(client, err)
Handler->>Classifier: ClassifyConnectionLostError(err)
Classifier-->>Handler: classified ConnectError
Handler->>Logger: Warn("MQTT connection dropped", code, kind, category)
Handler->>Paho: Disconnect(1000)
Handler->>Chan: send(classified)
note over Chan,Logger: logMqttConnectionLostHandler performs similar classification and logs at Info level
Class diagram for MQTT connection error classificationclassDiagram
class ConnectError {
int Code
error Kind
error Cause
string Category
Error() string
Unwrap() error
Is(target error) bool
}
class MqttErrorClassifier {
<<utility>>
classifyConnectError(err error) error
classifyProtocolReturnCode(rc byte) error
classifyConnectionLostError(err error) error
ClassifyConnectionLostError(err error) ConnectError
}
class MqttMessageConsumerErrors {
<<utility>>
logConnectFatal(connErr ConnectError, err error, msg string, fallbackCode int, fallbackCategory string) void
notifyOnMqttConnectionLostHandler(mqttConnectionFailedChan chan_error) func
logMqttConnectionLostHandler(client MQTT_Client, err error) void
}
MqttErrorClassifier --> ConnectError : creates
MqttMessageConsumerErrors ..> ConnectError : uses
MqttMessageConsumerErrors ..> MqttErrorClassifier : calls
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The numeric error codes (e.g. 104/111/113 mapped to ECONNRESET/ECONNREFUSED/EHOSTUNREACH and HTTP-like 495/520/532) are fairly implicit and platform-specific; consider centralizing them with brief comments or using named errno constants where possible so future readers don’t have to reverse-map or assume Linux semantics.
- The error classification functions rely on several string contains checks (e.g. "connection lost", "broken pipe", "timeout"); if these messages change upstream or get localized this logic will silently degrade, so it may be worth isolating these heuristics behind clearly documented helpers or narrowing them to only cases you’ve actually observed in production logs.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The numeric error codes (e.g. 104/111/113 mapped to ECONNRESET/ECONNREFUSED/EHOSTUNREACH and HTTP-like 495/520/532) are fairly implicit and platform-specific; consider centralizing them with brief comments or using named errno constants where possible so future readers don’t have to reverse-map or assume Linux semantics.
- The error classification functions rely on several string contains checks (e.g. "connection lost", "broken pipe", "timeout"); if these messages change upstream or get localized this logic will silently degrade, so it may be worth isolating these heuristics behind clearly documented helpers or narrowing them to only cases you’ve actually observed in production logs.
## Individual Comments
### Comment 1
<location> `internal/mqtt/broker_client.go:19` </location>
<code_context>
"github.com/sirupsen/logrus"
)
+var (
+ // TLS failures
+ ErrTLSHandshake = errors.New("mqtt tls handshake failed")
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the error-classification logic into a dedicated file and shared helpers to reduce duplication and keep `CreateBrokerConnection` focused on connection setup.
You can keep all of this functionality while reducing complexity and duplication with a few small refactors.
### 1. Extract classification into its own file
Even without a new package, splitting the taxonomy and classifiers into `connect_errors.go` (or similar) will make `broker_client.go` easier to scan.
```go
// connect_errors.go
package mqtt
// all: ConnectError, sentinel errors, codes, categories,
// classifyConnectError, classifyProtocolReturnCode,
// classifyConnectionLostError, ClassifyConnectionLostError
```
```go
// broker_client.go
package mqtt
func CreateBrokerConnection(...) (..., error) {
// unchanged except calls to classify* helpers
}
```
No behavior change, but `CreateBrokerConnection` is no longer buried in 200 lines of taxonomy logic.
### 2. Share common network classification logic
`classifyConnectError` and `classifyConnectionLostError` both detect timeouts, EOF, EPIPE, ECONNRESET, and string hints. You can centralize the core logic and parameterize category/default:
```go
func classifyNetError(err error, category string, defaultKind error) *ConnectError {
if err == nil {
return &ConnectError{
Code: CodeBrokerConnect,
Kind: defaultKind,
Cause: errors.New("connection lost"),
Category: category,
}
}
if ce, ok := err.(*ConnectError); ok {
// keep pre-classified error
if ce.Category == "" {
ce.Category = category
}
return ce
}
if nerr, ok := err.(net.Error); ok && nerr.Timeout() {
return newConnectError(CodeTimeout, category, ErrTimeout, err).(*ConnectError)
}
if errors.Is(err, io.EOF) {
return newConnectError(CodeEOF, category, ErrEOF, err).(*ConnectError)
}
if errors.Is(err, syscall.EPIPE) {
return newConnectError(CodeBrokenPipe, category, ErrBrokenPipe, err).(*ConnectError)
}
if errors.Is(err, syscall.ECONNRESET) {
return newConnectError(CodeConnectionLost, category, ErrConnectionLost, err).(*ConnectError)
}
if ce := classifyByMessage(err, category); ce != nil {
return ce
}
return newConnectError(CodeBrokerConnect, category, defaultKind, err).(*ConnectError)
}
```
Then:
```go
func classifyConnectError(err error) error {
// keep TLS / x509 / net.OpError special cases here...
// fall back to generic network classifier
return classifyNetError(err, CategoryNetwork, ErrBrokerConnect)
}
func classifyConnectionLostError(err error) error {
return classifyNetError(err, CategoryRuntime, ErrConnectionLost)
}
```
This reduces branching duplication while keeping the semantics (different categories and defaults) intact.
### 3. Extract string-based heuristics into a table-driven helper
Both classifiers use similar `strings.Contains` chains. You can encapsulate this and make it easier to extend:
```go
type messageRule struct {
substr string
code int
kind error
}
var networkMessageRules = []messageRule{
{"connection lost", CodeConnectionLost, ErrConnectionLost},
{"connection refused", CodeConnectionRefused, ErrConnectionRefused},
{"host unreachable", CodeHostUnreachable, ErrHostUnreachable},
{"timeout", CodeTimeout, ErrTimeout},
{"eof", CodeEOF, ErrEOF},
{"broken pipe", CodeBrokenPipe, ErrBrokenPipe},
}
func classifyByMessage(err error, category string) *ConnectError {
lower := strings.ToLower(err.Error())
for _, r := range networkMessageRules {
if strings.Contains(lower, r.substr) {
return newConnectError(r.code, category, r.kind, err).(*ConnectError)
}
}
return nil
}
```
`classifyConnectError` and `classifyConnectionLostError` then just call `classifyByMessage`, instead of each having their own long `switch` on `strings.Contains`.
These changes keep all existing behavior but narrow the responsibilities of `broker_client.go`, reduce duplication, and make the classification logic easier to follow and extend.
</issue_to_address>
### Comment 2
<location> `cmd/cloud-connector/mqtt_message_consumer.go:98` </location>
<code_context>
mqttClient, err := mqtt.CreateBrokerConnection(cfg.MqttBrokerAddress, brokerOptions...)
if err != nil {
- logger.LogFatalError("Failed to connect to MQTT broker", err)
</code_context>
<issue_to_address>
**issue (complexity):** Consider centralizing MQTT error classification and logging into shared helpers and data-driven tables to reduce duplicated logic and verbose conditional branching.
You can keep the richer diagnostics and reduce the complexity/noise in three focused steps.
---
### 1. Remove `connErr` from callsites and let `logConnectFatal` own the `errors.As`
Right now every caller must do `errors.As` and pass `connErr` plus fallback code/category. You can keep the same behavior but centralize the `ConnectError` introspection:
```go
// Centralized helper
func logConnectFatal(err error, msg string, fallbackCode int, fallbackCategory string) {
var connErr *mqtt.ConnectError
_ = errors.As(err, &connErr)
code := fallbackCode
category := fallbackCategory
kind := ""
if connErr != nil {
if connErr.Code != 0 {
code = connErr.Code
}
if connErr.Category != "" {
category = connErr.Category
}
if connErr.Kind != nil {
kind = connErr.Kind.Error()
}
}
logger.Log.WithFields(logrus.Fields{
"error": err,
"mqtt_error_code": code,
"mqtt_error_kind": kind,
"mqtt_error_category": category,
}).Fatal(msg)
}
```
Callsites become simpler (no `connErr` parameter and no `errors.As` at the top):
```go
mqttClient, err := mqtt.CreateBrokerConnection(cfg.MqttBrokerAddress, brokerOptions...)
if err != nil {
switch {
case errors.Is(err, mqtt.ErrTLSHandshake):
logConnectFatal(err, "MQTT TLS handshake failed", mqtt.CodeTLSHandshake, mqtt.CategoryTLS)
// ...
default:
logger.LogFatalError("Failed to connect to MQTT broker", err)
}
}
```
This keeps the exact same semantics (including fallbacks) but removes repeated error-unpacking logic from the startup path.
---
### 2. Replace the long `switch` with a compact table of error mappings
The `switch` is mostly data (error → message/code/category). You can move that to a small table and loop over it, which makes it easier to scan and extend:
```go
type mqttConnectCase struct {
target error
msg string
code int
category string
}
var mqttConnectCases = []mqttConnectCase{
// TLS-specific
{mqtt.ErrTLSHandshake, "MQTT TLS handshake failed", mqtt.CodeTLSHandshake, mqtt.CategoryTLS},
// Protocol and CONNACK failures
{mqtt.ErrProtocolVersion, "MQTT protocol/auth error (protocol version rejected)", mqtt.CodeProtocolVersion, mqtt.CategoryProtocol},
{mqtt.ErrIdentifierRejected, "MQTT protocol/auth error (client identifier rejected)", mqtt.CodeIdentifierRejected, mqtt.CategoryProtocol},
{mqtt.ErrServerUnavailable, "MQTT protocol/auth error (server unavailable)", mqtt.CodeServerUnavailable, mqtt.CategoryProtocol},
{mqtt.ErrBadUsernameOrPassword, "MQTT protocol/auth error (bad username or password)", mqtt.CodeBadUsernameOrPassword, mqtt.CategoryProtocol},
{mqtt.ErrNotAuthorized, "MQTT protocol/auth error (not authorized)", mqtt.CodeNotAuthorized, mqtt.CategoryProtocol},
// Network or transport failures
{mqtt.ErrConnectionLost, "MQTT connection dropped during handshake", mqtt.CodeConnectionLost, mqtt.CategoryNetwork},
{mqtt.ErrConnectionRefused, "MQTT broker refused TCP connection", mqtt.CodeConnectionRefused, mqtt.CategoryNetwork},
{mqtt.ErrHostUnreachable, "MQTT broker unreachable (routing/DNS/VPC)", mqtt.CodeHostUnreachable, mqtt.CategoryNetwork},
{mqtt.ErrBrokerConnect, "MQTT connect failure (unspecified)", mqtt.CodeBrokerConnect, mqtt.CategoryNetwork},
}
func handleMqttConnectError(err error) {
for _, c := range mqttConnectCases {
if errors.Is(err, c.target) {
logConnectFatal(err, c.msg, c.code, c.category)
return
}
}
logger.LogFatalError("Failed to connect to MQTT broker", err)
}
```
Startup code becomes:
```go
mqttClient, err := mqtt.CreateBrokerConnection(cfg.MqttBrokerAddress, brokerOptions...)
if err != nil {
handleMqttConnectError(err)
}
```
Behavior is unchanged (same messages, codes, categories), but the branching is now purely data-driven and less coupled to the callsite.
---
### 3. Deduplicate connection-lost logging via a shared helper
`notifyOnMqttConnectionLostHandler` and `logMqttConnectionLostHandler` both classify and log the same fields. You can centralize the classification+logging and only vary severity and side effects:
```go
func logMqttConnectionLost(level logrus.Level, err error) mqtt.ClassifiedError {
classified := mqtt.ClassifyConnectionLostError(err)
logger.Log.WithFields(logrus.Fields{
"error": err,
"mqtt_error_code": classified.Code,
"mqtt_error_kind": classified.Kind,
"mqtt_error_category": classified.Category,
}).Log(level, "MQTT connection dropped")
return classified
}
```
Handlers then become very small:
```go
func notifyOnMqttConnectionLostHandler(mqttConnectionFailedChan chan error) func(MQTT.Client, error) {
return func(client MQTT.Client, err error) {
classified := logMqttConnectionLost(logrus.WarnLevel, err)
client.Disconnect(1000)
mqttConnectionFailedChan <- classified // preserves classified payload
}
}
func logMqttConnectionLostHandler(client MQTT.Client, err error) {
logMqttConnectionLost(logrus.InfoLevel, err)
}
```
This keeps the richer diagnostics and classification you added, while making the control flow and responsibilities much clearer and easier to extend.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| "github.com/sirupsen/logrus" | ||
| ) | ||
|
|
||
| var ( |
There was a problem hiding this comment.
issue (complexity): Consider extracting the error-classification logic into a dedicated file and shared helpers to reduce duplication and keep CreateBrokerConnection focused on connection setup.
You can keep all of this functionality while reducing complexity and duplication with a few small refactors.
1. Extract classification into its own file
Even without a new package, splitting the taxonomy and classifiers into connect_errors.go (or similar) will make broker_client.go easier to scan.
// connect_errors.go
package mqtt
// all: ConnectError, sentinel errors, codes, categories,
// classifyConnectError, classifyProtocolReturnCode,
// classifyConnectionLostError, ClassifyConnectionLostError// broker_client.go
package mqtt
func CreateBrokerConnection(...) (..., error) {
// unchanged except calls to classify* helpers
}No behavior change, but CreateBrokerConnection is no longer buried in 200 lines of taxonomy logic.
2. Share common network classification logic
classifyConnectError and classifyConnectionLostError both detect timeouts, EOF, EPIPE, ECONNRESET, and string hints. You can centralize the core logic and parameterize category/default:
func classifyNetError(err error, category string, defaultKind error) *ConnectError {
if err == nil {
return &ConnectError{
Code: CodeBrokerConnect,
Kind: defaultKind,
Cause: errors.New("connection lost"),
Category: category,
}
}
if ce, ok := err.(*ConnectError); ok {
// keep pre-classified error
if ce.Category == "" {
ce.Category = category
}
return ce
}
if nerr, ok := err.(net.Error); ok && nerr.Timeout() {
return newConnectError(CodeTimeout, category, ErrTimeout, err).(*ConnectError)
}
if errors.Is(err, io.EOF) {
return newConnectError(CodeEOF, category, ErrEOF, err).(*ConnectError)
}
if errors.Is(err, syscall.EPIPE) {
return newConnectError(CodeBrokenPipe, category, ErrBrokenPipe, err).(*ConnectError)
}
if errors.Is(err, syscall.ECONNRESET) {
return newConnectError(CodeConnectionLost, category, ErrConnectionLost, err).(*ConnectError)
}
if ce := classifyByMessage(err, category); ce != nil {
return ce
}
return newConnectError(CodeBrokerConnect, category, defaultKind, err).(*ConnectError)
}Then:
func classifyConnectError(err error) error {
// keep TLS / x509 / net.OpError special cases here...
// fall back to generic network classifier
return classifyNetError(err, CategoryNetwork, ErrBrokerConnect)
}
func classifyConnectionLostError(err error) error {
return classifyNetError(err, CategoryRuntime, ErrConnectionLost)
}This reduces branching duplication while keeping the semantics (different categories and defaults) intact.
3. Extract string-based heuristics into a table-driven helper
Both classifiers use similar strings.Contains chains. You can encapsulate this and make it easier to extend:
type messageRule struct {
substr string
code int
kind error
}
var networkMessageRules = []messageRule{
{"connection lost", CodeConnectionLost, ErrConnectionLost},
{"connection refused", CodeConnectionRefused, ErrConnectionRefused},
{"host unreachable", CodeHostUnreachable, ErrHostUnreachable},
{"timeout", CodeTimeout, ErrTimeout},
{"eof", CodeEOF, ErrEOF},
{"broken pipe", CodeBrokenPipe, ErrBrokenPipe},
}
func classifyByMessage(err error, category string) *ConnectError {
lower := strings.ToLower(err.Error())
for _, r := range networkMessageRules {
if strings.Contains(lower, r.substr) {
return newConnectError(r.code, category, r.kind, err).(*ConnectError)
}
}
return nil
}classifyConnectError and classifyConnectionLostError then just call classifyByMessage, instead of each having their own long switch on strings.Contains.
These changes keep all existing behavior but narrow the responsibilities of broker_client.go, reduce duplication, and make the classification logic easier to follow and extend.
| @@ -97,7 +97,40 @@ func startMqttMessageConsumer(mgmtAddr string) { | |||
|
|
|||
| mqttClient, err := mqtt.CreateBrokerConnection(cfg.MqttBrokerAddress, brokerOptions...) | |||
There was a problem hiding this comment.
issue (complexity): Consider centralizing MQTT error classification and logging into shared helpers and data-driven tables to reduce duplicated logic and verbose conditional branching.
You can keep the richer diagnostics and reduce the complexity/noise in three focused steps.
1. Remove connErr from callsites and let logConnectFatal own the errors.As
Right now every caller must do errors.As and pass connErr plus fallback code/category. You can keep the same behavior but centralize the ConnectError introspection:
// Centralized helper
func logConnectFatal(err error, msg string, fallbackCode int, fallbackCategory string) {
var connErr *mqtt.ConnectError
_ = errors.As(err, &connErr)
code := fallbackCode
category := fallbackCategory
kind := ""
if connErr != nil {
if connErr.Code != 0 {
code = connErr.Code
}
if connErr.Category != "" {
category = connErr.Category
}
if connErr.Kind != nil {
kind = connErr.Kind.Error()
}
}
logger.Log.WithFields(logrus.Fields{
"error": err,
"mqtt_error_code": code,
"mqtt_error_kind": kind,
"mqtt_error_category": category,
}).Fatal(msg)
}Callsites become simpler (no connErr parameter and no errors.As at the top):
mqttClient, err := mqtt.CreateBrokerConnection(cfg.MqttBrokerAddress, brokerOptions...)
if err != nil {
switch {
case errors.Is(err, mqtt.ErrTLSHandshake):
logConnectFatal(err, "MQTT TLS handshake failed", mqtt.CodeTLSHandshake, mqtt.CategoryTLS)
// ...
default:
logger.LogFatalError("Failed to connect to MQTT broker", err)
}
}This keeps the exact same semantics (including fallbacks) but removes repeated error-unpacking logic from the startup path.
2. Replace the long switch with a compact table of error mappings
The switch is mostly data (error → message/code/category). You can move that to a small table and loop over it, which makes it easier to scan and extend:
type mqttConnectCase struct {
target error
msg string
code int
category string
}
var mqttConnectCases = []mqttConnectCase{
// TLS-specific
{mqtt.ErrTLSHandshake, "MQTT TLS handshake failed", mqtt.CodeTLSHandshake, mqtt.CategoryTLS},
// Protocol and CONNACK failures
{mqtt.ErrProtocolVersion, "MQTT protocol/auth error (protocol version rejected)", mqtt.CodeProtocolVersion, mqtt.CategoryProtocol},
{mqtt.ErrIdentifierRejected, "MQTT protocol/auth error (client identifier rejected)", mqtt.CodeIdentifierRejected, mqtt.CategoryProtocol},
{mqtt.ErrServerUnavailable, "MQTT protocol/auth error (server unavailable)", mqtt.CodeServerUnavailable, mqtt.CategoryProtocol},
{mqtt.ErrBadUsernameOrPassword, "MQTT protocol/auth error (bad username or password)", mqtt.CodeBadUsernameOrPassword, mqtt.CategoryProtocol},
{mqtt.ErrNotAuthorized, "MQTT protocol/auth error (not authorized)", mqtt.CodeNotAuthorized, mqtt.CategoryProtocol},
// Network or transport failures
{mqtt.ErrConnectionLost, "MQTT connection dropped during handshake", mqtt.CodeConnectionLost, mqtt.CategoryNetwork},
{mqtt.ErrConnectionRefused, "MQTT broker refused TCP connection", mqtt.CodeConnectionRefused, mqtt.CategoryNetwork},
{mqtt.ErrHostUnreachable, "MQTT broker unreachable (routing/DNS/VPC)", mqtt.CodeHostUnreachable, mqtt.CategoryNetwork},
{mqtt.ErrBrokerConnect, "MQTT connect failure (unspecified)", mqtt.CodeBrokerConnect, mqtt.CategoryNetwork},
}
func handleMqttConnectError(err error) {
for _, c := range mqttConnectCases {
if errors.Is(err, c.target) {
logConnectFatal(err, c.msg, c.code, c.category)
return
}
}
logger.LogFatalError("Failed to connect to MQTT broker", err)
}Startup code becomes:
mqttClient, err := mqtt.CreateBrokerConnection(cfg.MqttBrokerAddress, brokerOptions...)
if err != nil {
handleMqttConnectError(err)
}Behavior is unchanged (same messages, codes, categories), but the branching is now purely data-driven and less coupled to the callsite.
3. Deduplicate connection-lost logging via a shared helper
notifyOnMqttConnectionLostHandler and logMqttConnectionLostHandler both classify and log the same fields. You can centralize the classification+logging and only vary severity and side effects:
func logMqttConnectionLost(level logrus.Level, err error) mqtt.ClassifiedError {
classified := mqtt.ClassifyConnectionLostError(err)
logger.Log.WithFields(logrus.Fields{
"error": err,
"mqtt_error_code": classified.Code,
"mqtt_error_kind": classified.Kind,
"mqtt_error_category": classified.Category,
}).Log(level, "MQTT connection dropped")
return classified
}Handlers then become very small:
func notifyOnMqttConnectionLostHandler(mqttConnectionFailedChan chan error) func(MQTT.Client, error) {
return func(client MQTT.Client, err error) {
classified := logMqttConnectionLost(logrus.WarnLevel, err)
client.Disconnect(1000)
mqttConnectionFailedChan <- classified // preserves classified payload
}
}
func logMqttConnectionLostHandler(client MQTT.Client, err error) {
logMqttConnectionLost(logrus.InfoLevel, err)
}This keeps the richer diagnostics and classification you added, while making the control flow and responsibilities much clearer and easier to extend.
loadtheaccumulator
requested changes
Feb 18, 2026
Contributor
There was a problem hiding this comment.
There is complexity and bug risk with this pass of the PR.
I recommend breaking it into multiple PRs, and there are at least a couple of ways to do it. One approach that scales is:
- Simplify the PR. Remove the error code lookups and scale back to a generic error handler for each return to limit the number of major types in the first PR.
- Create scaffolding. Move the error handling into a separate file from the logic to start and, if it grows too large in content and/or scope, break that file down even more. This makes it easier to read and possibly easier to add unit tests. It is not a must to call the functions in the scaffolding in the same PR the scaffolding is created and can allow code to be reviewed, tested, and pushed to prod more quickly.
- This PR only covers the logging aspect, which is good. Create a separate PR to enhance the scaffolding with Prometheus metrics and labels.
- Consider calling the scaffolding from the application logic in a final separate PR.
EDIT: 5 is: include unit tests with the code.
Once that is working in production. Adding further enhancements such as the mqtt library error code lookups, etc. to scaffolding in subsequent PRs will reduce risk and make the changes easier to review and test.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What?
Why?
Improves visibility, alerting, and logging for better historical logging as well as better responses for outages and maintenance recovery.
How?
Improves MQTT connect/connection-lost error handling by classifying TLS/protocol/network failures, adding scenario-specific log messages and structured fields for alerting. No new libraries added.
Testing
Anything Else?
This PR may need additional comments, commits, reviews, tests, etc before merging
Secure Coding Practices Checklist Link
Secure Coding Checklist
Summary by Sourcery
Improve MQTT broker connection error handling and logging for better classification and observability of connection failures.
Enhancements: