Add IoT Metrics Support #710
Conversation
| if (!s_set_metrics(py_connection->native, metrics_py)) { | ||
| goto done; |
There was a problem hiding this comment.
In crt-cpp, a failure on setting metrics is ignored. Here, it fails connection attempts. I think we should document the correct behavior and implement it everywhere.
There was a problem hiding this comment.
There is still inconsistency on setting metrics failure between crt-cpp (just log failure and proceed with connection) and crt-python (fail connect attempt on failure). I think the cpp logic is the correct one because we don't want the metrics-related issues to break users.
There was a problem hiding this comment.
Let me clarify the current state: The reason crt-cpp doesn't fail the connection is that it lacks a proper mechanism to propagate the configuration failure. In the other three SDKs (Python, Java, Node.js), I implemented it to fail the connection attempt when setting metrics fails.
However, I think you raise a valid point. Since metrics are set internally by the SDK (not explicitly requested by the user), we probably shouldn't let metrics-related failures break the user's connection. The core functionality should remain working even if the optional metrics feature encounters issues. I will update it to keep it consistent with cpp.
There was a problem hiding this comment.
I'm in agreement with the decision here. Let's not fail a connection attempt with anything related to metrics. At most we should log the issue but move on. All behavior should be identical across our SDKs.
| 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...
| proxy_options (Optional[awscrt.http.HttpProxyOptions]): | ||
| Optional proxy options for all connections. | ||
|
|
||
| enable_metrics (bool): If true, enable IoT SDK metrics in CONNECT packet username field, otherwise, disabled. |
There was a problem hiding this comment.
Trivial: I think we should work on the wording here. The initial sentence makes it sound like the default is false. Instead of the "If true" maybe we should just describe what the bool is for. "Enable or disable IoT SDK metrics in CONNECT packet username field." Also wondering if we should add a bit of extra text regarding why but that's debatable. e.g. "Our team uses these metrics to help determine what features and improvements to prioritize." We can leave the "Default to True" but I would remove the follow-up that provides a reason to set it to false.
| @dataclass | ||
| class SdkMetrics: | ||
| """ | ||
| Configuration for IoT SDK metrics that are embedded in MQTT username field. |
There was a problem hiding this comment.
Trivial/debatable: MQTT Connect Packet username field ?
| on_lifecycle_event_connection_success_fn (Callable[[LifecycleConnectSuccessData],]): Callback for Lifecycle Event Connection Success. | ||
| on_lifecycle_event_connection_failure_fn (Callable[[LifecycleConnectFailureData],]): Callback for Lifecycle Event Connection Failure. | ||
| on_lifecycle_event_disconnection_fn (Callable[[LifecycleDisconnectData],]): Callback for Lifecycle Event Disconnection. | ||
| enable_metrics (bool): If true, enable IoT SDK metrics in CONNECT packet username field, otherwise, disabled. Default to True. You may set it to false if you are not using AWS IoT services, and using a custom authentication mechanism. |
There was a problem hiding this comment.
Same as comment above in regards to rewording.
| /* # */ &will_content_type.len, | ||
| /* O */ &will_user_properties_py, | ||
|
|
||
| /* Metrics */ |
There was a problem hiding this comment.
Trivial: Why are we injecting the metrics into the middle here instead of adding it to the end? I feel like this has a good potential to need expansion as we add more metrics in which case it'd be easier/cleaner to add to the end instead of tracking down and continuing to add in the middle.
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.