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

Fatal error logging followup fixes #6610

Merged
merged 13 commits into from Oct 10, 2018

Conversation

Projects
None yet
4 participants
@cosmicexplorer
Copy link
Contributor

cosmicexplorer commented Oct 9, 2018

Problem

This PR contains several deemed-necessary-by-me-personally fixes to fatal error logging which weren't put into #6552 because that diff was already massive. This should be considered the same PR as that one.

Solution

  • Default the logs/ directory to be .pants.d/logs so we don't litter the buildroot with a top-level logs/ directory.
  • Add a timestamp to any fatal errors printed to the terminal to more easily debug race conditions.
  • Add the process title (from setproctitle) to the fatal error log message (this makes it more clear when the process is pantsd, compared to the sys.argv, which are arbitrary).
  • Remove any mention of exception stacktraces from Exiter.
  • Add ExceptionSink.reset_should_print_backtrace() (with a new mutable field _should_print_backtrace) to determine whether to print stacktraces to the terminal on exit.
  • Testing for all of the above.

cosmicexplorer added some commits Oct 9, 2018

@cosmicexplorer cosmicexplorer requested review from stuhood and ity Oct 9, 2018

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:logging-followup-fixes branch from 5b70885 to 6276262 Oct 9, 2018

cosmicexplorer added some commits Oct 9, 2018

@stuhood

stuhood approved these changes Oct 9, 2018

Copy link
Member

stuhood left a comment

Would remove the debug output; otherwise, this looks good. Thanks!

pid: {pid}
{message}
"""

@classmethod
def _format_exception_message(cls, msg, pid):
if cls._bootstrap_option_values:

This comment has been minimized.

@stuhood

stuhood Oct 9, 2018

Member

Rather than individually poking at fields of the bootstrap options, it would be good to capture the fields you need in set_bootstrap_options, and to have defaults for those fields.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Oct 9, 2018

Contributor

Truly fantastic idea. Will do.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Oct 9, 2018

Contributor

Done!

@@ -27,6 +27,9 @@ class OptionValueContainer(object):
def __init__(self):
self._value_map = {} # key -> either raw value or RankedValue wrapping the raw value.

def _debug_dump(self):

This comment has been minimized.

@stuhood

stuhood Oct 9, 2018

Member

Adding this for a test is fine, but it shouldn't be called in other consuming code.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Oct 9, 2018

Contributor

This can be removed if we avoid keeping a copy of all the bootstrap values.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Oct 9, 2018

Contributor

Removed!

pid: {pid}
{message}
"""

@classmethod
def _format_exception_message(cls, msg, pid):
if cls._bootstrap_option_values:
bootstrap_fmt = cls._bootstrap_option_values._debug_dump()

This comment has been minimized.

@stuhood

stuhood Oct 9, 2018

Member

This is going to be a bit too noisy probably?

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Oct 9, 2018

Contributor

It totally is, and this is why the testing has the nonce thing at all. I think the first suggestion you described (to capture the relevant fields) is the one to go for here, that obviates the _debug_dump() method.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Oct 9, 2018

Contributor

Removed!

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:logging-followup-fixes branch from a62ba4c to fff15f0 Oct 9, 2018

@ity

ity approved these changes Oct 9, 2018

Copy link
Member

ity left a comment

this is looking good to me overall - looks like some legit failures in travis ci though

@baroquebobcat
Copy link
Contributor

baroquebobcat left a comment

Looks good to me. I've got a couple minor nits.

options_bootstrapper = OptionsBootstrapper(env=self._env, args=self._args)
bootstrap_options = options_bootstrapper.get_bootstrap_options()
global_bootstrap_options = bootstrap_options.for_global_scope()
ExceptionSink.reset_should_print_backtrace_to_terminal(

This comment has been minimized.

@baroquebobcat

baroquebobcat Oct 10, 2018

Contributor

nit: I think whitespace between the ExceptionSink commands and the options bootstrapping would make this a little easier to read.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Oct 10, 2018

Contributor

Done!

pants_run.stderr_data)
self.assertRegexpMatches(pants_run.stderr_data, """\\A\
timestamp: ([^\n]+)
Exception caught: \\(<class 'pants\\.build_graph\\.address_lookup_error\\.AddressLookupError'>\\)

This comment has been minimized.

@baroquebobcat

baroquebobcat Oct 10, 2018

Contributor

It'd be nice if the error could omit the <class ', '> bits and look more like Exception caught: (pants.build_graph.address_lookup_error.AddressLookupError) (backtrace omitted).

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Oct 10, 2018

Contributor

Oooooh, I totally agree with this one.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Oct 10, 2018

Contributor

Done!

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:logging-followup-fixes branch from eb76442 to 6dd674c Oct 10, 2018

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:logging-followup-fixes branch from 6dd674c to 6056baa Oct 10, 2018

@cosmicexplorer cosmicexplorer merged commit 7205bc1 into pantsbuild:master Oct 10, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

cosmicexplorer added a commit that referenced this pull request Oct 10, 2018

Fatal error logging followup fixes (#6610)
### Problem

This PR contains several deemed-necessary-by-me-personally fixes to fatal error logging which weren't put into #6552 because that diff was already massive. This should be considered the same PR as that one.

### Solution

- Default the `logs/` directory to be `.pants.d/logs` so we don't litter the buildroot with a top-level `logs/` directory.
- Add a timestamp to any fatal errors printed to the terminal to more easily debug race conditions.
- Add the process title (from `setproctitle`) to the fatal error log message (this makes it more clear when the process is pantsd, compared to the `sys.argv`, which are arbitrary).
- Remove any mention of exception stacktraces from Exiter. 
- Add `ExceptionSink.reset_should_print_backtrace()` (with a new mutable field `_should_print_backtrace`) to determine whether to print stacktraces to the terminal on exit.
- Testing for all of the above.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment