Skip to content
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.

Sign up for GitHub

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

Jump to bottom

HPCC-29332 Add opentelemetry based tracing #17538

Closed
wants to merge 12 commits into from
Closed

HPCC-29332 Add opentelemetry based tracing #17538

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
628b646
HPCC-29332 Add opentelemetry-cpp to vcpkg list
rpastrana Jun 29, 2023
4a100ff
HPCC-29332 Add opentelemetry-cpp to vcpkg list
rpastrana Jul 10, 2023
424055e
Merge branch 'master' of https://github.com/hpcc-systems/HPCC-Platfor…
rpastrana Jul 18, 2023
5451509
HPCC-29332 Add opentelemetry-cpp to vcpkg list
rpastrana Jul 26, 2023
eeaf913
HPCC-29332 WIP
rpastrana Jul 30, 2023
39495f9
HPCC-29332 WIP
rpastrana Aug 15, 2023
8fac373
HPCC-29332 WIP
rpastrana Aug 15, 2023
cb310f3
HPCC-29332 Cleanup
rpastrana Aug 15, 2023
92c69f4
Merge branch 'master' of https://github.com/hpcc-systems/HPCC-Platfor…
rpastrana Aug 18, 2023
ec8d04c
HPCC-29332 Interface cleanup
rpastrana Aug 23, 2023
3a71c97
HPCC-29332 Interface cleanup
rpastrana Aug 26, 2023
3738790
HPCC-29332 Code Revie
rpastrana Aug 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 54 additions & 1 deletion esp/bindings/http/platform/httpservice.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,48 @@ void checkSetCORSAllowOrigin(EspHttpBinding *binding, CHttpRequest *req, CHttpRe

int CEspHttpServer::processRequest()
{
//Mock http headers from request
Owned<IProperties> mockHTTPHeaders = createProperties();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code should be moved to some unit tests - probably in jlibtests.cpp. That can then be expanded with other unit tests. E.g. creating server spans with and without incoming traceids, invalid trace ids etc. and check that they behave correctly.
Then can extend to check that client and internal spans can be created on parallel threads and that it doesn't cause problems, and gather overhead for creating the spans.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

100% agree on the usefulness of unit tests


//The traceparent header uses the version-trace_id-parent_id-trace_flags format where:
//version is always 00. trace_id is a hex-encoded trace id. span_id is a hex-encoded span id. trace_flags is a hex-encoded 8-bit field that contains tracing flags such as sampling, trace level, etc.
mockHTTPHeaders->setProp(/*opentelemetry::trace::propagation::kTraceParent*/"traceparent", "00-beca49ca8f3138a2842e5cf21402bfff-4b960b3e4647da3f-01");
mockHTTPHeaders->setProp(/*opentelemetry::trace::propagation::kTraceState*/"tracestate", "IncomingUGID"); //opentelemetry::trace::propagation::kTraceState
mockHTTPHeaders->setProp(HPCCSemanticConventions::kGLOBALIDHTTPHeader, "IncomingUGID");
mockHTTPHeaders->setProp(HPCCSemanticConventions::kCallerIdHTTPHeader, "IncomingCID");

StringArray mockHTTPHeadersSA;
//mock opentel traceparent context
mockHTTPHeadersSA.append("traceparent:00-beca49ca8f3138a2842e5cf21402bfff-4b960b3e4647da3f-01");
//mock opentel tracestate https://www.w3.org/TR/trace-context/#trace-context-http-headers-format
mockHTTPHeadersSA.append("tracestate:hpcc=4b960b3e4647da3f");
mockHTTPHeadersSA.append("HPCC-Global-Id:someGlobalID");
mockHTTPHeadersSA.append("HPCC-Caller-Id:IncomingCID");

Owned<ISpan> serverSpan = queryTraceManager().createServerSpan("rootProcessingHTTPRequest", mockHTTPHeadersSA);
serverSpan->setSpanAttribute("http.request_port", "8010");
serverSpan->setSpanAttribute("app.name", "esp");
serverSpan->setSpanAttribute("app.version", "1.0.0");
serverSpan->setSpanAttribute("app.instance", "esp1");
serverSpan->setSpanAttribute("http.method", m_request->queryMethod());
serverSpan->setSpanAttribute("http.url", m_request->queryPath());
serverSpan->setSpanAttribute("http.host", m_request->queryHost());

{
//Mock http headers from request
Owned<IProperties> mockClientContext = createProperties();
serverSpan->createClientSpan("MockExternalCall")->injectSpanContext(mockClientContext);

// Print out mockClientContext
Owned<IPropertyIterator> iter(mockClientContext->getIterator());
for(iter->first(); iter->isValid(); iter->next())
{
const char* key = iter->getPropKey();
const char* value = mockClientContext->queryProp(key);
DBGLOG("MockClientContext: %s=%s", key, value);
}
}

IEspContext* ctx = m_request->queryContext();
StringBuffer errMessage;
m_request->setPersistentEnabled(m_apport->queryProtocol()->persistentEnabled() && !shouldClose);
Expand Down Expand Up @@ -236,9 +278,12 @@ int CEspHttpServer::processRequest()
m_request->getEspPathInfo(stype, &pathEx, &serviceName, &methodName, false);
ESPLOG(LogNormal,"sub service type: %s. parm: %s", getSubServiceDesc(stype), m_request->queryParamStr());

//all thesee attributes could/should be tracked by opentel trace/spans
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: theese.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely something to leave for now and only consider once the telemetry implementation is stable,

m_request->updateContext();
ctx->setServiceName(serviceName.str());
ctx->setHTTPMethod(method.str());
//remove before checkin
serverSpan->setSpanAttribute("http.method", method.str());
ctx->setServiceMethod(methodName.str());
ctx->addTraceSummaryValue(LogMin, "app.protocol", method.str(), TXSUMMARY_GRP_ENTERPRISE);
ctx->addTraceSummaryValue(LogMin, "app.service", serviceName.str(), TXSUMMARY_GRP_ENTERPRISE);
Expand Down Expand Up @@ -268,8 +313,16 @@ int CEspHttpServer::processRequest()
ESPLOG(LogMin, "%s %s, from %s", method.str(), m_request->getPath(pathStr).str(), m_request->getPeer(peerStr).str());
else //user ID is in HTTP header
ESPLOG(LogMin, "%s %s, from %s@%s", method.str(), m_request->getPath(pathStr).str(), userid, m_request->getPeer(peerStr).str());

//checkUserAuth could declare nested span
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably also move to the unit tests. Creating spans when we call ldap etc. is something worth recording in a jira to come back to later.

//and/or declare this as an event on reqProcessSpan
{
Owned<ISpan> userAuthSpan = serverSpan->createInternalSpan("userAuthSpan");
authState = checkUserAuth();

StringBuffer authStateStr;
userAuthSpan->toString(authStateStr);
fprintf(stdout, "%s\n", authStateStr.str());
}
if ((authState == authTaskDone) || (authState == authFailed))
return 0;

Expand Down
2 changes: 1 addition & 1 deletion system/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ if (NOT JLIB_ONLY)
HPCC_ADD_SUBDIRECTORY (xmllib)
HPCC_ADD_SUBDIRECTORY (xmllibtest "PLATFORM")
HPCC_ADD_SUBDIRECTORY (masking)

if (USE_AERON)
project (aeron_include)
SET(CMAKE_UNITY_BUILD FALSE)
Expand Down
43 changes: 43 additions & 0 deletions system/jlib/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,12 @@ endif(NOT TARGET libbase58)

find_package(yaml CONFIG REQUIRED)

#For OpenTel exporter
find_package(Protobuf REQUIRED)
#For http exporter
#find_package(CURL REQUIRED)
find_package(opentelemetry-cpp CONFIG REQUIRED)

SET (CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${STRICT_CXX_FLAGS}")

set ( SRCS
Expand Down Expand Up @@ -207,6 +213,7 @@ include_directories (
${HPCC_SOURCE_DIR}/system/security/shared
${HPCC_SOURCE_DIR}/system/security/cryptohelper
${HPCC_SOURCE_DIR}/system/httplib
${OPENTELEMETRY_CPP_INCLUDE_DIRS}
${CMAKE_CURRENT_BINARY_DIR} # for generated jelog.h file
${CMAKE_BINARY_DIR}
${CMAKE_BINARY_DIR}/oss
Expand All @@ -230,6 +237,42 @@ if ( ${HAVE_LIBCRYPT} )
target_link_libraries ( jlib crypt)
endif ( ${HAVE_LIBCRYPT} )

find_package(gRPC CONFIG REQUIRED)

target_link_libraries ( jlib
opentelemetry-cpp::api
opentelemetry-cpp::ext
opentelemetry-cpp::sdk
opentelemetry-cpp::ostream_span_exporter
# opentelemetry-cpp::api - Imported target of opentelemetry-cpp::api
# opentelemetry-cpp::sdk - Imported target of opentelemetry-cpp::sdk
# opentelemetry-cpp::ext - Imported target of opentelemetry-cpp::ext
# opentelemetry-cpp::version - Imported target of opentelemetry-cpp::version
# opentelemetry-cpp::common - Imported target of opentelemetry-cpp::common
# opentelemetry-cpp::trace - Imported target of opentelemetry-cpp::trace
# opentelemetry-cpp::metrics - Imported target of opentelemetry-cpp::metrics
opentelemetry-cpp::logs # - Imported target of opentelemetry-cpp::logs
opentelemetry-cpp::in_memory_span_exporter # - Imported target of opentelemetry-cpp::in_memory_span_exporter
opentelemetry-cpp::otlp_grpc_client # - Imported target of opentelemetry-cpp::otlp_grpc_client
# opentelemetry-cpp::otlp_recordable - Imported target of opentelemetry-cpp::otlp_recordable
opentelemetry-cpp::otlp_grpc_exporter # - Imported target of opentelemetry-cpp::otlp_grpc_exporter
# opentelemetry-cpp::otlp_grpc_log_record_exporter - Imported target of opentelemetry-cpp::otlp_grpc_log_record_exporter
# opentelemetry-cpp::otlp_grpc_metrics_exporter - Imported target of opentelemetry-cpp::otlp_grpc_metrics_exporter
# opentelemetry-cpp::otlp_http_client - Imported target of opentelemetry-cpp::otlp_http_client
# opentelemetry-cpp::otlp_http_exporter - Imported target of opentelemetry-cpp::otlp_http_exporter
# opentelemetry-cpp::otlp_http_log_record_exporter - Imported target of opentelemetry-cpp::otlp_http_log_record_exporter
# opentelemetry-cpp::otlp_http_metric_exporter - Imported target of opentelemetry-cpp::otlp_http_metric_exporter
opentelemetry-cpp::ostream_log_record_exporter # - Imported target of opentelemetry-cpp::ostream_log_record_exporter
# opentelemetry-cpp::ostream_metrics_exporter - Imported target of opentelemetry-cpp::ostream_metrics_exporter
# opentelemetry-cpp::ostream_span_exporter - Imported target of opentelemetry-cpp::ostream_span_exporter
# opentelemetry-cpp::elasticsearch_log_record_exporter - Imported target of opentelemetry-cpp::elasticsearch_log_record_exporter
# opentelemetry-cpp::etw_exporter - Imported target of opentelemetry-cpp::etw_exporter
# opentelemetry-cpp::zpages - Imported target of opentelemetry-cpp::zpages
# opentelemetry-cpp::http_client_curl - Imported target of opentelemetry-cpp::http_client_curl
# opentelemetry-cpp::opentracing_shim - Imported target of opentelemetry-cpp::opentracing_shim

)

IF (USE_OPENSSL)
target_link_libraries ( jlib ${OPENSSL_LIBRARIES})
endif (USE_OPENSSL)
Expand Down
2 changes: 2 additions & 0 deletions system/jlib/jptree.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9067,6 +9067,8 @@ jlib_decl IPropertyTree * loadConfiguration(IPropertyTree *componentDefault, con
configFileUpdater->init(std::get<0>(result).c_str(), componentDefault, argv, componentTag, envPrefix, legacyFilename, mapper, altNameAttribute);
if (monitor)
configFileUpdater->startMonitoring();

initTraceManager(componentTag, componentConfiguration.get());
return componentConfiguration.getLink();
}

Expand Down
Loading
Loading