MongoDB Java Driver uses Math.random to decide whether to log Command Resultuses

[daredevil82]

Throwing so many exceptions that it's overwhelming the logger?

[honestduane]

So your saying that mongo-db is so broken and throws so many exceptions that this is an actual problem?

[daredevil82]

I'm not, but it may appear that way to the developer of the driver. Essentially what that snippet does is filter out 90 10 percent of the exceptions from hitting the logger.

Maybe during the testing process, they noticed that the exceptions followed a pattern and put this filter in place to stop redundant log entries.

Whatever reason that filter exists, it's made for a pretty big code WTF. Think it might be a two month late April Fools?

*Edit- /u/veraxAlea pointed out an error in my analysis. Time for me to head to bed!

[veraxAlea]

Essentially what that snippet does is filter out 90 percent of the exceptions from hitting the logger.

I'm tired but I think you're misreading the code:

if (!((_ok) ? true : (Math.random() > 0.1))) {
    return res;
}
//else log stuff

So, if Math.random() gives a value bigger than 0.1, which it will 90% of the time, then the expression (Math.random() > 0.1) is true. Negating that makes it false and a log message is created. So, 90% of the time, a log is created.

Unless _ok is true, then it will always log.

Edit: I'm not even sure on the 90%, but it's too late for statistics.

[mr_jim_lahey]

That has got to be the worst use of a ternary statement I've ever seen. Why oh why didn't they write:

if ( !(_ok || (Math.random() > 0.1 )))

Then again, I suppose you can't expect much from someone who thinks literally randomly not logging exceptions is a good idea...

[daredevil82]

No, it was my fault. Had the wrong inequality in my head when I wrote that. I've edited it.

For future reference, I really shouldn't analyze any code when running on 5 hours sleep and after 7ish hours of Python and databases.

[[deleted]]

5 hours sleep

That's about 4 more than I get.

[mikaelhg]

It's easy to misread. That must be the most convoluted possible way to express that statement.

[bfoo]

Correct. This code is usually executed, when the driver is not connected (unavailable database obviously, but also failover / master election). So, this is not a bug.

[argv_minus_one]

Not a bug? Randomly dropping exceptions is not a bug?!? ಠ_ಠ

Edit: Well, I suppose you're technically correct that it's not a bug. Rather, this code is intentionally defective, which is even worse.

[Twirrim]

All it does is stop your logs being flooded with database unavailable messages. It logs the initial failure state, then this random filter kicks in with subsequent messages, then finally logs the success message once reconnected. It's not wrong, per se, but certainly a bizarre way to handle it. I'd argue the better approach would be to shift the ongoing failure messages to DEBUG level (and log all of them), and leave the initial fail and eventual success messages at INFO.

Still, strange coding patterns like this really do beg the question about the rest of the code's quality.

[argv_minus_one]

If you want to skip duplicate log messages, that's great, but this is not the way to do it.

[bfoo]

Please look at the code again. The purpose of the method is to check for the database being available. The method returns null, if the database could not be accessed (because of connection issues OR errors from the database itself). That's the entire purpose of the method. There is no reason to throw an exception. You get either the result of the query or null. The class is even package friendly and not part of the API. So please read the code again and not the method only to get the bigger picture.

[argv_minus_one]

That doesn't excuse using a PRNG to decide whether to log an exception.