Skip to content

Conversation

BertKleewein
Copy link
Member

@BertKleewein BertKleewein commented Nov 23, 2022

Looking for feedback on mqtt_transport.py changes. Tony and I have been talking about something like this as a way for customers to get a quick status on the health of our library. This is far from complete, but the intention is to provide data for observability/debugging only with the hope of avoiding the need to pour through reams of debug data.

The config object looks like this;

  "paho_config": {
    "transport": "websockets",
    "protocol": "4",
    "keepalive": 60,
    "connect_timeout": 5.0,
    "reconnect_on_failure": true,
    "reconnect_delay_min": 7200,
    "reconnect_delay_max": 120,
    "host": "bertk.azure-devices.net",
    "port": 443,
    "proxy_args": {},
    "socket_class": "WebsocketWrapper",
    "socket_name": "No socket name"
  },

and the status object looks like this:

  "paho_status": {
    "connect_rc_codes": {
      "0": 2
    },
    "on_connect_rc_codes": {
      "0": 2
    },
    "connect_exceptions": {
      "timeout": 3
    },
    "disconnect_rc_codes": {},
    "on_disconnect_rc_codes": {
      "16": 1,
      "0": 1
    },
    "disconnect_exceptions": {},
    "publish_rc_codes": {
      "0": 631
    },
    "publish_exceptions": {},
    "subscribe_rc_codes": {},
    "subscribe_exceptions": {},
    "unsubscribe_rc_codes": {},
    "unsubscribe_exceptions": {},
    "count_message_received": 0,
    "count_subscribe": 0,
    "count_suback": 0,
    "count_unsubscribe": 0,
    "count_unsuback": 0,
    "count_publish": 631,
    "count_puback": 631,
    "shut_down": false,
    "time_since_last_paho_traffic_in": "0:00:00.779999",
    "time_since_last_paho_traffic_out": "0:00:05.885535",
    "client_object_id": 140285330079296,
    "thread_name": "Thread-14",
    "thread_is_alive": "True",
    "len_out_mesage_queue": 0,
    "len_in_message_queue": 0,
    "len_out_pakcet_queue": 0,
    "thread_terminate": false,
    "paho_connection_state": 1
  },

thread_is_alive: bool = False
len_out_mesage_queue: int = 0
len_in_message_queue: int = 0
len_out_pakcet_queue: int = 0

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spelling?

Copy link

@anthonyvercolano anthonyvercolano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I like it.

Copy link
Member

@cartertinney cartertinney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments

from . import transport_exceptions as exceptions
from enum import Enum
import socks
import dataclasses
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this requires Python 3.7, there should be an update to the setup.py to exclude Python 3.6 (and a removal of the associated classifier)

@dataclasses.dataclass
class PahoStatus(object):
connect_rc_codes: dict = dataclasses.field(default_factory=dict)
"""Value->count dictionary of rc codes returned from the `connect` method"""
Copy link
Member

@cartertinney cartertinney Nov 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is.... super nitpicky... (but also kind of not) but you aren't supposed to use """ to make comments anywhere except a docstring. As I recall, they are interpreted differently by the runtime, because the """ is a string literal, wheras a # comment just gets completely ignored.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, not nitpicky. These string literals end up taking space at runtime, so the distinction is important. I did it this way on purpose. Even though docstrings on attributes are technically not part of the Python standard, some tools will pick these up (if you put them after the attribute definition). I think PyCharm will pick these up, and maybe some versions of Sphynx.

"""
config = PahoConfig()

config.transport = self._mqtt_client._transport
Copy link
Member

@cartertinney cartertinney Nov 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel as though accessing convention-private attributes of Paho is dangerous. There is no guarantee they don't change in an update to Paho, which could break anyone using our library until we make an update. Furthermore, that risk doesn't really seem worth it for a logging utility.

I could be convinced of the utility if we were to build some exception handling in to protect against this possibility, but even still, seems dicey.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it's dangerous. I also think it's super useful -- at least some of these are. Using getattr() with a default would be much safer.



def disconnect_output_port(disconnect_type, transport, host):
def disconnect_output_port(disconnect_type, transport, host=None):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great change. Was thinking of doing this myself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants