Skip to content

logging: optional details for ASSERT #3934

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Aug 3, 2018
Merged

logging: optional details for ASSERT #3934

merged 2 commits into from
Aug 3, 2018

Conversation

alyssawilk
Copy link
Contributor

@alyssawilk alyssawilk commented Jul 23, 2018 •
edited
Loading

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

ggreenway
ggreenway previously approved these changes Jul 23, 2018
View reviewed changes
mattklein123
mattklein123 previously approved these changes Jul 23, 2018
View reviewed changes
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.

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
Copy link
Contributor Author

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
Copy link
Member

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

@alyssawilk
Copy link
Contributor Author

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
Copy link
Member

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

@htuch
Copy link
Member

htuch commented Jul 24, 2018

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
Copy link
Member

htuch commented Jul 24, 2018

@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.

@htuch htuch self-assigned this Jul 24, 2018
@alyssawilk
Adding optional logs for asserts
14cf57b 
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@alyssawilk
Copy link
Contributor Author

@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. :-)

@alyssawilk alyssawilk changed the title logging: Requiring details for RELEASE_ASSERT logging: optional details for ASSERT Jul 25, 2018
@htuch
Copy link
Member

htuch commented Jul 25, 2018

@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
mattklein123 reviewed Jul 26, 2018
View reviewed changes
source/common/network/connection_impl.cc Outdated
@@ -504,7 +504,7 @@ void ConnectionImpl::onWriteReady() {
}

void ConnectionImpl::setConnectionStats(const ConnectionStats& stats) {
ASSERT(!connection_stats_);
ASSERT(!connection_stats_, "Perhaps there are duplicate filters in your configuration?");
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

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
Copy link
Member

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.

@alyssawilk
updated comment
49f9a9e 
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
mattklein123
mattklein123 approved these changes Aug 2, 2018
View reviewed changes
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 the updated comment!

htuch
htuch approved these changes Aug 3, 2018
View reviewed changes
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Nice preprocessor hackery.

@htuch htuch merged commit fa628c4 into envoyproxy:master Aug 3, 2018
@alyssawilk alyssawilk deleted the fix_assert branch November 28, 2018 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattklein123 mattklein123 mattklein123 approved these changes

@ggreenway ggreenway ggreenway left review comments

@lizan lizan Awaiting requested review from lizan

@zuercher zuercher Awaiting requested review from zuercher

+1 more reviewer

@htuch htuch htuch approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@htuch htuch

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@alyssawilk @mattklein123 @htuch @ggreenway