-
Notifications
You must be signed in to change notification settings - Fork 3
Topic/otel shlib #347
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 #347
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 adds OpenTelemetry (OTLP) instrumentation to the tr69hostif component for distributed tracing and metrics collection. The changes integrate the rdk_otel_instrumentation library to track parameter get/set operations with timing metrics and span tracing.
Changes:
- Adds OpenTelemetry initialization and instrumentation calls to track GET/SET operations in the message handler
- Removes duplicate getCurrentTime function from hostIf_utils.cpp (keeping the implementation in webpa_adapter.cpp)
- Updates build configuration to link against the new rdk_otlp shared library
- Modifies format specifiers for time values and removes unused stub header file
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/unittest/stubs/rdk_debug.h | Removes unused stub header file for unit tests |
| src/hostif/src/hostIf_utils.cpp | Removes getCurrentTime function (implementation exists elsewhere) |
| src/hostif/src/hostIf_main.cpp | Initializes OpenTelemetry instrumentation at startup |
| src/hostif/parodusClient/pal/mock-parodus/Makefile.am | Adds -lrdk_otlp library to test mock build |
| src/hostif/parodusClient/pal/Makefile.am | Minor formatting change to library flags |
| src/hostif/handlers/src/hostIf_msgHandler.cpp | Instruments GET/SET handlers with tracing spans and metrics |
| src/Makefile.am | Adds include path and library link for RDK OTLP instrumentation |
| cov_build.sh | Updates RFC clone to use topic/otel_shlib branch |
| configure.ac | Comments out libproc package check |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * | ||
| * @param[in] Time spec timer | ||
| */ | ||
| void getCurrentTime(struct timespec *timer) | ||
| { | ||
| clock_gettime(CLOCK_REALTIME, timer); | ||
| } | ||
|
|
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 function getCurrentTime is removed from this file, but it's still called in other parts of the codebase (e.g., src/hostif/httpserver/src/http_server.cpp lines 69 and 218, src/hostif/parodusClient/pal/libpd.cpp lines 172 and 201). While the function exists in webpa_adapter.cpp, these usages may need verification to ensure they still link correctly, especially since the declaration in hostIf_utils.h (line 157) is unchanged.
| ], | ||
| [ | ||
| PKG_CHECK_MODULES([PROCPS], [libproc >= 3.2.8])]) | ||
| ###### PKG_CHECK_MODULES([PROCPS], [libproc >= 3.2.8])]) |
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 else branch of the AC_ARG_ENABLE([morty]) block is incomplete - it has a commented-out PKG_CHECK_MODULES statement with ###### prefix and appears to be missing a closing bracket. This will cause a syntax error in the configure script. The line should either be properly uncommented and completed, or the entire else block should be removed if not needed.
| -I${PKG_CONFIG_SYSROOT_DIR}$(includedir)/wrp-c $(GLIB_CFLAGS) | ||
|
|
||
| libparodusclient_la_LDFLAGS= -llibparodus -lrdkloggers -lwdmp-c -lwrp-c -lcjson -lpthread | ||
| libparodusclient_la_LDFLAGS= -llibparodus -lrdkloggers -lwdmp-c -lwrp-c -lcjson -lpthread |
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 library -lrdk_otlp is added to the main build in src/Makefile.am but not to the parodus client library (libparodusclient.la) in this Makefile. This inconsistency may cause linking issues if libparodusclient needs to use the rdk_otlp functions. Verify whether this library should also be added here or if the dependency is properly handled elsewhere.
| libparodusclient_la_LDFLAGS= -llibparodus -lrdkloggers -lwdmp-c -lwrp-c -lcjson -lpthread | |
| libparodusclient_la_LDFLAGS= -llibparodus -lrdkloggers -lwdmp-c -lwrp-c -lrdk_otlp -lcjson -lpthread |
| rdk_otlp_init("tr69hostif", "1.0.0"); | ||
| rdk_otlp_metrics_init(); | ||
| RDK_LOG(RDK_LOG_INFO, LOG_TR69HOSTIF, "TR69HostIF OTLP instrumentation initialized\n"); |
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 OpenTelemetry initialization (rdk_otlp_init and rdk_otlp_metrics_init) is performed at startup, but there's no corresponding cleanup or shutdown call in the exit_gracefully function (around line 603-648). This could lead to resource leaks or incomplete telemetry data. Consider adding cleanup calls like rdk_otlp_shutdown or rdk_otlp_cleanup before the application exits.
|
|
||
| // Record metric - convert microseconds to seconds | ||
| double duration_seconds = timeTaken / 1000000.0; | ||
| //rdk_otlp_metrics_record_parameter_operation(stMsgData->paramName, "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.
This line uses tabs for indentation while the surrounding code uses spaces. The inconsistent indentation should be corrected to match the rest of the file which uses spaces.
| //rdk_otlp_metrics_record_parameter_operation(stMsgData->paramName, "get", duration_seconds); | |
| //rdk_otlp_metrics_record_parameter_operation(stMsgData->paramName, "get", duration_seconds); |
|
|
||
| // Record metric - convert microseconds to seconds | ||
| double duration_seconds = timeTakenset / 1000000.0; | ||
| //rdk_otlp_metrics_record_parameter_operation(stMsgData->paramName, "set", 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.
This line uses tabs for indentation while the surrounding code uses spaces. The inconsistent indentation should be corrected to match the rest of the file which uses spaces.
| //rdk_otlp_metrics_record_parameter_operation(stMsgData->paramName, "set", duration_seconds); | |
| //rdk_otlp_metrics_record_parameter_operation(stMsgData->paramName, "set", duration_seconds); |
| -I$(top_srcdir)/src/hostif/profiles/DeviceInfo \ | ||
| -I$(top_srcdir)/src/hostif/profiles/Device | ||
| -I$(top_srcdir)/src/hostif/profiles/Device \ | ||
| -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.
The hardcoded absolute path /mnt/L2_CONTAINER_SHARED_VOLUME/dtracing/rdk_otel_instrumentation/include is problematic for portability and maintainability. Consider using a configurable path via environment variable or autoconf variable (e.g., --with-rdk-otlp-include) to make the build more flexible across different environments.
| -I${PKG_CONFIG_SYSROOT_DIR}$(includedir)/wrp-c $(GLIB_CFLAGS) | ||
|
|
||
| libparodusclient_la_LDFLAGS= -llibparodus -lrdkloggers -lwdmp-c -lwrp-c -lcjson -lpthread | ||
| libparodusclient_la_LDFLAGS= -llibparodus -lrdkloggers -lwdmp-c -lwrp-c -lcjson -lpthread |
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.
Trailing whitespace detected at the end of this line. This should be removed to maintain code cleanliness.
| rdk_otlp_start_child_span(stMsgData->paramName, "get"); | ||
| try | ||
| { |
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 rdk_otlp_start_child_span is called before the try block, which means if any exception occurs during span creation, the corresponding rdk_otlp_finish_child_span will still be called. This could lead to mismatched span start/finish calls if the start function throws an exception. Consider moving the span start inside the try block or wrapping the entire function body (including span management) in a try-catch to ensure proper cleanup.
| rdk_otlp_start_child_span(stMsgData->paramName, "get"); | |
| try | |
| { | |
| try | |
| { | |
| rdk_otlp_start_child_span(stMsgData->paramName, "get"); |
|
|
||
|
|
||
| // Start child span (retrieves parent context from TraceContextManager) | ||
| rdk_otlp_start_child_span(stMsgData->paramName, "set"); |
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 rdk_otlp_start_child_span is called before any error handling, and rdk_otlp_finish_child_span is always called at the end. However, if an exception occurs during span creation or if the function returns early due to other conditions, this could lead to mismatched span start/finish calls. Consider using RAII pattern (e.g., a scope guard) to ensure the span is properly closed even in exceptional cases.
| #endif | ||
| #include "x_rdk_req_handler.h" | ||
|
|
||
| #include "rdk_otlp_instrumentation.h" |
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.
Coverity Issue - Unrecoverable parse warning
cannot open source file "rdk_otlp_instrumentation.h"
Low Impact, CWE-none
PARSE_ERROR
No description provided.