Conversation
Reason for change: provider component- tr69hostif acquires TraceParent & TraceContext using OTEL and Sets to RBUS> The provider component, calls RPC operations like EventPublish The consumer/subscriber callback receives the same TraceParent & TraceContext in RRD through RRD_SET_ISSUE_EVENT event Test Procedure: Refer the ticket descriptions Risks: Medium Priority: P0 Signed-off-by: Thamim Razith Abbas Ali <tabbas651@comcast.com>
Code Coverage Summary |
There was a problem hiding this comment.
🟡 Changes recommended
The new OTLP instrumentation introduces unit-test build/link breakage risk and trace-context propagation is not made atomic against concurrent RBUS usage of the shared global handle.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Enables OpenTelemetry distributed trace context propagation across RBUS by initializing OTLP instrumentation in the RBUS DML provider and attaching/clearing RBUS trace context around an event publish in the DeviceInfo remote-debugger setter.
Changes:
- Link DeviceInfo profile with
rdk_otlp(-lrdk_otlp). - Initialize OTLP instrumentation during RBUS DML provider startup.
- Start/finish a distributed trace and set/clear RBUS trace context around
rbusEvent_Publish()for the remote-debugger issue event.
File summaries
| File | Description |
|---|---|
| src/hostif/profiles/DeviceInfo/Makefile.am | Adds -lrdk_otlp to support new OTLP instrumentation calls from DeviceInfo code. |
| src/hostif/profiles/DeviceInfo/Device_DeviceInfo.cpp | Starts an OTLP distributed trace and propagates trace context into RBUS before publishing the remote-debugger issue event. |
| src/hostif/handlers/src/hostIf_rbus_Dml_Provider.cpp | Initializes OTLP instrumentation during RBUS DML provider initialization. |
Review details
Suppressed comments (1)
src/hostif/profiles/DeviceInfo/Device_DeviceInfo.cpp:4029
rbusHandle_SetTraceContextFromString()/rbusHandle_ClearTraceContext()mutate trace context on the shared globalrbusHandle. Sincetr69hostifis multi-threaded, other threads can interleave RBUS calls while the context is temporarily set here, causing trace context to “bleed” into unrelated RBUS operations (or vice-versa).
To make propagation reliable, ensure the set→publish→clear sequence is atomic across all RBUS users (e.g., a global RBUS mutex used by all RBUS operations that can run concurrently, or a dedicated per-operation RBUS handle for publishing).
if(issueStr == NULL)
{
RDK_LOG(RDK_LOG_ERROR,LOG_TR69HOSTIF,"[%s %d] Memory Allocation Failed.\n",__FUNCTION__,__LINE__);
return retVal;
}
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| //#include "rbus.h" | ||
|
|
||
|
|
||
| #include "rdk_otlp_instrumentation.h" |
| #include "hostIf_NotificationHandler.h" | ||
| #include "safec_lib.h" | ||
| #include "rdk_otlp_instrumentation.h" | ||
|
|
There was a problem hiding this comment.
🟡 Changes recommended
The new OTLP symbols are used by multiple binaries/objects, but link flags are only updated in one profile Makefile, which is likely to break linking for tr69hostif and/or existing gtest targets.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
|
|
||
| AM_LDFLAGS = $(PROCPS_LIBS) -lIARMBus -lyajl -ldbus-1 -lcurl -lrfcapi -lrbus -luploadstblogs | ||
|
|
||
| AM_LDFLAGS = $(PROCPS_LIBS) -lIARMBus -lyajl -ldbus-1 -lcurl -lrfcapi -lrbus -luploadstblogs -lrdk_otlp |
| //#include "rbus.h" | ||
|
|
||
|
|
||
| #include "rdk_otlp_instrumentation.h" |
|
|
||
| #include "hostIf_NotificationHandler.h" | ||
| #include "safec_lib.h" | ||
| #include "rdk_otlp_instrumentation.h" |
Code Coverage Summary |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved OTLP linkage, incomplete trace-context propagation, and insufficient test coverage remain.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (4)
src/hostif/handlers/src/hostIf_rbus_Dml_Provider.cpp:468
- This reference is introduced in
libMsgHandlers.la, butsrc/hostif/handlers/Makefile.amstill does not link-lrdk_otlp; the flag added under DeviceInfo only makes the final daemon link succeed accidentally when that profile is also present. Targets that link the handler library independently, includinghandlers_gtest, get an undefined reference tordk_otlp_init. Declare the dependency for the handler target and its tests (preferably through configure/pkg-config).
rdk_otlp_init("tr69hostif_provider", "1.0.0");
src/hostif/profiles/DeviceInfo/Device_DeviceInfo.cpp:4238
- The existing unit-test RBUS stubs declare
rbusHandle_SetTraceContextFromStringandrbusHandle_ClearTraceContextbut do not define them. Because the gtest targets compileDevice_DeviceInfo.cppdirectly, these new calls will produce unresolved symbols; add stub implementations (and assertions if propagation is tested) or link the real RBUS library for those targets.
rbusError_t clearTraceRc = rbusHandle_ClearTraceContext(rbusHandle);
src/hostif/profiles/DeviceInfo/Device_DeviceInfo.cpp:4198
- This wraps only
RRD_SET_ISSUE_EVENT. The same provider has otherrbusEvent_Publishpaths forRRD_WEBCFG_ISSUE_EVENT,RDK_REBOOTSTOP_ENABLE, andRDM_DOWNLOAD_EVENT, which still publish without setting the RBUS trace context, so cross-path propagation remains absent for those subscriber callbacks. Apply the same sequence through a shared publish helper or instrument each intended path.
rdk_otlp_start_distributed_trace(RRD_SET_ISSUE_EVENT, "publish");
src/hostif/profiles/DeviceInfo/Makefile.am:35
- The new
rdk_otlp_*calls are built intolibMsgHandlersas well aslibdeviceinfo, but this adds-lrdk_otlponly to DeviceInfo's link flags. The DeviceInfo and handlers gtest targets compile these sources directly and theirCOMMON_LDADDvalues do not linklibrdk_otlp;dm_stubs.cppalso has no definitions for the newly used RBUS trace APIs. Those test binaries will therefore fail at link time, and the handler library does not declare its own dependency. Move the library dependency to the target/library that uses it and add the required test stubs or test link dependencies.
AM_LDFLAGS = $(PROCPS_LIBS) -lIARMBus -lyajl -ldbus-1 -lcurl -lrfcapi -lrbus -luploadstblogs -lrdk_otlp
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
| rc = rbusEvent_Publish(rbusHandle, &event); | ||
| if (traceContextSet) | ||
| { | ||
| rbusError_t clearTraceRc = rbusHandle_ClearTraceContext(rbusHandle); |
| if (tp != NULL && tp[0] != '\0') | ||
| { | ||
| rbusError_t traceRc = rbusHandle_SetTraceContextFromString(rbusHandle, tp, ""); |
Reason for change: When provider component acquires TraceParent & TraceContext using OTEL and Sets to RBUS>
The provider component, calls RPC operations like EventPublish The consumer/subscriber callback receives the same TraceParent & TraceContext
Test Procedure: Refer the ticket descriptions
Risks: Medium
Priority: P0