Metric update#735
Conversation
rakshil14-2
force-pushed
the
metric_update
branch
from
ed2b3c8 to
0356f48
Compare
rakshil14-2
force-pushed
the
metric_update
branch
from
316db8e to
dffd72b
Compare
Update submodule added function to track features lint making public module fix CI errors Added TLS feature at CRT layer lint making aws_iot_metric public updating doc fix docs error Introduce Opt-out for Metrics updating doc_string updating submodules Test cases changing public attributes to private changed to public attributes
rakshil14-2
force-pushed
the
metric_update
branch
from
dffd72b to
35bef38
Compare
| # Feature ID Constants | ||
|
|
||
|
|
||
| class MetricsFeatureId(str, Enum): |
There was a problem hiding this comment.
I think we should keep the ids private. I didn't see why end user would need to use or know about the metrics value. Similar for the other encoding value classes.
|
|
||
| # Merge features: if version matches, merge CRT + SDK; otherwise CRT only | ||
| if (user_metrics_version is not None and user_metrics_version.isdigit() and int( | ||
| user_metrics_version) == IOT_SDK_METRICS_FEATURE_VERSION and user_feature): |
There was a problem hiding this comment.
It seems unnecessary to validate user_features, as the default user_feature will be "".
| return final_metrics | ||
|
|
||
|
|
||
| def create_metrics_mqtt5(client_options): |
There was a problem hiding this comment.
Similarly, create_metrics should be a private APIs.
| if "/" in pair: | ||
| fid, val = pair.split("/", 1) | ||
| merged[fid] = val | ||
| return ",".join(f"{k}/{v}" for k, v in sorted(merged.items())) |
There was a problem hiding this comment.
I realize we also sorted the feature list in Swift, however, thinking about it, it seems unnecessary to sort the feature list.
| options (TlsContextOptions): Configuration options. | ||
| """ | ||
| __slots__ = () | ||
| __slots__ = ('min_tls_ver', 'cipher_pref') |
There was a problem hiding this comment.
As the members are for metrics tracking only, they should not be publicly accessible. Or it should at least be read-only.
Swift made the same mistake.
| self.proxy_options = proxy_options if proxy_options else websocket_proxy_options | ||
| if enable_metrics: | ||
| self._metrics = AWSIoTMetrics() | ||
| if not disable_metrics: |
There was a problem hiding this comment.
Nit: Here is a double negative (not disable_metrics), consider flipping the condition for readability.
| # Handle metrics configuration | ||
| if client_options.enable_metrics: | ||
| self._metrics = AWSIoTMetrics() | ||
| if not client_options.disable_metrics: |
There was a problem hiding this comment.
Nit: Similar, double negative (not disable_metrics) here.
| if (PyObject_IsTrue(is_metrics_enabled_py)) { | ||
| metrics_tmp.library_name = metrics_library_name; | ||
|
|
||
| if (PyObject_IsTrue(is_metrics_enabled_py) && metrics_py != Py_None) { |
There was a problem hiding this comment.
Could we have a helper function to handle metrics object, and reuse it across mqtt5 and mqtt3?
2 participants
Issue
Description of changes:
AWSIoTMetricsclass as public. This class will hold all information regarding metrics.enableMetrics->disableMetrics: change name convention to disable to focus on the "opt-out" pattern.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.