logging: Requiring details for RELEASE_ASSERT #3842

alyssawilk · 2018-07-11T18:09:08Z

After a whole bunch of macro shenanigans (several variants uploaded) replacing RELEASE_ASSERT(CONDITIONAL) with RELEASE_ASSERT(CONDITIONAL, REASON)

The really ugly VA_ARG macros would allow folks to easily slip in RELEASE_ASSERTS without details, and there were only 133 RELEASE_ASSERTS in the code base.

For folks with downstream RELEASE_ASSERTS I suggest
for file in grep -rIl RELEASE_ASSERT; do sed -i 's/(RELEASE_ASSERT.).;/\1, "");/' $file; done
with a bonus grep -rIl "RELEASE_ASSERT[^;]$" to track down multi-line stragglers.

Risk Level: Medium (may break downstream builds)
Testing: All tests pass, new assert test.
Docs Changes: n/a
Release Notes: n/a

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk · 2018-07-11T18:10:23Z

For anyone interested in the details, the first pass with RELEASE_ASSERT_SELECTOR was fairly readable, but unfortunately did early eval of X, which meant error messages went from IMO more
useful rc != EAGAIN to rc != 11.

After [more time than I care to admit] playing with the C++ preprocessor I got a monster series of about 8 more macros that did exactly what I wanted it to do, and then spent another [still longer than I want to admit] trying and failing to simplify it without unsetting the delicate balance of preprocessing order. Having utterly failed to do that, I opted to keep things simple and just resorted to sed. :-P

I'm totally amenable to reverting to either prior version so feel free to bikeshed away :-)

alyssawilk · 2018-07-11T18:10:50Z

@htuch @ggreenway @mattklein123 as having shown interest on #3829

ggreenway · 2018-07-11T18:15:01Z

source/common/common/assert.h

      ENVOY_LOG_TO_LOGGER(Envoy::Logger::Registry::getLog(Envoy::Logger::Id::assert), critical,    \
-                          "assert failure: {}", #X);                                               \
+                          "assert failure: {}.{}{}", #X,                                           \
+                          details.length() == 0 ? "" : " Details: ", DETAILS);                     \


details.empty()

Also, s/DETAILS/details/.

ggreenway · 2018-07-11T18:16:42Z

source/common/common/assert.h

+ *
+ * The new style of release assert is of the form
+ * RELEASE_ASSERT(foo == bar, "reason foo should actually be bar");
+ * new uses of RELEASE_ASSERT are strongly encouraged to supply a verbose


I think this comment applies to one of your previous iterations. This is a required param of the macro, right?

I think this is correct, explaining the old macro and legacy ""s. I've made wording stronger for "include details already" :-)

ggreenway · 2018-07-11T18:17:18Z

source/common/common/assert.h

  do {                                                                                             \
    if (!(X)) {                                                                                    \
+      std::string details = DETAILS;                                                               \


Could this be an absl::string_view to avoid copying? Or does that break things?

Why do you want to optimize copy on a termination path?!

I would recommend writing this as const std::string& details = (DETAILS);

Ha, good point; I think that comment just came out of habit.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk · 2018-07-11T19:32:56Z

(still looking at coverage failure)

htuch

Nice

htuch · 2018-07-11T19:48:50Z

source/common/common/assert.h

  do {                                                                                             \
    if (!(X)) {                                                                                    \
+      std::string details = DETAILS;                                                               \


I would recommend writing this as const std::string& details = (DETAILS);

htuch · 2018-07-11T19:49:35Z

source/common/common/assert.h

      ENVOY_LOG_TO_LOGGER(Envoy::Logger::Registry::getLog(Envoy::Logger::Id::assert), critical,    \
-                          "assert failure: {}", #X);                                               \
+                          "assert failure: {}.{}{}", #X,                                           \
+                          details.length() == 0 ? "" : " Details: ", DETAILS);                     \


Also, s/DETAILS/details/.

htuch · 2018-07-11T19:52:13Z

source/common/common/assert.h

 */
-#define RELEASE_ASSERT(X)                                                                          \
+#define RELEASE_ASSERT(X, DETAILS)                                                                 \


Do you want ... and __VA_ARGS__ here to support arbitrary fmtlib?

I figured we'd have folks do fmt:: inline as I did in the unit tests.

I think it's like a one line change in the macro to have it do the magic for the user (and default to something sane), e.g. (untested):

#define RELEASE_ASSERT(X, DETAILS,...) \ ... ENVOY_LOG_TO_LOGGER(Envoy::Logger::Registry::getLog(Envoy::Logger::Id::assert), critical, "assert failure: {}.{}{}", #X, details.empty() ? "" : " Details: ", fmt::format(details, ##__VA_ARGS__)); \ ...

see https://gcc.gnu.org/onlinedocs/cpp/Variadic-Macros.html

I'm not convinced it's that simple. It could be you have some minor typo, but adding in the vargs as you proposed lands me an
assert error argument index out of range
Meanwhile I find
{ RELEASE_ASSERT(0 == EAGAIN, fmt::format("using {}", "fmt")); },
more readable than
{ RELEASE_ASSERT(0 == EAGAIN, "native {}", "fmt"); },
because it's not inherently a logging macro.

mattklein123

LGTM once tests are passing. Nice!

ggreenway

LGTM

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

lizan · 2018-07-11T23:14:06Z

Somehow there is a missed RELEASE_ASSERT, master doesn't build...
https://github.com/envoyproxy/envoy/blob/master/source/extensions/filters/network/thrift_proxy/transport_impl.cc#L63

* origin/master: config: making v2-config-only a boolean flag (envoyproxy#3847) lc trie: add exclusive flag. (envoyproxy#3825) upstream: introduce PriorityStateManager, refactor EDS (envoyproxy#3783) test: deflaking header_integration_test (envoyproxy#3849) http: new style WebSockets, where headers and data are processed by the filter chain. (envoyproxy#3776) common: minor doc updates (envoyproxy#3845) fix master build (envoyproxy#3844) logging: Requiring details for RELEASE_ASSERT (envoyproxy#3842) test: add test for consistency of RawStatData internal memory representation (envoyproxy#3843) common: jittered backoff implementation (envoyproxy#3791) format: run buildifier on .bzl files. (envoyproxy#3824) Support mutable metadata for endpoints (envoyproxy#3814) test: deflaking a test, improving debugability (envoyproxy#3829) Update ApiConfigSource docs with grpc_services only for GRPC configs (envoyproxy#3834) Add hard-coded /hot_restart_version test (envoyproxy#3832) healthchecks: Add interval_jitter_percent healthcheck option (envoyproxy#3816) Signed-off-by: Snow Pettersen <snowp@squareup.com>

Allowing ASSERT to optionally use the details added to RELEASE_ASSERT in #3842 Risk Level: Low Testing: bazel //test/..., unit tests of new and old behavior. Docs Changes: n/a Release Notes: n/a Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk added 4 commits July 10, 2018 16:39

Reworking release assert

31b7f0f

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

version one, EAGAIN -> 0

396a60d

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Probably too much macro black magic

43fd393

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Simplifying.

6c608ed

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk requested a review from zuercher as a code owner July 11, 2018 18:09

ggreenway requested changes Jul 11, 2018

View reviewed changes

reviewer comments

fbb06d0

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

htuch assigned mattklein123, ggreenway and htuch Jul 11, 2018

htuch suggested changes Jul 11, 2018

View reviewed changes

mattklein123 reviewed Jul 11, 2018

View reviewed changes

ggreenway previously approved these changes Jul 11, 2018

View reviewed changes

Tests + comments

a3143dd

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk dismissed ggreenway’s stale review via a3143dd July 11, 2018 21:04

alyssawilk force-pushed the logging branch from 5684246 to a3143dd Compare July 11, 2018 21:04

htuch approved these changes Jul 11, 2018

View reviewed changes

ggreenway approved these changes Jul 11, 2018

View reviewed changes

mattklein123 approved these changes Jul 11, 2018

View reviewed changes

htuch merged commit 6881200 into envoyproxy:master Jul 11, 2018

lizan mentioned this pull request Jul 11, 2018

build: fix RELEASE_ASSERT missed in #3842 #3844

Merged

alyssawilk mentioned this pull request Jul 23, 2018

logging: optional details for ASSERT #3934

Merged

alyssawilk deleted the logging branch November 28, 2018 16:02

logging: Requiring details for RELEASE_ASSERT #3842

logging: Requiring details for RELEASE_ASSERT #3842

Uh oh!

Conversation

alyssawilk commented Jul 11, 2018

Uh oh!

alyssawilk commented Jul 11, 2018

Uh oh!

alyssawilk commented Jul 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented Jul 11, 2018

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch Jul 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

lizan commented Jul 11, 2018

Uh oh!

Uh oh!

htuch Jul 11, 2018 •

edited

Loading