-
Notifications
You must be signed in to change notification settings - Fork 104
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
[WIP] Enabling nested spans for trace mode OpenTelemetry #222
Conversation
@@ -50,6 +50,12 @@ | |||
#include "triton/common/triton_json.h" | |||
#include "tritonserver_apis.h" | |||
|
|||
#ifndef _WIN32 | |||
#include "opentelemetry/sdk/trace/tracer.h" | |||
namespace otel_trace_api = opentelemetry::trace; |
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.
@oandreeva-nv in your screenshot examples, why are they labeled "unknown_service"? Is there some label we can put in the source to have these say "Triton" or something in the collector/UI view if there is no label already set?
(i.e. if supporting passing existing span/context in the future, maybe that would have some label associated with it, I'm not sure how those APIs look)
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.
It's due to the jaeger. I was planning to investigate if we can do something with this on our side, or if it is something, that need to be addressed on the collector's side
Working on the doc for discussions |
Closing: moved this logic to server side |
This PR enables nested spans for trace mode OpenTelemetry. Major changes are:
cclang
, so it has a lot ofcclang
related changesCurrent logic : on the core side I create 2 spans: Request span, which has the same name as the model, this request is sent to, and compute span, which holds compute related timestamps and events and represents the amount of time compute took:
How it looks:
Note: this PR should be merged with server's PR: triton-inference-server/server#5928