Conversation
Summary: Integrating telemetry T2 markers into Rialto Type: Fix Test Plan: UT/CT, Fullstack Jira: RDKEMW-16516
Summary: Integrating telemetry T2 markers into Rialto Server Manager
Type: Fix
Test Plan: UT/CT, Fullstack
Jira: RDKEMW-16519
Summary: Integrating telemetry T2 markers into Rialto Server
Type: Fix
Test Plan: UT/CT, Fullstack
Jira: RDKEMW-16518
|
Pull request must be merged with a description containing the required fields, Summary: If there is no jira releated to this change, please put 'Jira: NO-JIRA'. Description can be changed by editing the top comment on your pull request and making a new commit. |
There was a problem hiding this comment.
Pull request overview
Integrates Telemetry 2.0 (T2) markers across Rialto Server Manager, media server, and media client code paths to emit additional runtime diagnostics (primarily on error/failure scenarios).
Changes:
- Added a telemetry wrapper header (
RialtoTelemetry.h) and wired it into the logging library/build. - Emitted
TELEMETRY_EVENT_STRINGmarkers across many failure paths in Server Manager, media server, and media client components; addedTELEMETRY_INITin the media server service entrypoint. - Updated a unit test expectation to account for additional
ErrorText()access.
Reviewed changes
Copilot reviewed 54 out of 54 changed files in this pull request and generated 21 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unittests/media/client/ipc/ipcModuleBase/IpcModuleBase.cpp | Adjusted mock expectations for additional controller ErrorText() usage. |
| serverManager/ipc/source/Client.cpp | Added telemetry marker on unexpected disconnect/recovery path. |
| serverManager/common/source/SessionServerAppManager.cpp | Added telemetry markers for connection/state/configuration failures. |
| serverManager/common/source/HealthcheckService.cpp | Added telemetry marker when ping failure threshold triggers recovery. |
| media/server/service/source/main.cpp | Included telemetry header and added TELEMETRY_INIT for the server process. |
| media/server/service/source/WebAudioPlayerService.cpp | Added telemetry markers for WebAudioPlayer service failure paths. |
| media/server/service/source/SessionServerManager.cpp | Added telemetry markers for init/config/state failure paths. |
| media/server/service/source/MediaPipelineService.cpp | Added telemetry markers for MediaPipeline service failure paths. |
| media/server/service/source/CdmService.cpp | Added telemetry markers for CDM service failure paths. |
| media/server/main/source/WebAudioPlayerServerInternal.cpp | Added telemetry markers for internal WebAudio failures/exceptions. |
| media/server/main/source/TextTrackSession.cpp | Added telemetry marker when TextTrack session creation fails. |
| media/server/main/source/TextTrackAccessor.cpp | Added telemetry markers for TextTrack control/session operation failures. |
| media/server/main/source/SharedMemoryBuffer.cpp | Added telemetry marker on shared memory init failure. |
| media/server/main/source/MediaPipelineServerInternal.cpp | Added telemetry markers for shm mapping / player / source-position failures. |
| media/server/main/source/MediaKeysServerInternal.cpp | Added telemetry markers for MediaKeys internal failures/exceptions. |
| media/server/main/source/MediaKeysCapabilities.cpp | Added telemetry markers for MediaKeysCapabilities factory/get failures. |
| media/server/main/source/MediaKeySession.cpp | Added telemetry markers for MediaKeySession creation and operation failures. |
| media/server/main/source/DataReaderV2.cpp | Added telemetry markers for metadata/segment parsing failures. |
| media/server/ipc/source/WebAudioPlayerModuleService.cpp | Added telemetry markers for incompatible controller handling and factory creation failures. |
| media/server/ipc/source/SessionManagementServer.cpp | Added telemetry markers for IPC server initialization failures. |
| media/server/ipc/source/ServerManagerModuleService.cpp | Added telemetry marker for incompatible controller handling. |
| media/server/ipc/source/MediaPipelineModuleService.cpp | Added telemetry markers for incompatible controller handling. |
| media/server/ipc/source/MediaPipelineCapabilitiesModuleService.cpp | Added telemetry markers for incompatible controller handling. |
| media/server/ipc/source/MediaKeysModuleService.cpp | Added telemetry markers for factory creation failures and incompatible controller handling. |
| media/server/ipc/source/MediaKeysCapabilitiesModuleService.cpp | Added telemetry markers for factory creation failures and incompatible controller handling. |
| media/server/ipc/source/ControlModuleService.cpp | Added telemetry markers for incompatible controller and schema incompatibility handling. |
| media/server/ipc/source/ApplicationManagementServer.cpp | Added telemetry markers for IPC server/client initialization failures. |
| media/server/gstplayer/source/tasks/webAudio/HandleBusMessage.cpp | Added telemetry marker on GStreamer bus error handling (WebAudio). |
| media/server/gstplayer/source/tasks/generic/SwitchSource.cpp | Added telemetry marker on source switch failure. |
| media/server/gstplayer/source/tasks/generic/SetSourcePosition.cpp | Added telemetry markers for invalid source-type/source-null cases. |
| media/server/gstplayer/source/tasks/generic/SetPosition.cpp | Added telemetry markers for seek failures (null client/pipeline/gst error). |
| media/server/gstplayer/source/tasks/generic/ReadShmDataAndAttachSamples.cpp | Added telemetry markers when attaching samples fails (exceptions). |
| media/server/gstplayer/source/tasks/generic/HandleBusMessage.cpp | Added telemetry marker on GStreamer bus error handling (generic). |
| media/server/gstplayer/source/tasks/generic/AttachSource.cpp | Added telemetry marker when caps creation fails. |
| media/server/gstplayer/source/tasks/generic/AttachSamples.cpp | Added telemetry markers when attaching samples fails (exceptions). |
| media/server/gstplayer/source/GstWebAudioPlayer.cpp | Added telemetry markers for pipeline/appsrc creation and state-change failures. |
| media/server/gstplayer/source/GstGenericPlayer.cpp | Added telemetry markers for cast failure, metadata failure, and pipeline state-change failures. |
| media/client/main/source/WebAudioPlayer.cpp | Added telemetry markers on client factory/player creation exceptions. |
| media/client/main/source/MediaPipelineCapabilities.cpp | Added telemetry markers on client factory/capabilities creation exceptions. |
| media/client/main/source/MediaKeysCapabilities.cpp | Added telemetry markers on client capabilities creation exceptions. |
| media/client/main/source/MediaKeys.cpp | Added telemetry markers on client MediaKeys creation exceptions. |
| media/client/main/source/ClientController.cpp | Added telemetry markers for shared memory init/get failures. |
| media/client/ipc/source/WebAudioPlayerIpc.cpp | Added telemetry markers for client IPC factory creation failures and RPC failures. |
| media/client/ipc/source/MediaPipelineIpc.cpp | Added telemetry markers for client MediaPipeline IPC RPC failures. |
| media/client/ipc/source/MediaPipelineCapabilitiesIpc.cpp | Added telemetry markers for client capabilities IPC RPC failures/exceptions. |
| media/client/ipc/source/MediaKeysIpc.cpp | Added telemetry markers for client MediaKeys IPC failures, including status conversion failures. |
| media/client/ipc/source/MediaKeysCapabilitiesIpc.cpp | Added telemetry markers for client capabilities IPC failures/exceptions. |
| media/client/ipc/source/IpcClient.cpp | Added telemetry marker on unexpected IPC disconnect. |
| media/client/ipc/source/ControlIpc.cpp | Added telemetry markers for RPC failures and schema incompatibility. |
| media/client/common/include/RialtoClientLogging.h | Included <cstdio> to support new snprintf usage in client code paths. |
| logging/include/RialtoTelemetry.h | Added telemetry wrapper macros with stub mode when telemetry is disabled. |
| logging/include/RialtoLogging.h | Included telemetry wrapper header from core logging header. |
| logging/CMakeLists.txt | Added telemetry build wiring and compile-definition gating for the logging library. |
| cmake/Findtelemetry.cmake | Added a CMake find-module for the telemetry library/headers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| char telemetryBuff[128] = {0}; | ||
| snprintf(telemetryBuff, sizeof(telemetryBuff), | ||
| "Rialto App Management socket is not a number."); | ||
| TELEMETRY_EVENT_STRING("Rialto Server - SessionServerManager", telemetryBuff); |
There was a problem hiding this comment.
snprintf is used here, but this translation unit does not include <cstdio>/<stdio.h> (it currently only includes standard headers like <algorithm>, <stdexcept>, etc.). This will fail to compile on strict toolchains; add the appropriate header (or provide snprintf via a common header if this pattern is intended across the codebase).
| char telemetryBuff[128] = {0}; | ||
| snprintf(telemetryBuff, sizeof(telemetryBuff), | ||
| "Failed to create the media keys capabilities module service factory, reason: %s", e.what()); | ||
| TELEMETRY_EVENT_STRING("Rialto Client - MediaKeysCapabilitiesModuleService", telemetryBuff); | ||
| } |
There was a problem hiding this comment.
These telemetry events are emitted from the server IPC capabilities module service, but the marker string starts with "Rialto Client - ...". This will skew telemetry attribution; update the marker to the server-side prefix/name.
| target_compile_definitions(RialtoLogging PUBLIC RIALTO_TELEMETRY_SUPPORT=1) | ||
| target_include_directories(RialtoLogging PUBLIC ${TELEMETRY_INCLUDE_DIRS}) | ||
| target_link_libraries(RialtoLogging PUBLIC ${TELEMETRY_LIBRARIES}) |
There was a problem hiding this comment.
In the EthanLog_FOUND branch, the telemetry configuration is duplicated and is applied to RialtoLogging (not RialtoEthanLog). This likely leaves RialtoEthanLog without RIALTO_TELEMETRY_SUPPORT/telemetry link deps and also duplicates status output; apply the telemetry target_* calls to RialtoEthanLog or factor the configuration into a shared helper.
| target_compile_definitions(RialtoLogging PUBLIC RIALTO_TELEMETRY_SUPPORT=1) | |
| target_include_directories(RialtoLogging PUBLIC ${TELEMETRY_INCLUDE_DIRS}) | |
| target_link_libraries(RialtoLogging PUBLIC ${TELEMETRY_LIBRARIES}) | |
| target_compile_definitions(RialtoEthanLog PUBLIC RIALTO_TELEMETRY_SUPPORT=1) | |
| target_include_directories(RialtoEthanLog PUBLIC ${TELEMETRY_INCLUDE_DIRS}) | |
| target_link_libraries(RialtoEthanLog PUBLIC ${TELEMETRY_LIBRARIES}) |
| RIALTO_CLIENT_LOG_ERROR("failed to ack due to '%s'", ipcController->ErrorText().c_str()); | ||
| char telemetryBuff[128] = {0}; | ||
| snprintf(telemetryBuff, sizeof(telemetryBuff), "Failed to ack due to '%s'", ipcController->ErrorText().c_str()); | ||
| TELEMETRY_EVENT_STRING("Rialto Client - ControllIpc", telemetryBuff); |
There was a problem hiding this comment.
Telemetry marker string typo repeated here: "ControllIpc" should be "ControlIpc" to keep telemetry naming consistent.
| TELEMETRY_EVENT_STRING("Rialto Client - ControllIpc", telemetryBuff); | |
| TELEMETRY_EVENT_STRING("Rialto Client - ControlIpc", telemetryBuff); |
| char telemetryBuff[128] = {0}; | ||
| snprintf(telemetryBuff, sizeof(telemetryBuff), | ||
| "Failed to create the media keys capabilities factory, reason: %s", e.what()); | ||
| TELEMETRY_EVENT_STRING("Rialto Client - MediaKeysCapabilities", telemetryBuff); | ||
| } |
There was a problem hiding this comment.
The telemetry marker uses "Rialto Client - MediaKeysCapabilities", but this code is in the server component (media/server/...) and logs with RIALTO_SERVER_LOG_ERROR. This mislabels telemetry and will make dashboards misleading; please change the marker prefix to Rialto Server (or whatever the standard server marker is).
| #define TELEMETRY_EVENT_STRING(marker, value) \ | ||
| do \ | ||
| { \ | ||
| t2_event_s(reinterpret_cast<char *>(marker), reinterpret_cast<char *>(value)); \ | ||
| } while (0) |
There was a problem hiding this comment.
TELEMETRY_EVENT_STRING casts marker/value to char* with reinterpret_cast, which discards constness and can be undefined behavior. Use const_cast<char*> at most, or update the wrapper to accept const char* so callers don’t need to worry about mutability.
| if(CMAKE_TELEMETRY_2_0_REQUIRED) | ||
| message(STATUS "Telemetry 2.0 support enabled") | ||
| target_compile_definitions(RialtoLogging PUBLIC RIALTO_TELEMETRY_SUPPORT=1) | ||
| target_include_directories(RialtoLogging PUBLIC ${TELEMETRY_INCLUDE_DIRS}) | ||
| target_link_libraries(RialtoLogging PUBLIC ${TELEMETRY_LIBRARIES}) |
There was a problem hiding this comment.
TELEMETRY_INCLUDE_DIRS / TELEMETRY_LIBRARIES are used here, but there is no find_package(telemetry) (or equivalent include of Findtelemetry.cmake) anywhere in the repo, so these variables will be undefined and telemetry linking will not work when CMAKE_TELEMETRY_2_0_REQUIRED is enabled. Please ensure the telemetry find-module is invoked before these target_* calls.
| RIALTO_SERVER_LOG_ERROR("Failed to create the media keys capabilities module service, reason: %s", e.what()); | ||
| char telemetryBuff[128] = {0}; | ||
| snprintf(telemetryBuff, sizeof(telemetryBuff), | ||
| "Failed to create the media keys capabilities module service, reason: %s", e.what()); | ||
| TELEMETRY_EVENT_STRING("Rialto Client - MediaKeysCapabilitiesModuleService", telemetryBuff); |
There was a problem hiding this comment.
Second occurrence of the same issue as earlier: this server-side factory failure is reported with a "Rialto Client - MediaKeysCapabilitiesModuleService" telemetry marker. Please change it to a server-side marker for correct attribution.
| char telemetryBuff[128] = {0}; | ||
| snprintf(telemetryBuff, sizeof(telemetryBuff), | ||
| "Failed to create the web audio player, reason: %s", e.what()); | ||
| TELEMETRY_EVENT_STRING("Rialto Server - WebAudioPlayer", telemetryBuff); | ||
| } |
There was a problem hiding this comment.
Second occurrence of the same issue as earlier: this is client-side code but the telemetry marker uses "Rialto Server - WebAudioPlayer". Please change it to a client-side marker consistently across the file.
| snprintf(telemetryBuff, sizeof(telemetryBuff), | ||
| "Server and Client proto schema versions are not compatible. Server schema version: " | ||
| "%s, Client schema version: %s", | ||
| kServerSchemaVersion.str().c_str(), kCurrentSchemaVersion.str().c_str()); | ||
| TELEMETRY_EVENT_STRING("Rialto Client - ControllIpc", telemetryBuff); |
There was a problem hiding this comment.
Telemetry marker string typo repeated here: "ControllIpc" should be "ControlIpc" to keep telemetry naming consistent.
Summary: Integrating telemetry T2 markers into Rialto Server
Type: Fix
Test Plan: UT/CT, Fullstack
Jira: RDKEMW-16518