-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Add trace endpoints. Add trace log frequency #3849
Conversation
The trace logic has been reworked to the following:
|
TIMESTAMPS" to trace timestamps, "TENSORS" to trace tensors. | ||
This value is an array of string whhere user may specify multiple levels to | ||
trace multiple informations. | ||
- "trace_rate" : the trace sampling rate. The value represents how many requests |
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.
Need to add "trace_count" here and also in the cmdline. trace_count defaults to 0 which means to continue collecting trace indefinitely. But it can be set to non-zero to cause trace_level to go to OFF after collecting trace_count traces.
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.
If trace_level
is updated to OFF when trace_count
exceeds, I assume the following will be true?
- Turning OFF global
trace_level
bytrace_count
does not cause any model specific setting to turn off itstrace_level
even if it wasn't specified for the model. In other words, thetrace_level
in model setting will be marked as "specified" once the globaltrace_level
is affected bytrace_count
- The user can turn
trace_level
back on with the update API. For global setting, due to the behavior in 1., this change will not be reflected to model setting unless the user "clear" the modeltrace_level
first.
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.
Would it be easier to explain and implement if we don't actually change trace_level when trace_count is done? In the implementation we would trace only if trace_level was not OFF and trace_count > 0. We can use trace_count = -1 to mean unlimited trace_count.
Also be easier for user to just reset trace_count if they want to restart or change it (no need to also reset trace_level). Also, we can be consistent withe trace_count, when it is set in global and has not been specifically set in model then we can just set it in the model as well.
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.
So we use trace_count
to directly indicate the remaining trace to be sampled for the particular setting, and a -1 value is used for no trace limit? Yes, that will be more clear to know why exactly the trace stops.
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.
Yes, and we don't adjust trace_level ourselves.
TIMESTAMPS" to trace timestamps, "TENSORS" to trace tensors. | ||
This value is an array of string whhere user may specify multiple levels to | ||
trace multiple informations. | ||
- "trace_rate" : the trace sampling rate. The value represents how many requests |
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.
Would it be easier to explain and implement if we don't actually change trace_level when trace_count is done? In the implementation we would trace only if trace_level was not OFF and trace_count > 0. We can use trace_count = -1 to mean unlimited trace_count.
Also be easier for user to just reset trace_count if they want to restart or change it (no need to also reset trace_level). Also, we can be consistent withe trace_count, when it is set in global and has not been specifically set in model then we can just set it in the model as well.
src/servers/main.cc
Outdated
@@ -476,8 +476,8 @@ std::vector<Option> options_ | |||
{OPTION_TRACE_COUNT, "trace-count", Option::ArgInt, | |||
"Set the number of traces to be sampled. Once the specified number " |
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.
Delete "Once the specified... to disable tracing"
@deadeyegoodwin look-and-feel of the trace endpoints and the new trace configuration requested