Conversation
…into iot_metrics
This reverts commit 4bf6518.
| /* Error */ | ||
| int m_lastError; | ||
|
|
||
| /** Enable AWS IoT Metrics Collection. This is always set to true for now. */ |
There was a problem hiding this comment.
Trivial: Maybe it'll become clearer below but what does this mean it's always set to true? Is this not tied to the enableMetrics bools that can be set to false?
There was a problem hiding this comment.
It seems like this can't be changed to false. Is there a reason we're not allowing metrics to be disabled?
There was a problem hiding this comment.
This option was originally there to match MQTT3 behavior, which allows users to disable metrics. Since the MQTT5 client does not supports toggling metrics collection, it should be safe to remove.
sbSteveK
left a comment
sbSteveK
left a comment
There was a problem hiding this comment.
We need to add what enable metrics defaults to in some locations and add tests that turning them off results in an unpadded username.
| Crt::String m_sdkName = "CPPv2"; | ||
| Crt::String m_sdkVersion = AWS_CRT_CPP_VERSION; |
There was a problem hiding this comment.
This part sill confuses me. Will the library name sometimes be set to CPPv2 (if mqtt client created with builder) and sometimes to IoTDeviceSDK/CPP (if created without builder) ? Can we change the default value of m_sdkName to IoTDeviceSDK/CPP ?
There was a problem hiding this comment.
As mentioned here, the SDK already exposes an API for setting a customized sdkName.
As we will introduce metadata later, once metadata is there, we will update the SDKs in a single change.
There was a problem hiding this comment.
Yeah, I understand that we can't remove this. But we can change the default value. Or am I missing something?
| aws_string_destroy(password); | ||
| return AWS_OP_SUCCESS; | ||
| } | ||
| AWS_TEST_CASE(Mqtt311DirectConnectionWithMetricsCollection, s_TestMqtt311DirectConnectionWithMetricsCollection) |
There was a problem hiding this comment.
This test should be added to tests/CMakeLists.txt
| /** | ||
| * Enable AWS IoT metrics. Default to enabled. | ||
| * | ||
| * @param enabled enable AWS IoT metrics to collect SDK usage data | ||
| * @return this option object | ||
| */ | ||
| Mqtt5ClientOptions &WithMetricsCollection(bool enabled) noexcept; |
There was a problem hiding this comment.
I think we should provide more details to users about what info we collect (version, configuration, etc.) and what we don't (probably something general like "private data" would suffice), so users won't opt out of metrics just in case we collect something they don't want to share.
I'd add these details to all public API that expose metrics-related functionality.
| struct aws_mqtt_iot_metrics metrics; | ||
| AWS_ZERO_STRUCT(metrics); | ||
| m_sdkMetrics.initializeRawOptions(metrics); | ||
| if (aws_mqtt_client_connection_set_metrics(m_underlyingConnection, &metrics)) |
There was a problem hiding this comment.
Will it be possible to add meta info into metrics later on? For example, afaiks the cleanSession flag is not available at this point, so we'll have to add it to metrics at the connection time.
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.