Add MQTT connectivity diagnostics and startup logging#126
Merged
Conversation
Co-authored-by: Copilot <copilot@github.com>
Contributor
Reviewer's GuideAdds in-memory MQTT connectivity diagnostics and logging, wires them into the /mqtt view and template, and logs broker initialization and lifecycle events, with unit tests for the new behavior. Sequence diagram for MQTT connection lifecycle loggingsequenceDiagram
participant AppModule as AppModule
participant FlaskMqtt as FlaskMqtt
participant Broker as Broker
participant NotificationsModule as NotificationsModule
participant Logger as Logger
AppModule->>Logger: info Initializing MQTT broker at host:port (TLS=flag)
AppModule->>FlaskMqtt: init_app(app)
AppModule->>FlaskMqtt: subscribe(+/notification)
AppModule->>FlaskMqtt: subscribe(+/status)
AppModule->>Logger: info MQTT initialized, lifecycle reported by callbacks
Broker-->>FlaskMqtt: TCP connection established
FlaskMqtt-->>NotificationsModule: on_connect(client, userdata, flags, rc)
alt rc == 0
NotificationsModule->>Logger: info Connected to MQTT broker, flags, rc
NotificationsModule->>NotificationsModule: _append_mqtt_log(system, Connected, Connected)
else rc != 0
NotificationsModule->>Logger: error MQTT connection failed, flags, rc
NotificationsModule->>NotificationsModule: _append_mqtt_log(system, Connection failed, Connection failed)
end
Broker-->>FlaskMqtt: Disconnect
FlaskMqtt-->>NotificationsModule: on_disconnect(client, userdata, rc)
alt rc == 0
NotificationsModule->>Logger: info MQTT broker disconnected cleanly, rc
NotificationsModule->>NotificationsModule: _append_mqtt_log(system, Disconnected cleanly, Clean disconnect)
else rc != 0
NotificationsModule->>Logger: warning MQTT broker disconnected unexpectedly, rc
NotificationsModule->>NotificationsModule: _append_mqtt_log(system, Unexpected disconnect, Disconnected)
end
Broker-->>FlaskMqtt: PUBLISH topic payload
FlaskMqtt-->>NotificationsModule: on_message(client, userdata, message)
NotificationsModule->>NotificationsModule: _decode_payload(message.payload)
NotificationsModule->>Logger: debug Received message topic, payload
NotificationsModule->>NotificationsModule: _append_mqtt_log(topic, payload, Received)
Class diagram for MQTT diagnostics and logging utilitiesclassDiagram
class NotificationsModule {
+Mqtt mqtt
+threading.Lock mqtt_log_lock
+deque mqtt_log_entries
+_append_mqtt_log(topic, payload, status)
+get_mqtt_logs()
+_decode_payload(payload)
+is_mqtt_connected()
+handle_connect(client, userdata, flags, rc)
+handle_disconnect(client, userdata, rc)
+handle_messages(client, userdata, message)
+handle_publish(client, userdata, mid)
+handle_subscribe(client, userdata, mid, granted_qos)
}
class RoutesModule {
+mqtt_log()
}
class AppModule {
+mqtt_init_block()
}
class FlaskMqtt {
+connected
+init_app(app)
+subscribe(topic)
+on_connect()
+on_disconnect()
+on_message()
+on_publish()
+on_subscribe()
}
class MqttLogTemplate {
+broker_connected
+tls_enabled
+log_entries
}
NotificationsModule "1" --> "1" FlaskMqtt : wraps
RoutesModule --> NotificationsModule : uses get_mqtt_logs
RoutesModule --> NotificationsModule : uses is_mqtt_connected
RoutesModule --> MqttLogTemplate : renders
AppModule --> NotificationsModule : uses mqtt
AppModule --> FlaskMqtt : init_app, subscribe
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request improves MQTT observability by adding detailed initialization logs in app.py and implementing connection and disconnection handlers in notifications.py. The review feedback suggests refining these handlers to correctly interpret return codes, specifically by logging connection failures as errors and distinguishing between intentional and unexpected disconnections to reduce log noise.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider differentiating between clean and error disconnects in
handle_disconnect(e.g., log atinfoforrc == 0andwarningotherwise) so that normal shutdowns don’t generate warning noise in the logs. - The
logger.info('MQTT initialized; connected=%s', mqtt.connected)call immediately aftermqtt.init_app(app)may be misleading because the connection state is usually established asynchronously; you might want to rely on theon_connect/on_disconnectcallbacks for accurate connection status instead of the immediate post-init value.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider differentiating between clean and error disconnects in `handle_disconnect` (e.g., log at `info` for `rc == 0` and `warning` otherwise) so that normal shutdowns don’t generate warning noise in the logs.
- The `logger.info('MQTT initialized; connected=%s', mqtt.connected)` call immediately after `mqtt.init_app(app)` may be misleading because the connection state is usually established asynchronously; you might want to rely on the `on_connect`/`on_disconnect` callbacks for accurate connection status instead of the immediate post-init value.
## Individual Comments
### Comment 1
<location path="notifications.py" line_range="18-20" />
<code_context>
+ logger.info('Connected to MQTT broker; flags=%s, rc=%s', flags, rc)
+
+
+@mqtt.on_disconnect()
+def handle_disconnect(_client, _userdata, rc):
+ logger.warning('Disconnected from MQTT broker; rc=%s', rc)
+
+
</code_context>
<issue_to_address>
**suggestion:** Consider differentiating between clean and unclean disconnects in logging
All disconnects are currently logged as warnings regardless of `rc`. Since `rc == 0` typically means a clean, expected disconnect, consider logging those at `INFO` and reserving `WARNING` for non‑zero `rc` to reduce noise and highlight actual issues.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…live feed and logs
Owner
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The status strings (e.g. 'Connected', 'Connection failed', 'Clean disconnect') and the
positive_statusesmapping are duplicated acrossnotifications.pyandroutes.mqtt_log; consider centralizing these in a shared constant or enum so status handling and styling stay consistent as new cases are added. - The
_decode_payloadhelper logs a warning on every non-UTF8 payload; if binary payloads are expected, consider downgrading this to debug or adding some form of rate limiting to avoid noisy logs in high-throughput scenarios.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The status strings (e.g. 'Connected', 'Connection failed', 'Clean disconnect') and the `positive_statuses` mapping are duplicated across `notifications.py` and `routes.mqtt_log`; consider centralizing these in a shared constant or enum so status handling and styling stay consistent as new cases are added.
- The `_decode_payload` helper logs a warning on every non-UTF8 payload; if binary payloads are expected, consider downgrading this to debug or adding some form of rate limiting to avoid noisy logs in high-throughput scenarios.
## Individual Comments
### Comment 1
<location path="routes.py" line_range="168-172" />
<code_context>
def mqtt_log():
- return render_template('mqtt_log.html')
+ connected = is_mqtt_connected()
+ positive_statuses = {'Received', 'Connected', 'Clean disconnect'}
+ log_entries = [
+ {
+ **entry,
+ 'status_class': 'status-pill--active' if entry.get('status') in positive_statuses else '',
+ }
+ for entry in get_mqtt_logs()
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Avoid duplicating status strings in multiple places by centralizing them as constants or deriving them from a single source.
These status labels are now defined both in the MQTT callbacks (e.g. `'Connected'`, `'Clean disconnect'`) and here for `status_class`, so changes in one place can silently desync the mapping. Please either define these statuses (or a status→class mapping) once in `notifications.py` and reuse it here, or compute `status_class` inside `_append_mqtt_log` to keep the logic centralized.
Suggested implementation:
```python
def mqtt_log():
connected = is_mqtt_connected()
log_entries = get_mqtt_logs()
return render_template(
'mqtt_log.html',
broker_connected=connected,
tls_enabled=current_app.config.get('MQTT_TLS_ENABLED', False),
log_entries=log_entries,
)
```
To fully implement the suggestion and avoid duplicated status strings:
1. In `notifications.py`, within `_append_mqtt_log` (or the equivalent function that creates MQTT log entries), compute and attach `status_class` there, e.g.:
- Define a single shared mapping or set for statuses:
- `POSITIVE_STATUSES = {'Received', 'Connected', 'Clean disconnect'}` **or**
- `STATUS_CLASSES = {'Received': 'status-pill--active', 'Connected': 'status-pill--active', 'Clean disconnect': 'status-pill--active'}`.
- When constructing each log entry, set `entry['status_class']` based on this mapping/set.
2. Ensure `get_mqtt_logs()` returns entries that include `status_class`, so `routes.py` can just pass through the list without recomputing it.
This keeps all status/visual mapping logic in one module (`notifications.py`) and prevents future drift between callbacks and the route.
</issue_to_address>
### Comment 2
<location path="routes.py" line_range="176-181" />
<code_context>
+ return render_template(
+ 'mqtt_log.html',
+ broker_connected=connected,
+ tls_enabled=current_app.config.get('MQTT_TLS_ENABLED', False),
+ log_entries=log_entries,
+ )
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Normalize `MQTT_TLS_ENABLED` to a strict boolean to avoid surprising truthy/falsey behavior in the template.
As written, non-boolean config values like `'false'` or `0` will be treated as truthy by Jinja, so the UI may show "Encrypted" when TLS is effectively off. Coercing the config value to a real boolean (e.g., `bool(current_app.config.get('MQTT_TLS_ENABLED', False))` or explicit string handling) will ensure the status reflects the actual TLS setting.
```suggestion
return render_template(
'mqtt_log.html',
broker_connected=connected,
tls_enabled=bool(current_app.config.get('MQTT_TLS_ENABLED', False)),
log_entries=log_entries,
)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…ary-payload log to debug
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
1 participant
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.
This PR adds MQTT connectivity diagnostics for the HiveMQ broker setup. It logs broker initialization details and registers MQTT connect/disconnect event handlers so connection state is visible in application logs.
Summary by Sourcery
Add in-memory MQTT event monitoring, expose broker connection and TLS status in the MQTT monitor view, and log broker initialization and connection lifecycle events.
New Features:
Enhancements:
Tests: