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.

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 mode and trace config entries in trace settings API #7050

Merged
merged 19 commits into from
Apr 11, 2024

Conversation

indrajit96
Copy link
Contributor

PR for
https://jirasw.nvidia.com/browse/DLIS-4960

/trace/setting API updated to return

  1. trace_mode
  2. trace_config

Old output:
{"trace_level":["TIMESTAMPS"],"trace_rate":"1","trace_count":"-1","log_frequency":"0","trace_file":""}

New Output:
{"trace_level":["TIMESTAMPS"],"trace_rate":"1","trace_count":"-1","log_frequency":"0","trace_file":"","trace_mode":"OPENTELEMETRY","url":"localhost:4318/v1/traces","bsp_max_queue_size":"1","bsp_max_export_batch_size":"512","bsp_schedule_delay":"5000"}

Tested for both modes TRITON and OPENTELEMETRY.

@rmccorm4
Copy link
Contributor

Please update PR title to be more descriptive for the changes 🙏

@indrajit96 indrajit96 changed the title Indrajit tracing Add trace mode and trace config entries in trace settings API Mar 27, 2024
@indrajit96
Copy link
Contributor Author

Please update PR title to be more descriptive for the changes 🙏
Fixed thanks :)
github automatically took my branch name as PR title

src/grpc/grpc_server.cc Outdated Show resolved Hide resolved
src/grpc/grpc_server.cc Outdated Show resolved Hide resolved
src/grpc/grpc_server.cc Outdated Show resolved Hide resolved
src/http_server.cc Outdated Show resolved Hide resolved
Comment on lines +2007 to +2008
auto mode_key = std::to_string(trace_mode);
auto trace_options_it = config_map.find(mode_key);
Copy link
Contributor

@rmccorm4 rmccorm4 Apr 5, 2024

Choose a reason for hiding this comment

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

Food for thought in the future with Olga, but I found this code misleading to read.

Upon reading it, I naively thought std::to_string(trace_mode) would be something like "TRITON" or "OPENTELEMETRY", but it's actually "0" or "1" based on order in the enum. InferenceTraceModeString is the one that actually converts to a string.

TLDR; I found it confusing that the keys in this map are "0" and "1" rather than the actual string versions of the modes. The keys in the map could probably just be the enums themselves (TRACE_MODE_TRITON or TRACE_MODE_OPENTELEMETRY [interpreted as integer 0 or 1] rather than string 0 or 1), or the actual string representations ("triton" and "opentelemtry").

No action here, just mentioning something I found misleading to think about down the line.

rmccorm4
rmccorm4 previously approved these changes Apr 5, 2024
Copy link
Contributor

@rmccorm4 rmccorm4 left a comment

Choose a reason for hiding this comment

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

Nice changes! 🚀

@rmccorm4 rmccorm4 requested a review from tanmayv25 April 5, 2024 19:22
Comment on lines +1396 to +1397
request.model_name(), &level, &rate, &count, &log_frequency, &filepath,
&trace_mode, &config_map);
Copy link
Contributor

Choose a reason for hiding this comment

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

This function is probably due for some cleanup in the near future, where you can encapsulate this growing list of parameters into a struct/class or something if makes sense like:

GetTraceSetting(model_name, &trace_settings)

rather than all the individual args.

However, I think this can be a follow-up cleanup rather than a blocker for this change. Feel free to add a JIRA ticket if you agree.

I think some other sections could probably be condensed or cleaned up as well if encapsulating the fields in an object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I will take this up in the Update PR for
https://jirasw.nvidia.com/browse/DLIS-4795

@rmccorm4
Copy link
Contributor

rmccorm4 commented Apr 8, 2024

Random note @indrajit96 but I also see the message in this block (not from your changes) is wrong. The warning should mention deprecrated --trace-log-frequency and not --trace-file.

Feel free to add to this PR, or submit a separate small message-fix PR

@@ -64,6 +64,7 @@ namespace triton { namespace server {

using TraceConfig = std::vector<
std::pair<std::string, std::variant<std::string, int, uint32_t>>>;
// Key is trace mode,
Copy link
Contributor

Choose a reason for hiding this comment

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

Not only trace mode, IIRC, for global settings the key is an empty string

src/http_server.cc Outdated Show resolved Hide resolved
@oandreeva-nv
Copy link
Contributor

As a follow-up, I think it is nice to send to the user only relevant settings, i.e. if mode is opentelemetry, then trace file and log frequency are irrelevant.

Next suggestion, can we lowercase mode in the response, please? When users set up mode, it is in lower case, so it would be nice to send it back in lower case as well

@indrajit96
Copy link
Contributor Author

Random note @indrajit96 but I also see the message in this block (not from your changes) is wrong. The warning should mention deprecrated --trace-log-frequency and not --trace-file.

Feel free to add to this PR, or submit a separate small message-fix PR

Resolved

@indrajit96
Copy link
Contributor Author

As a follow-up, I think it is nice to send to the user only relevant settings, i.e. if mode is opentelemetry, then trace file and log frequency are irrelevant.

Next suggestion, can we lowercase mode in the response, please? When users set up mode, it is in lower case, so it would be nice to send it back in lower case as well

Resolved

src/command_line_parser.cc Outdated Show resolved Hide resolved
src/http_server.cc Outdated Show resolved Hide resolved
qa/L0_trace/test.sh Outdated Show resolved Hide resolved
@indrajit96 indrajit96 merged commit 752f7cb into main Apr 11, 2024
2 checks passed
@indrajit96 indrajit96 deleted the indrajit_tracing branch April 11, 2024 18:04
indrajit96 added a commit that referenced this pull request Apr 11, 2024
Add trace_mode and trace_config to getTraceSettingsAPI

---------

Co-authored-by: Ryan McCormick <[email protected]>
mc-nv pushed a commit that referenced this pull request Apr 11, 2024
…#7103)

Add trace_mode and trace_config to getTraceSettingsAPI

---------

Co-authored-by: Ryan McCormick <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants