[SERVER-47926] Less verbose uassert Created: 04/May/20  Updated: 12/Nov/20  Resolved: 12/Nov/20

Status: Closed
Project: Core Server
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Improvement Priority: Major - P3
Reporter: Lingzhi Deng Assignee: Benjamin Caimano (Inactive)
Resolution: Duplicate Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Duplicate
duplicates SERVER-48922 Create internal assertion macro Closed
duplicates SERVER-44570 Create a non process-fatal variant of... Closed
Related
related to SERVER-48688 Improving diagnostics around user ass... Closed
is related to SERVER-47881 Make ReplicationCoordinatorImpl::getL... Closed
Sprint: Service arch 2020-05-18
Participants:

 Description   

Currently, uasserts log at debug level D1. As we move to more use of exceptions to deal with error handling, we should probably consider adjusting our logging policy for uasserts. Maybe we should only log when we're returning an error to the user instead of whenever we throw an exception.



 Comments   
Comment by Lingzhi Deng [ 12/Nov/20 ]

Sounds good to me. Thanks for the work!

Comment by Benjamin Caimano (Inactive) [ 12/Nov/20 ]

Alright, I'm closing this as a duplicate of both SERVER-48922 and SERVER-44570. We now have both less verbose uassert and more verbose uassert. lingzhi.deng, if you feel that we haven't met your needs, please let me know either by reopening this ticket or filing a new one.

Comment by Amirsaman Memaripour [ 25/Jun/20 ]

Now that we have internalAssert (SERVER-48922), can we close this ticket?

Comment by Benjamin Caimano (Inactive) [ 19/May/20 ]

Apologies on the slow response, it's been a busy couple of weeks.

Laying out my thoughts:

  • This ticket as written seems like a natural pairing with the UserAssertAfterLog tag, which can be passed to any level of LOGV2 statement. Sadly, when I traced the code path, it looks like perhaps that tag will cause us to log twice, once at the desired level and once at D1 (see the raw uasserted here). We could fix that up pretty easily as a way to offer "internal" user assertions that log at a desired level.
  • I think the concept of an internal-only exception pattern seems like it has been useful to Replication. ServiceArch probably has an equivalent usage of exceptions with CallbackCanceled. If we do go this route, I think I'd want something that throws a new type. That new type could be derived from AssertionException or just the ExceptionForCat for a new ErrorCategory (see here). This would allow us to make a separate code path when error handling and potentially ban these errors from reaching the user. We could probably quick win this kind of work simultaneously with PM-1620.
  • Anything we do along these lines will be awkward to integrate with uassertStatusOK. Sometimes people catch-throw, futurize, or otherwise convert between Status and DBException. As is, I believe we log some errors multiple times because of this. The least intrusive thing I can think of is to reuse uassert but only log and increment the counter if the ErrorCode involved isn't in a special ErrorCategory.
Comment by Andy Schwerin [ 04/May/20 ]

Hmmm... iassert? Having the exceptions built and thrown by a helper has been useful.

Comment by Lingzhi Deng [ 04/May/20 ]

Yes, after reading Kelsey's comment in SERVER-47881, I think this makes sense.

Comment by Eric Milkie [ 04/May/20 ]

I don't think we need to change the behavior of uassert; I think we need to not abuse them as ways of throwing internal exceptions. Instead, we should be using "throw" in these situations.
The u in uassert is for "user" – that is, this is a problem that needs to always be elevated all the way back to the client and the user. It shouldn't be caught and converted into something else. Note that we also have a counter for uasserts that shows up in Atlas and is useful for triaging. We shouldn't be suppressing uasserts by catching them since the counter will still be incremented.

Generated at Thu Feb 08 05:15:38 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.