-
Notifications
You must be signed in to change notification settings - Fork 37
Add Platform Metrics Support For Mqtt3 Client #412
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
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #412 +/- ##
==========================================
- Coverage 84.73% 84.71% -0.03%
==========================================
Files 25 26 +1
Lines 10508 10683 +175
==========================================
+ Hits 8904 9050 +146
- Misses 1604 1633 +29 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
source/client.c
Outdated
|
|
||
| if (connection->username) { | ||
| struct aws_byte_cursor username_cur = aws_byte_cursor_from_string(connection->username); | ||
| if (connection->username || connection->metrics) { |
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.
Not a fan of this conditional complexity. We don't need to persist the username buffer in the connection object, we can just make a local buffer and free it after initing the packet. The rest of the logic could just be internalized in a metrics API that takes (username, metrics, output_buffer) which you already have I think.
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.
I removed the persist final username buffer and use a local one, while I didnt see much I can do with the conditional complexity here.
source/mqtt_iot_sdk_metrics.c
Outdated
| } | ||
|
|
||
| struct aws_byte_cursor sdk_attr_value = | ||
| metrics.library_name.len > 0 ? metrics.library_name : aws_byte_cursor_from_c_str("IoTDeviceSDK/C"); |
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.
Do we want to know the version # of aws-c-mqtt?
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.
Currently, I plan to put the version into metadata list, in a format of Metadata=(aws-c-mqtt=X.Y.Z, aws-crt-cpp=A.B.C), so that we could collect version numbers for all stacks.
source/mqtt_iot_sdk_metrics.c
Outdated
|
|
||
| /* Add Platform if not present */ | ||
| if (!has_platform) { | ||
| struct aws_byte_cursor platform_cursor = aws_get_platform_build_os_string(); |
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.
I don't like this function at all. I'd much rather see an append platform string function that could be dynamic (ie include version and other dynamic information) rather than something that's just a handful of static strings.
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.
I updated the function so it build username from flexible query params, which user would have ability to pass in dynamic information.
While I didn't see how we could avoid having the static string, as long as we would like to append the platform string by default.
source/mqtt_iot_sdk_metrics.c
Outdated
| if (s_build_username_query( | ||
| original_username, base_username_length, ¶ms_list, output_username, out_full_username_size)) { |
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.
Restoring the username from params may slightly change it (e.g. ?a&b becomes ?a=&b=). I don't think it'll cause any issues, but we still should be aware of this.
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.
Nice catch, updated s_build_username_query to handle the case.
While I figured that this is a behavior change. As we didn't have any restriction on the username, while now we start to treat the username as query string.
In a way, this should be fine, as the service side already treated the username as a query string, and use QueryStringDecoder to handle the username. It should build the same string we build here regardless. While, I will bring this to the team for further discussion.
| .ptr = NULL, | ||
| .len = 0, | ||
| }; | ||
| if (aws_byte_cursor_is_valid(&username_cur)) { |
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.
Under what conditions would this not be valid? Does it make sense to keep going?
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.
This is unlikely to happen, but the only scenario I can think of is when the username is not set, and the metrics string exceeds the size limit, resulting in an invalid username.
I believe it's still acceptable to continue the connection, as metrics are not mandatory for a successful connection.
Issue #, if available:
Description of changes:
aws_mqtt_client_connection_set_metricsto set metrics for mqtt3.SDK name field
For the SDK name field, the library will use the format
IoTDeviceSDK/[Language]. And the value would be unified across the CRT/SDKV2/SDKV1 for the same language. e.x.: aws-crt-cpp/aws-iot-device-sdk-cpp/aws-iot-deivce-sdk-cpp-v2 will all be tracked asIoTDeviceSDK/CPP. The binding layer is responsible for setting the SDK name field. If not set, the name will default to beIoTDeviceSDK/C, indicating the client is using aws-c-mqtt library.Username encoding
IoT gateway team mentioned they are follow the query string format for username field. ( More specific, they used QueryStringDecoder API to decode the string. ) Beyond that, the username should follow the standard MQTT specs.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.