-
Notifications
You must be signed in to change notification settings - Fork 4
Topic/otel shlib #165
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: develop
Are you sure you want to change the base?
Topic/otel shlib #165
Conversation
Signed-off-by: PriyaDharshini_Kathiravan <priyakathiravan05@gmail.com>
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.
Pull request overview
This pull request integrates OpenTelemetry (OTLP) instrumentation into the RDK RFC (Remote Feature Control) codebase to enable distributed tracing and metrics collection for parameter operations.
Changes:
- Added OpenTelemetry initialization and distributed tracing to
tr181andrfcMgrapplications - Instrumented
getRFCParameterandsetRFCParameterfunctions with timing metrics - Updated build configuration to link against the
lrdk_otlplibrary and include necessary headers
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| utils/tr181utils.cpp | Added OpenTelemetry initialization in main() and distributed trace span creation/completion in getAttribute/setAttribute functions |
| utils/Makefile.am | Added -lrdk_otlp library dependency and include path for OpenTelemetry headers |
| rfcapi/rfcapi.cpp | Added timing instrumentation to measure and record metrics for getRFCParameter and setRFCParameter operations |
| rfcapi/Makefile.am | Added -lrdk_otlp library dependency and include path for OpenTelemetry headers |
| rfcMgr/rfc_main.cpp | Added OpenTelemetry initialization calls at application startup |
| rfcMgr/Makefile.am | Added -lrdk_otlp library dependency to linker flags |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| */ | ||
| static int setAttribute(char * const paramName ,char type, char * value) | ||
| { | ||
| //Start distributed trace (creates parent span and stores context) |
Copilot
AI
Feb 4, 2026
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.
Missing space after comment slashes. The codebase convention is to use "// " with a space after the slashes for inline comments, as seen elsewhere in this file (lines 141, 172, 309).
| //Start distributed trace (creates parent span and stores context) | |
| // Start distributed trace (creates parent span and stores context) |
| //auto timeTaken = std::chrono::duration_cast<std::chrono::microseconds>(endTime - startTime).count(); | ||
| // Record metrics (convert microseconds to seconds) | ||
| //double duration_seconds = timeTaken / 1000000.0; | ||
| //rdk_otlp_metrics_record_parameter_operation(pcParameterName, "get", duration_seconds); |
Copilot
AI
Feb 4, 2026
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.
The operation type should be "set" not "get" for the setRFCParameter function. This will cause incorrect metrics to be recorded, making it impossible to distinguish between get and set operations in the telemetry data.
| //rdk_otlp_metrics_record_parameter_operation(pcParameterName, "get", duration_seconds); | |
| //rdk_otlp_metrics_record_parameter_operation(pcParameterName, "set", duration_seconds); |
| CURL *curl_handle = NULL; | ||
| CURLcode res = CURLE_FAILED_INIT; | ||
|
|
||
| //auto startTime = std::chrono::high_resolution_clock::now(); |
Copilot
AI
Feb 4, 2026
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.
The timing measurement started here will not result in metrics being recorded if the function returns early at line 481 (wildcard/NULL parameter validation failure). This creates incomplete telemetry data where failed operations won't be tracked. Consider restructuring the code to ensure metrics are recorded for all execution paths, including error cases.
| CURL *curl_handle = NULL; | ||
| CURLcode res = CURLE_FAILED_INIT; | ||
|
|
||
| //auto startTime = std::chrono::high_resolution_clock::now(); |
Copilot
AI
Feb 4, 2026
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.
The timing measurement started here will not result in metrics being recorded if the function returns early at lines 279, 294, 300, 305, or 308 (validation failures or fallback to file-based retrieval). This creates incomplete telemetry data where these execution paths won't be tracked. Consider restructuring the code to ensure metrics are recorded for all execution paths, including error cases and fallback scenarios.
| else | ||
| librfcapi_la_CPPFLAGS = "-std=c++11" -DLINUX -fPIC -g -O2 -Wall -I=/usr/include/cjson -I=/usr/include/wdmp-c $(IARMBUS_EVENT_FLAG) | ||
| librfcapi_la_LIBADD = -lcurl -lcjson -lrdkloggers | ||
| librfcapi_la_CPPFLAGS = "-std=c++11" -DLINUX -fPIC -g -O2 -Wall -I=/usr/include/cjson -I=/usr/include/wdmp-c $(IARMBUS_EVENT_FLAG) -I/mnt/L2_CONTAINER_SHARED_VOLUME/dtracing/rdk_otel_instrumentation/include |
Copilot
AI
Feb 4, 2026
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.
Hardcoded absolute path "/mnt/L2_CONTAINER_SHARED_VOLUME/dtracing/rdk_otel_instrumentation/include" makes the build non-portable and environment-specific. Consider using a configurable variable or pkg-config to locate the OpenTelemetry instrumentation headers, similar to how other dependencies are handled (e.g., cjson_CFLAGS, curl_CFLAGS).
| librfcapi_la_CPPFLAGS = "-std=c++11" -DLINUX -fPIC -g -O2 -Wall -I=/usr/include/cjson -I=/usr/include/wdmp-c $(IARMBUS_EVENT_FLAG) -I/mnt/L2_CONTAINER_SHARED_VOLUME/dtracing/rdk_otel_instrumentation/include | |
| librfcapi_la_CPPFLAGS = "-std=c++11" -DLINUX -fPIC -g -O2 -Wall -I=/usr/include/cjson -I=/usr/include/wdmp-c $(IARMBUS_EVENT_FLAG) $(OTEL_INSTRUMENTATION_CFLAGS) |
| tr181_LDADD = ../rfcapi/.libs/librfcapi.la ../tr181api/.libs/libtr181api.la | ||
| tr181_CPPFLAGS = -std=c++0x -Wall -I${top_srcdir}/rfcapi/ -I${top_srcdir}/tr181api/ -I=/usr/include/wdmp-c | ||
| tr181_LDADD = ../rfcapi/.libs/librfcapi.la ../tr181api/.libs/libtr181api.la -lrdk_otlp | ||
| tr181_CPPFLAGS = -std=c++0x -Wall -I${top_srcdir}/rfcapi/ -I${top_srcdir}/tr181api/ -I=/usr/include/wdmp-c -I/mnt/L2_CONTAINER_SHARED_VOLUME/dtracing/rdk_otel_instrumentation/include |
Copilot
AI
Feb 4, 2026
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.
Hardcoded absolute path "/mnt/L2_CONTAINER_SHARED_VOLUME/dtracing/rdk_otel_instrumentation/include" makes the build non-portable and environment-specific. Consider using a configurable variable or pkg-config to locate the OpenTelemetry instrumentation headers, similar to how other dependencies are handled.
| tr181_CPPFLAGS = -std=c++0x -Wall -I${top_srcdir}/rfcapi/ -I${top_srcdir}/tr181api/ -I=/usr/include/wdmp-c -I/mnt/L2_CONTAINER_SHARED_VOLUME/dtracing/rdk_otel_instrumentation/include | |
| RDK_OTEL_INSTRUMENTATION_CFLAGS ?= -I/mnt/L2_CONTAINER_SHARED_VOLUME/dtracing/rdk_otel_instrumentation/include | |
| tr181_CPPFLAGS = -std=c++0x -Wall -I${top_srcdir}/rfcapi/ -I${top_srcdir}/tr181api/ -I=/usr/include/wdmp-c $(RDK_OTEL_INSTRUMENTATION_CFLAGS) |
| @@ -23,7 +23,7 @@ rfcMgr_SOURCES = rfc_main.cpp rfc_manager.cpp rfc_common.cpp mtlsUtils.cpp rfc_x | |||
| rfcMgr_CPPFLAGS = -std=c++17 -Wall -Werror -DRDK_LOGGER -I${top_srcdir}/rfcMgr -I${top_srcdir}/rfcapi/ $(cjson_CFLAGS) $(curl_CFLAGS) $(CFLAGS) | |||
Copilot
AI
Feb 4, 2026
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.
The rfcMgr includes the "rdk_otlp_instrumentation.h" header file at line 22 and calls rdk_otlp_init and rdk_otlp_metrics_init, but the Makefile doesn't include the path to find this header. The include path should be added to rfcMgr_CPPFLAGS, similar to what was done in rfcapi/Makefile.am and utils/Makefile.am. Without this, the build will fail with a "header not found" error.
| rfcMgr_CPPFLAGS = -std=c++17 -Wall -Werror -DRDK_LOGGER -I${top_srcdir}/rfcMgr -I${top_srcdir}/rfcapi/ $(cjson_CFLAGS) $(curl_CFLAGS) $(CFLAGS) | |
| rfcMgr_CPPFLAGS = -std=c++17 -Wall -Werror -DRDK_LOGGER -I${top_srcdir}/rfcMgr -I${top_srcdir}/rfcapi/ -I$(PKG_CONFIG_SYSROOT_DIR)${includedir}/rdk/otlp $(cjson_CFLAGS) $(curl_CFLAGS) $(CFLAGS) |
No description provided.