-
Notifications
You must be signed in to change notification settings - Fork 301
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
HPCC-29332 Add opentelemetry based tracing #17538
Changes from 11 commits
628b646
4a100ff
424055e
5451509
eeaf913
39495f9
8fac373
cb310f3
92c69f4
ec8d04c
3a71c97
3738790
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -184,6 +184,39 @@ void checkSetCORSAllowOrigin(EspHttpBinding *binding, CHttpRequest *req, CHttpRe | |
|
||
int CEspHttpServer::processRequest() | ||
{ | ||
//Mock http headers from request | ||
Owned<IProperties> mockHTTPHeaders = createProperties(); | ||
|
||
//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"); | ||
|
||
//This span tracks processing of httprequest | ||
//Owned<ISpan> reqProcessSpan = queryTraceManager("esp")->createServerSpan("ProcessingHTTPRequest", mockHTTPHeaders); | ||
|
||
StringArray mockEmptyHTTPHeaders; | ||
Owned<ISpan> rootSpan = queryTraceManager().createServerSpan("rootProcessingHTTPRequest", mockEmptyHTTPHeaders, nullptr); | ||
Owned<ISpan> reqProcessSpan = queryTraceManager().createServerSpan("childProcessingHTTPRequest", mockHTTPHeadersSA, rootSpan.get()); | ||
|
||
reqProcessSpan->setSpanAttribute("http.request_port", "8010"); | ||
reqProcessSpan->setSpanAttribute("app.name", "esp"); | ||
reqProcessSpan->setSpanAttribute("app.version", "1.0.0"); | ||
reqProcessSpan->setSpanAttribute("app.instance", "esp1"); | ||
reqProcessSpan->setSpanAttribute("http.method", m_request->queryMethod()); | ||
reqProcessSpan->setSpanAttribute("http.url", m_request->queryPath()); | ||
reqProcessSpan->setSpanAttribute("http.host", m_request->queryHost()); | ||
|
||
IEspContext* ctx = m_request->queryContext(); | ||
StringBuffer errMessage; | ||
m_request->setPersistentEnabled(m_apport->queryProtocol()->persistentEnabled() && !shouldClose); | ||
|
@@ -236,9 +269,11 @@ 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. typo: theese. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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()); | ||
reqProcessSpan->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); | ||
|
@@ -268,7 +303,8 @@ 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
authState = checkUserAuth(); | ||
if ((authState == authTaskDone) || (authState == authFailed)) | ||
return 0; | ||
|
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 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.
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.
100% agree on the usefulness of unit tests