Topic/otel shlib #165
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -27,7 +27,7 @@ if ENABLE_RDKC | |||||
| librfcapi_la_CPPFLAGS = "-std=c++11" -DLINUX -fPIC -g -O2 -Wall -DRDKC -I${top_srcdir}/../rdklogger/include/ | ||||||
| librfcapi_la_LIBADD = -L${top_srcdir}/../rdklogger/src/.libs -lrdkloggers | ||||||
| else | ||||||
| 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 | ||||||
|
There was a problem hiding this comment. 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).
Suggested change
|
||||||
| librfcapi_la_LIBADD = -lcurl -lcjson -lrdkloggers -lrdk_otlp | ||||||
| endif | ||||||
| endif | ||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,6 +1,6 @@ | ||||||
| /* | ||||||
| * If not stated otherwise in this file or this component's LICENSE file the | ||||||
| * following copyright and licenses apply: | ||||||
|
Check failure on line 3 in rfcapi/rfcapi.cpp
|
||||||
| * | ||||||
| * Copyright 2016 RDK Management | ||||||
| * | ||||||
|
|
@@ -29,6 +29,9 @@ | |||||
| #include <dirent.h> | ||||||
| #include "rfcapi.h" | ||||||
| #include "rdk_debug.h" | ||||||
| #include "rdk_otlp_instrumentation.h" | ||||||
| #include <chrono> | ||||||
| #include <iostream> | ||||||
| using namespace std; | ||||||
|
|
||||||
| #define LOG_RFCAPI "LOG.RDK.RFCAPI" | ||||||
|
|
@@ -266,6 +269,7 @@ | |||||
| CURL *curl_handle = NULL; | ||||||
| CURLcode res = CURLE_FAILED_INIT; | ||||||
|
|
||||||
| //auto startTime = std::chrono::high_resolution_clock::now(); | ||||||
|
There was a problem hiding this comment. 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. |
||||||
| if(!strcmp(pcParameterName+strlen(pcParameterName)-1,".")) | ||||||
| { | ||||||
| #ifdef TEMP_LOGGING | ||||||
|
|
@@ -449,6 +453,11 @@ | |||||
| cJSON_Delete(response_json); | ||||||
| } | ||||||
| } | ||||||
| //auto endTime = std::chrono::high_resolution_clock::now(); | ||||||
| //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); | ||||||
| return ret; | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -462,7 +471,7 @@ | |||||
| string response; | ||||||
| CURL *curl_handle = NULL; | ||||||
| CURLcode res = CURLE_FAILED_INIT; | ||||||
| //auto startTime = std::chrono::high_resolution_clock::now(); | ||||||
|
There was a problem hiding this comment. 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. |
||||||
| if(!strcmp(pcParameterName+strlen(pcParameterName)-1,".") && pcParameterValue == NULL) | ||||||
| { | ||||||
| #ifdef TEMP_LOGGING | ||||||
|
|
@@ -564,6 +573,11 @@ | |||||
| cJSON_Delete(response_json); | ||||||
| } | ||||||
| } | ||||||
| //auto endTime = std::chrono::high_resolution_clock::now(); | ||||||
| //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); | ||||||
|
There was a problem hiding this comment. 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.
Suggested change
|
||||||
| return ret; | ||||||
| } | ||||||
|
|
||||||
|
|
||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -27,8 +27,8 @@ bin_PROGRAMS += rfctool | |||||||
| endif | ||||||||
|
|
||||||||
| tr181_SOURCES = tr181utils.cpp trsetutils.cpp | ||||||||
| 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 | ||||||||
|
There was a problem hiding this comment. 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.
Suggested change
|
||||||||
|
|
||||||||
| if IS_IARMBUS_ENABLED | ||||||||
| tr181_LDADD += -lIARMBus | ||||||||
|
|
||||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,6 +1,6 @@ | ||||||
| /* | ||||||
| * If not stated otherwise in this file or this component's LICENSE file the | ||||||
| * following copyright and licenses apply: | ||||||
|
Check failure on line 3 in utils/tr181utils.cpp
|
||||||
| * | ||||||
| * Copyright 2018 RDK Management | ||||||
| * | ||||||
|
|
@@ -40,7 +40,7 @@ | |||||
| #include "trsetutils.h" | ||||||
| #include <fcntl.h> | ||||||
| #include <unistd.h> | ||||||
| #include "rdk_otlp_instrumentation.h" | ||||||
| using namespace std; | ||||||
|
|
||||||
| static char value_type = 'u'; | ||||||
|
|
@@ -138,6 +138,9 @@ | |||||
| */ | ||||||
| static int getAttribute(char * const paramName) | ||||||
| { | ||||||
| // Start distributed trace (creates parent span and stores context) | ||||||
| rdk_otlp_start_distributed_trace(paramName, "get"); | ||||||
|
|
||||||
| if (id && !strncmp(id, "localOnly", 9)) { | ||||||
| TR181_ParamData_t param; | ||||||
| tr181ErrorCode_t status = getLocalParam(id, paramName, ¶m); | ||||||
|
|
@@ -150,6 +153,7 @@ | |||||
| { | ||||||
| cout << __FUNCTION__ << " >> Failed to retrieve : Reason " << getTR181ErrorString(status) << endl; | ||||||
| } | ||||||
| rdk_otlp_finish_distributed_trace(); | ||||||
| return status; | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -165,7 +169,8 @@ | |||||
| { | ||||||
| cout << __FUNCTION__ << " >> Failed to retrieve : Reason " << getRFCErrorString(status) << endl; | ||||||
| } | ||||||
| // Finish distributed trace (ends parent span) | ||||||
| rdk_otlp_finish_distributed_trace(); | ||||||
| return status; | ||||||
| } | ||||||
| /** | ||||||
|
|
@@ -177,6 +182,8 @@ | |||||
| */ | ||||||
| static int setAttribute(char * const paramName ,char type, char * value) | ||||||
| { | ||||||
| //Start distributed trace (creates parent span and stores context) | ||||||
|
There was a problem hiding this comment. 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).
Suggested change
|
||||||
| rdk_otlp_start_distributed_trace(paramName, "set"); | ||||||
| if (id && !strncmp(id, "localOnly", 9)) { | ||||||
| int status = setLocalParam(id, paramName, value); | ||||||
| if(status == 0) | ||||||
|
|
@@ -187,6 +194,7 @@ | |||||
| { | ||||||
| cout << __FUNCTION__ << " >> Failed to Set Local Param." << endl; | ||||||
| } | ||||||
| rdk_otlp_finish_distributed_trace(); | ||||||
| return status; | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -203,6 +211,7 @@ | |||||
| else | ||||||
| cout << __FUNCTION__ << " >> Set operation success " << endl; | ||||||
|
|
||||||
| rdk_otlp_finish_distributed_trace(); | ||||||
| return status; | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -297,6 +306,10 @@ | |||||
| #ifndef GTEST_ENABLE | ||||||
| int main(int argc, char *argv []) | ||||||
| { | ||||||
| // Initialize OpenTelemetry instrumentation | ||||||
| rdk_otlp_init("tr181", "1.0.0"); | ||||||
| rdk_otlp_metrics_init(); | ||||||
|
|
||||||
| if(legacyRfcEnabled() == true) | ||||||
| { | ||||||
| return trsetutil(argc,argv); | ||||||
|
|
||||||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.