RDK-60621 - Data Collection Through T2 Events For Components Logging To btmgrlog.txt#42
RDK-60621 - Data Collection Through T2 Events For Components Logging To btmgrlog.txt#42ANANTHMARIMUTHU wants to merge 47 commits intodevelopfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a small telemetry wrapper (T2) and adds telemetry markers/events across Bluetooth Core and the BlueZ5 GDBus interface to report key pairing, discovery, adapter, and thread-creation failures.
Changes:
- Added
bt-telemetry.{c,h}wrapper aroundt2_init()/t2_event_*()and exposed it as an installed header. - Instrumented
btrCore.candbtrCore_gdbus_bluez5.cwith new telemetry markers and split-string events. - Updated Automake build files to compile/link the telemetry wrapper and telemetry sender library.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/btrCore.c |
Adds telemetry events for pairing failure, unsupported device detection, adapter path failure, pairing failure, and battery thread creation failure. |
src/bt-telemetry.c |
New implementation of telemetry wrapper functions calling T2 APIs and logging failures. |
include/telemetry/bt-telemetry.h |
New public header for telemetry wrapper APIs. |
src/bt-ifce/btrCore_gdbus_bluez5.c |
Adds telemetry for paired/connected state and discovery start/stop failures. |
src/Makefile.am |
Adds telemetry include path, compiles bt-telemetry.c into libbtrCore, links telemetry sender lib, installs header. |
src/bt-ifce/Makefile.am |
Links telemetry sender lib to libbtrCoreIfce. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
include/telemetry/bt-telemetry.h
Outdated
| * | ||
| * @param name Component name for telemetry identification | ||
| */ | ||
| void telemetry_init(const char* name); |
There was a problem hiding this comment.
The telemetry_init function is defined and exposed in the public API but is never called anywhere in the codebase. According to telemetry best practices, the t2_init function should be called once during component initialization (typically in BTRCore_Init) to properly register the component with the telemetry system. Without initialization, telemetry events may not be properly attributed to this component. Consider calling telemetry_init with an appropriate component name during BTRCore initialization.
src/bt-telemetry.c
Outdated
| void telemetry_init(const char* name) | ||
| { | ||
| if (name == NULL) { | ||
| BTRCORELOG_ERROR("T2: Failed to initialize telemetry - component name is NULL\n"); | ||
| return; | ||
| } | ||
| BTRCORELOG_INFO("T2: Initializing telemetry with component name=\"%s\"\n", name); | ||
| t2_init(name); | ||
| } | ||
|
|
||
| /** | ||
| * @brief Send marker with string value to T2 | ||
| */ | ||
| void telemetry_event_s(const char* marker, char* value) | ||
| { | ||
| if (marker == NULL) { | ||
| BTRCORELOG_ERROR("T2: telemetry_event_s - marker is NULL\n"); | ||
| return; | ||
| } | ||
| if (value == NULL) { | ||
| BTRCORELOG_ERROR("T2: telemetry_event_s - value is NULL\n"); | ||
| return; | ||
| } | ||
| T2ERROR t2error = t2_event_s(marker, value); | ||
| if (t2error != T2ERROR_SUCCESS) { | ||
| BTRCORELOG_ERROR("t2_event_s(\"%s\", \"%s\") returned error code %d\n", marker, value, t2error); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * @brief Send marker with integer value to T2 or report count based markers | ||
| */ | ||
| void telemetry_event_d(const char* marker, int value) | ||
| { | ||
| if (marker == NULL) { | ||
| BTRCORELOG_ERROR("T2: telemetry_event_d - marker is NULL\n"); | ||
| return; | ||
| } | ||
| T2ERROR t2error = t2_event_d(marker, value); | ||
| if (t2error != T2ERROR_SUCCESS) { | ||
| BTRCORELOG_ERROR("t2_event_d(\"%s\", %d) returned error code %d\n", marker, value, t2error); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * @brief Send marker with double value to T2 | ||
| */ | ||
| void telemetry_event_f(const char* marker, double value) | ||
| { | ||
| if (marker == NULL) { | ||
| BTRCORELOG_ERROR("T2: telemetry_event_f - marker is NULL\n"); | ||
| return; | ||
| } | ||
| T2ERROR t2error = t2_event_f(marker, value); | ||
| if (t2error != T2ERROR_SUCCESS) { | ||
| BTRCORELOG_ERROR("t2_event_f(\"%s\", %f) returned error code %d\n", marker, value, t2error); | ||
| } | ||
| } |
There was a problem hiding this comment.
The new telemetry wrapper functions (telemetry_init, telemetry_event_s, telemetry_event_d, telemetry_event_f) lack unit test coverage. Since the codebase has comprehensive unit tests for other components, these new functions should also have tests that verify null parameter handling, error reporting for failed t2_event calls, and proper forwarding to the underlying telemetry library.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| AC_MSG_ERROR([telemetry library (libtelemetry_msgsender) not found])) | ||
| if test "x$telemetry_msgsender_ok" = "xyes"; then | ||
| TELEMETRY_MSGLIBS="-ltelemetry_msgsender" | ||
| TELEMETRY_CFLAGS="-DHAVE_TELEMETRY_MSGSENDER" |
There was a problem hiding this comment.
The indentation of TELEMETRY_CFLAGS is inconsistent with the previous line. It should align with the TELEMETRY_MSGLIBS assignment above it.
| TELEMETRY_CFLAGS="-DHAVE_TELEMETRY_MSGSENDER" | |
| TELEMETRY_CFLAGS="-DHAVE_TELEMETRY_MSGSENDER" |
| * @param marker Telemetry marker/event name (use _split suffix for split markers) | ||
| * @param value String value to send | ||
| */ | ||
| void telemetry_event_s(const char* marker, char* value); |
There was a problem hiding this comment.
The value parameter should be const char* for consistency with the marker parameter and to indicate that the string is not modified by the function. However, if the underlying t2_event_s API requires a non-const char*, this may be unavoidable. If that's the case, consider adding a comment explaining why the parameter cannot be const.
| void telemetry_event_s(const char* marker, char* value); | |
| void telemetry_event_s(const char* marker, const char* value); |
ANANTHMARIMUTHU
changed the title
No description provided.