Skip to content

test: in coverage runs, log stacktraces directly to stderr#10397

Merged
mattklein123 merged 15 commits intoenvoyproxy:masterfrom
jmarantz:btrace-stderr
Mar 17, 2020
Merged

test: in coverage runs, log stacktraces directly to stderr#10397
mattklein123 merged 15 commits intoenvoyproxy:masterfrom
jmarantz:btrace-stderr

Conversation

@jmarantz
Copy link
Contributor

@jmarantz jmarantz commented Mar 15, 2020

Description:
Coverage tests run with -l trace --log-path /dev/null, in order to ensure that all of the code-paths from the maximum level of tracing are covered in tests, but we don't wind up filling up CI with useless detailed artifacts.

The downside of this is that if there's a crash, the backtrace is lost, as the backtracing mechanism uses logging. This PR resolves this by redirecting crash logs to stderr for coverage tests only.

Forcing a segv in a test, I see this from coverage with this PR:

[ RUN      ] IpVersions/MainCommonTest.ConstructDestructHotRestartDisabledNoInit/IPv4
#0 Envoy::SignalAction::sigHandler() [0xfc1a564]
#1 __restore_rt [0x7f3da788f390]
#2 __llvm_coverage_mapping [0x53f0501]
#3 __llvm_coverage_mapping [0x53f02f9]
#4 std::__1::operator<< <>() [0x542c838]
#5 Envoy::MainCommonBase::run() [0x7b84ef0]
#6 Envoy::MainCommon::run() [0x7b63693]
#7 Envoy::MainCommonTest_ConstructDestructHotRestartDisabledNoInit_Test::TestBody() [0x7b5a9cf]
#8 testing::internal::HandleSehExceptionsInMethodIfSupported<>() [0x10fabfc6]
#9 testing::internal::HandleExceptionsInMethodIfSupported<>() [0x10f98501]
#10 testing::Test::Run() [0x10f82dd2]
#11 testing::TestInfo::Run() [0x10f838d8]
#12 testing::TestSuite::Run() [0x10f84049]
#13 testing::internal::UnitTestImpl::RunAllTests() [0x10f91056]
#14 testing::internal::HandleSehExceptionsInMethodIfSupported<>() [0x10fb0e66]
#15 testing::internal::HandleExceptionsInMethodIfSupported<>() [0x10f9b321]
#16 testing::UnitTest::Run() [0x10f90aa0]
#17 RUN_ALL_TESTS() [0xf6bc105]
#18 Envoy::TestRunner::RunTests() [0xf6bb8db]
#19 main [0xf6b9f98]
#20 __libc_start_main [0x7f3da74d4830]
external/bazel_tools/tools/test/collect_coverage.sh: line 128: 17456 Segmentation fault      (core dumped) "$@"

Note that this was tested manually; we do not have a test for this test behavior.

Risk Level: medium -- mostly because this messes with the crash handler
Testing: //test/..., plus a manual coverage run with a segv injected.
Docs Changes: n/a -- this does not add an option for envoy servers; just for tests.
Release Notes: n/a

Signed-off-by: Joshua Marantz <jmarantz@google.com>
@mattklein123 mattklein123 self-assigned this Mar 15, 2020
Signed-off-by: Joshua Marantz <jmarantz@google.com>
jmarantz added 10 commits March 15, 2020 17:53
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
… from coverage script.

Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz jmarantz changed the title in coverage runs, log stacktraces directly to stderr. test: in coverage runs, log stacktraces directly to stderr Mar 16, 2020
@jmarantz jmarantz marked this pull request as ready for review March 16, 2020 22:11
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks for working on this. This is going to be super useful in figuring out our coverage issues. A few small comments.

/wait

return return_value;
}

bool findAndRemoveExact(absl::string_view option, int& argc, char**& argv) {
Copy link
Member

Choose a reason for hiding this comment

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

Should we just use TCLAP here? It seems like we are reimplementing a bunch org arg parsing for tests. Also, is there any reason to make this a real server option with documentation? Probably not but just throwing it out there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could; actually I have that in a patch saved away if we want to go in that direction.

OTOH we could use the TCLAP library to do this parsing rather than do this manually; I just copied the loop from above. I don't know if we can replace the one above because it uses the --name=value syntax rather than --name value, so I was trying to use a parallel approach but I'm not wedded to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually there's a third option I considered, using #ifdef ENVOY_CONFIG_COVERAGE and not making it a command-line option at all. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

+1 for simple. :)

Copy link
Member

Choose a reason for hiding this comment

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

(I think you would have to ifdef out that test which checks the log but that is probably fine)

bool BackwardsTrace::log_to_stderr_ = false;

void BackwardsTrace::setLogToStderr(bool log_to_stderr) {
std::cerr << "setting log_to_stderr_=" << log_to_stderr << std::endl;
Copy link
Member

Choose a reason for hiding this comment

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

nit: prepend with BackwardsTrace or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do; or I could just delete it.

Copy link
Member

Choose a reason for hiding this comment

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

Sure either way

… an ifdef.

Signed-off-by: Joshua Marantz <jmarantz@google.com>
mattklein123
mattklein123 previously approved these changes Mar 17, 2020
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Awesome, thank you!!!

Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
Copy link
Contributor Author

Sorry can you re-approve? Formatting issue needed to be fixed.

@mattklein123 mattklein123 merged commit a2e21a9 into envoyproxy:master Mar 17, 2020
@jmarantz jmarantz deleted the btrace-stderr branch March 17, 2020 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants