logging: optional details for ASSERT

[alyssawilk]

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

[mattklein123]

FWIW, this seems a bit less worth it to me than with RELEASE_ASSERT, since in many cases I think these types of assets are pretty obvious. Do we want to make this required? What's the thinking here?

[alyssawilk]

internal bug: "assert failure: !connection_stats_"
which turns out is what happens when you have 2 http connection manager filters configured for the same listener. Obviously we want to catch that at start-up rather than ASSERT on live traffic when queries come in. In the interim I also wanted to put an explanatory message at !connection_stats_ that one should examine the filter chain for duplicates since there looks like there was redundant work done but alas there was no place to add details.

[mattklein123]

What about for the debug ASSERT making the string optional but not requiring it? WDYT?

[alyssawilk]

Sure, given we're already passing X into a submacro so don't have the early evaluation problem, I can do and resurect my vararg magic I rejected from the RELEASE_ASSERT PR. Probably tomorrow though :-)

[mattklein123]

If it's easier you could also just do VERBOSE_ASSERT() or something like that. Whatever is easiest.

[htuch]

I'm not sure if we should do this by default. ASSERTs are intended to be lightweight self-documenting statements of invariants, so I would prefer we kept them by default lightweight. A VERBOSE_ASSERT is better IMHO for opt-in logging.

[htuch]

@alyssawilk that specific example is a place we should just have better config validation. So, I don't think the right fix is to patch up the ASSERT there.

[alyssawilk]

@htuch I already agreed to make it optional, just ended up out yesterday so didn't get it shipped.
w.r.t. the error in question I suspect we all agree it should be handled by config validation but do you think it's a bad thing to have a useful error message in the interim? If not I think we're in agreement again. :-)

[htuch]

@alyssawilk yeah, I see. I think VERBOSE_ASSERT is OK, but I would prefer to just fix TBH, since this won't apply to release builds.

[mattklein123]

Maybe this is what @htuch was asking about, but from a quick glance this seems non-obvious to me. How is this assert hit if there are duplicate filters?

[alyssawilk]

If you configure 2 http connection manager filters, each one has initialzeReadFitlerCallbacks called on it and calls setConnectionStats. The second setConnectionStats causes an assert fail.

As htuch@ says the real fix is to detect the broken config on start-up but if two filters both setConnectionStats it's nicer IMO to get an explanation than just an assert fail.

[alyssawilk]

ditto 2 tcp filters in a row. I guess if you had a tcp and an hcm it would as well, so I could try for a more clear explanation that 2 filters are both trying to own the connection stats and it's some sort of borked filter chain.

[mattklein123]

I could try for a more clear explanation that 2 filters are both trying to own the connection stats and it's some sort of borked filter chain.

Yeah if you are willing to try for a more fleshed out comment that would be helpful.

[mattklein123]

Thanks for the updated comment!

[htuch]

Nice preprocessor hackery.