-
Notifications
You must be signed in to change notification settings - Fork 1.7k
core: dump some debug info in /data/environment
#4755
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
Conversation
Summary: In #4751, we implemented `data_location` support for RustBoard, which means that the location no longer includes the gRPC address. But while this is an improvement for users, the address was convenient for development, and there’s no other way to get it from the UI. This patch augments `/data/environment` to dump some basic debug info: the TensorBoard version, the data provider, and the command-line flags. This behavior can be turned off. It’s enabled in `tensorboard(1)`, but disabled by default on the `CorePlugin` and `CorePluginLoader` APIs. Thus, users who build their own TensorBoards will not find themselves unexpectedly leaking information about their setup. For normal users of `tensorboard(1)`, the flags are fine to render; the only potentially sensitive thing is the logdir path, which is of course already prominent. Sample response, passed through `jq .`: ``` { "data_location": "logs/mnist/", "window_title": "", "debug": { "version": "2.5.0a0", "data_provider": "GrpcDataProvider(addr='localhost:42127')", "flags": { "logdir": "logs/mnist/", "logdir_spec": "", "host": null, "bind_all": true, "port": null, "reuse_port": false, "load_fast": true, "grpc_creds_type": "local", "grpc_data_provider": "", "purge_orphaned_data": true, "db": "", "db_import": false, "inspect": false, "version_tb": false, "tag": "", "event_file": "", "path_prefix": "", "window_title": "", "max_reload_threads": 1, "reload_interval": 5, "reload_task": "auto", "reload_multifile": null, "reload_multifile_inactive_secs": 86400, "generic_data": "auto", "samples_per_plugin": { "scalars": 0, "images": 0 }, "custom_predict_fn": "", "wit_data_dir": "", "__tensorboard_subcommand": "serve" } } } ``` Test Plan: Manually navigated to `/data/environment` and verified that the flags render and the data provider appears for both `--load_fast` and the legacy multiplexer. Checked that when the change to `default.py` is reverted, no debug data shows up. wchargin-branch: environment-debug-info wchargin-source: 772a3783d6f19e4060b37fe43076b963cfd8d96e
| default to prevent surprising information leaks in custom builds of | ||
| TensorBoard. | ||
| """ | ||
| self._flags = context.flags |
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.
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.
Yeah, I agree it's fine for now, although if you don't mind editing #2801 after this goes in so it reflects the current state that'd be helpful.
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.
Will do.
wchargin-branch: environment-debug-info wchargin-source: e55a96e2e1623792787aebc9603c54da67ad6e85
nfelt
left a comment
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.
Let's also update http_api.md?
https://github.com/tensorflow/tensorboard/blob/master/tensorboard/http_api.md#dataenvironment
Maybe worth specifying there and possibly in the code that the format of the debug payload is subject to change and intended for human consumption, not machine consumption, so that we can change it in the future without being told we broke someone's workflow.
| default to prevent surprising information leaks in custom builds of | ||
| TensorBoard. | ||
| """ | ||
| self._flags = context.flags |
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.
Yeah, I agree it's fine for now, although if you don't mind editing #2801 after this goes in so it reflects the current state that'd be helpful.
| ) | ||
| if self._include_debug_info: | ||
| environment["debug"] = { | ||
| "version": version.VERSION, |
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.
Maybe worth including version unconditionally at the top level? That seems safe, and it's more structured than the rest, so we could exempt it from the "human consumption only" constraint in case we do want to render it in the frontend.
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.
Sure; done. The Python version can be read from the Server response
header already, so I don’t see much harm in exposing the TensorBoard
version.
wchargin-branch: environment-debug-info wchargin-source: 35e9a6c306c132075e402bf6d74878237b65c806
wchargin-branch: environment-debug-info wchargin-source: 35e9a6c306c132075e402bf6d74878237b65c806
wchargin
left a comment
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.
Let's also update http_api.md? […]
Done; thanks.
| default to prevent surprising information leaks in custom builds of | ||
| TensorBoard. | ||
| """ | ||
| self._flags = context.flags |
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.
Will do.
| ) | ||
| if self._include_debug_info: | ||
| environment["debug"] = { | ||
| "version": version.VERSION, |
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.
Sure; done. The Python version can be read from the Server response
header already, so I don’t see much harm in exposing the TensorBoard
version.
Summary:
In #4751, we implemented
data_locationsupport for RustBoard, whichmeans that the location no longer includes the gRPC address. But while
this is an improvement for users, the address was convenient for
development, and there’s no other way to get it from the UI. This patch
augments
/data/environmentto dump some basic debug info: theTensorBoard version, the data provider, and the command-line flags.
This behavior can be turned off. It’s enabled in
tensorboard(1), butdisabled by default on the
CorePluginandCorePluginLoaderAPIs.Thus, users who build their own TensorBoards will not find themselves
unexpectedly leaking information about their setup. For normal users of
tensorboard(1), the flags are fine to render; the only potentiallysensitive thing is the logdir path, which is of course already
prominent.
Sample response, passed through
jq .:Test Plan:
Manually navigated to
/data/environmentand verified that the flagsrender and the data provider appears for both
--load_fastand thelegacy multiplexer. Checked that when the change to
default.pyisreverted, no debug data shows up.
wchargin-branch: environment-debug-info