Add the ability for backends to add entries to the trace #172
Conversation
Tabrizian
force-pushed
the
imant-trace-backend
branch
2 times, most recently
from
9f30cb2
to
69442e2
Compare
Tabrizian
force-pushed
the
imant-trace-backend
branch
7 times, most recently
from
cf1a090
to
ec154a6
Compare
Tabrizian
force-pushed
the
imant-trace-backend
branch
from
a67b7ef
to
cd3abc1
Compare
include/triton/core/tritonbackend.h
Outdated
| @@ -530,6 +530,15 @@ TRITONBACKEND_RequestOutputBufferProperties( | |||
| TRITONBACKEND_DECLSPEC TRITONSERVER_Error* TRITONBACKEND_RequestRelease( | |||
| TRITONBACKEND_Request* request, uint32_t release_flags); | |||
|
|
|||
| /// Get the trace associated with a request. The returned trace is owned by the | |||
| /// request, not the caller, and so should not be modified or freed. | |||
There was a problem hiding this comment.
Mention nullptr may be returned if request is not being traced
include/triton/core/tritonserver.h
Outdated
| /// \param trace_name Returns the name associated with the trace. | ||
| /// \return a TRITONSERVER_Error indicating success or failure. | ||
| TRITONSERVER_DECLSPEC TRITONSERVER_Error* TRITONSERVER_InferenceTraceName( | ||
| TRITONSERVER_InferenceTrace* trace, const char** trace_name); |
There was a problem hiding this comment.
Looking at the usage, this is actually the name of the custom activity? I think you should let TRITONSERVER_InferenceTraceReportActivity accept the name as another parameter so the backend doesn't have to call the setter before calling the report function (implicitly set by Triton). Also make it clear that if the tracer receive custom activity, should call the getter function(s) for additional information regarding the activity
There was a problem hiding this comment.
Looking at the usage, this is actually the name of the custom activity?
That's correct.
I think you should let TRITONSERVER_InferenceTraceReportActivity accept the name as another parameter so the backend doesn't have to call the setter before calling the report function (implicitly set by Triton).
I think the way it is currently implemented would allow us to add more fields to the trace without having to modify the signature of TRITONSERVER_InferenceTraceReportActivity. Let me know if this makes sense to you.
Also make it clear that if the tracer receive custom activity, should call the getter function(s) for additional information regarding the activity
👍
There was a problem hiding this comment.
I think you should let TRITONSERVER_InferenceTraceReportActivity accept the name as another parameter so the backend doesn't have to call the setter before calling the report function (implicitly set by Triton).
I actually see the problem now. How about adding a TRITONSERVER_InferenceTrace* trace_attributes that currently only has a name to TRITONSERVER_InferenceTraceReportActivity?
There was a problem hiding this comment.
I don't know if there is a need to add another struct for this, when capturing a timestamp, what else do you need to know beside the timestamp itself and the activity it indicates? Also note that those should be properties that are activity specific (i.e. may be different on different capture of the timestamp), if they are properties of the entire trace, they should not be tied to this function (i.e. request ID / parent ID is always the same for the entire trace).
If you perceive more capture specific attribute may be added into the future, I think you should make it generic (string key value pair to be attached to the capture) so all parties are not tied to the specific getter/setter exposed in Triton API. For example, Triton user is likely going to define their own attributes for tracking backend model execution, which is going to be not useful if they have to go through the API change so that the tracer can extract the information. On the other hand, the tracer implemented can simply dump all attached key-value if any, without worrying to modify the implementation whenever a new attribute is added.
|
What's the state of this PR @Tabrizian ? Do we intend to finalize it at some point? I think a new backend API like this could enable a cleaner way to report nested spans (create traces) for OpenTelemetry tracing closer to the site, rather than the current behavior of reporting everything all at the end in the TRITONBACKEND_ReportStatistics callback. CC @oandreeva-nv |
|
@rmccorm4 Sorry for the delay. This must have fallen through the cracks. Yeah, I think we probably want to have this API at some point for creating custom traces. Right now I think this is just blocked on testing. Part of it is now included in Olga's Otel PR. |
Tabrizian
force-pushed
the
imant-trace-backend
branch
from
cd3abc1
to
215cc2d
Compare
The API is tested by being used in the frontends.