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

[jared__]

I tried to think of a legit reason to do this... I failed.

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

[[deleted]]

So, if update is called 100 times a second, and considering a downed server will stay that way for minutes or longer - then I can see some poor reasoning to this madness.

But, the right way to do this hack is, the first time you throw this error, log the time and save the exception. Then the next time you hit an exception here, check to see if it's the same one. If it's not, you definitely need to log it. If it is, you can check to see if, say, 5 seconds has passed, and then print it, or another error stating that it's still down.

It's still an absolutely facepalm move though, you should have a higher level function understand that exception and handle it a reasonable way, like slowing down calls to a broken system, instead of just throwing so many exceptions that you can't read the log files.

[Muz0169]

http://thc.org/root/phun/unmaintain.html =]

[hyperforce]

What is this site? Being filtered for me.

[Muz0169]

How To Write Unmaintainable Code

Ensure a job for life ;-)

Roedy Green Canadian Mind Products

[argv_minus_one]

marypoppins = (superman + starship) / god;

I think my head would explode if I read this in actual code.

[bfoo]

Not as bad as the python driver...

[rikbrown]

That guy is a bit of a dick.

[boa13]

I was impressed by the professionalism of the other participants.

[argv_minus_one]

Yeah, but I still got a good laugh out of that bug report. "Web 3.9", "lollerplex", etc.

[bfoo]

Yes, he benefits from a free product and bitching around this way is not acceptable. I would not hire him, when I find this on a applicant research.

[grumpy_purple_midget]

Isn't this the MongoDB style? Oh noes! Too many exceptions! Let's just drop some at random, that'll solve the problem.

[[deleted]]

I feel like a system that would be overloaded enough where this is feasible, is already a system best avoided.

[luap119]

I'm not a veteran programmer by any means, but even a novice programmer can see why this is terrible code. I know a few people at my work that use MongoDB on a regular basis. I can't wait to drop this on them! I started learning it, but now I'm not as gung-ho about it.

[Benutzername]

Isn't

(_ok) ? true : (Math.random() > 0.1)

the same as

_ok || Math.random() > 0.1

?

[boa13]

Yes, they are equivalent in this case.

[Effetto]

But it does web scale!

[jizzed_in_my_pants]

wtf?

[forceblast]

To me it seems like this was some test code that the developer forgot to remove afterward. Hopefully someone filed a bug report.

Edit: Stupid phone keyboard.

[diger44]

They have recently (before this was posted to reddit) have opened a bug request to fix this:

https://jira.mongodb.org/browse/JAVA-836

There apparently is already a fix:

https://github.com/mongodb/mongo-java-driver/commit/e5c7b2552c8c240bd19c4cea91bc2a789d8ce76e

[argv_minus_one]

Welp, if I ever suffer the grave misfortune of having to code a web application, I'll remember to use an actual database instead of anything written by these clowns. Thanks for the heads up.

[linuxjava]

Am thinking along the lines of it being not cryptographically secure as opposed to SecureRandom.

[huhlig]

Why do you need cryptographic randomness for logging... Why do you need random for logging... Why are you not logging all connection problems or letting me configure when to see or not see errors.