-
Notifications
You must be signed in to change notification settings - Fork 50
Add IoT Metrics Support #710
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| if (!s_set_metrics(py_connection->native, metrics_py)) { | ||
| goto done; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.