Conversation
…into iot_metrics
This reverts commit 4bf6518.
| Mqtt5::Mqtt5ClientOptions mqtt5Options(allocator); | ||
| mqtt5Options.WithHostName(mqtt5TestVars.m_hostname_string); | ||
| mqtt5Options.WithPort(mqtt5TestVars.m_port_value); | ||
| mqtt5Options.WithMetricsCollection(false); |
There was a problem hiding this comment.
Are we testing that setting this to false is preventing metrics from being added? Also, I'm not seeing a test that is checking that metrics ARE being added neither. Maybe I'm missing something.
| /* 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.
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.