Conversation
awscrt/mqtt.py
Outdated
| from awscrt.io import ClientBootstrap, ClientTlsContext, SocketOptions | ||
| from dataclasses import dataclass | ||
| from awscrt.mqtt5 import Client as Mqtt5Client | ||
| from awscrt.mqtt5 import Client as Mqtt5Client, SdkMetrics |
There was a problem hiding this comment.
Instead of pulling SdkMetrics in from mqtt5, should we instead implement an mqtt3 version of SdkMetrics? It appears there's precedence for this with the double implementation of OperationStatisticsData. Unsure if this is a good or bad practice but it's one we've previously used. This could also potentially allow us to set different things by default for mqtt3 vs. mqtt5 or it could add confusion and a second place to update things when we make changes...
There was a problem hiding this comment.
I'd prefer not to duplicate the structure across MQTT3 and MQTT5. Maintaining it in two places increases the risk of them going out of sync over time. I realize that's the pattern we used in Python, but I wouldn't consider it a good practice to follow.
If we ever need different defaults for MQTT3 vs. MQTT5, we can always inherit from the base structure at that point.
There was a problem hiding this comment.
We could add a separate module, like we did with req-resp client. With meta data implemented, the metrics functionality should be big enough to be in its own module.
| self.keep_alive_secs: int = 1200 if keep_alive_secs is None else keep_alive_secs | ||
| self.ack_timeout_secs: int = 0 if ack_timeout_secs is None else ack_timeout_secs | ||
| self.clean_session: bool = True if clean_session is None else clean_session | ||
| self.enable_metrics: bool = True if enable_metrics is None else enable_metrics |
There was a problem hiding this comment.
Trivial: I think at this point it's impossible for enable_metrics to be None (Always defaults to true at creation) and can be set to enable_metrics.
sbSteveK
left a comment
sbSteveK
left a comment
There was a problem hiding this comment.
Seems solid. It looks like MQTT3 and MQTT5 will have to go through different binding paths when metrics is expanded. Would be ideal to use the same for both but unsure if that's possible without changes underneath?
Unfortunately, I don't think we can change the binding path for MQTT3 and MQTT5 without changes underneath... |
3 participants
Issue #, if available:
Description of changes:
IoTDeviceSDKMetricsand metrics related features , CRT is now appending AWS metrics by default.enableMetricsoption to allow user disable metrics.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.