When watchdog() is being called now the text is being expanded, so instead of using

Spam: @message

it is using

Spam:

This is causing issues with the mongodb watchdog because it groups all the watchdog entries together by the raw message which generally means that there is only 1 watchdog entry for Spam, Ham, and Unsure instead now with this change the majority of my main watchdog listing page is taken up with spam messages.

CommentFileSizeAuthor
#18 watchdog_not_being-1556682-18.patch4.37 KBPrimsi
#18 watchdog_not_being-1556682-18_TEST_ONLY.patch1.43 KBPrimsi
#16 mollom-1556682.patch2.94 KBPrimsi
#13 mollom-1556682.patch1.23 KBPrimsi
#5 mollom-1556682-2.patch1.1 KBRok Žlender
#1 mollom-1556682.patch1.1 KBgordon
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

gordon’s picture

gordon CreditAttribution: gordon commented
Status: Active » Needs review
FileSize
mollom-1556682.patch1.1 KB

I found that in the merging of the watchdog messages it was preprocessing the messages, removing the arguments and making the messages being posted as a single text string.

Ordinarily this would not be a problem, but using 3rd party watchdog tools like mongodb_watchdog which consolidates the log messages based upon the raw message (i.e. the message with the place holders still in tack "spam: %teaser") which makes it harder to manage the messages.

Also the side effect of this with mongodb because it creates a new collection for every new message text which will have ultimately 1 or 2 messages in it, was requiring that mongodb consumed a hugh amount of space.

I have made a change so that when it merges all the watchdog messages it will now merge both the message and the arguments. With the arguments I have put in some conflict detection and arguments will be renamed if there is going to be a conflict.

Status: Needs review » Needs work

The last submitted patch, mollom-1556682.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

We've introduced the new formatted log messages in #1000690: Revamp and reduce log messages (was: Not clear that fail-over is working)

The current log messages are always different, because we are grouping multiple requests to Mollom into a single message, but still want to see the full history. Thus, I'm not sure how this could use any kind of always identical placeholders to make Mongo happy...?

gordon’s picture

gordon CreditAttribution: gordon commented

Actually the patch that I have submitted, it gives the consolidated messages, but retains all the arguments so that mongodb can still consolidate, and I don't get a run away in space.

I need to take a look at the patch and see why it failed.

Rok Žlender’s picture

Rok Žlender CreditAttribution: Rok Žlender commented
Version: 6.x-2.0 » 7.x-1.x-dev
FileSize
mollom-1556682-2.patch1.1 KB

We got bitten by this as well mollom create about 21k collections since we rolled new version and are in process of applying this patch to production. There is a bug in the patch $new_arg is not defined I think you meant to use $new_key there attaching fixed up patch.

Rok Žlender’s picture

Rok Žlender CreditAttribution: Rok Žlender commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, mollom-1556682-2.patch, failed testing.

Rok Žlender’s picture

Rok Žlender CreditAttribution: Rok Žlender commented
Version: 7.x-1.x-dev » 7.x-2.x-dev
Status: Needs work » Needs review
Rok Žlender’s picture

Rok Žlender CreditAttribution: Rok Žlender commented

#5: mollom-1556682-2.patch queued for re-testing.

Rok Žlender’s picture

Rok Žlender CreditAttribution: Rok Žlender commented
Priority: Normal » Critical

Bumping this to critical no way to say how many sites out there use MongoDB and Mollom but for those they do this bug can crash their servers.

eshta’s picture

eshta CreditAttribution: eshta commented
Issue summary: View changes

I'm re-evaluating this to see if it is a release candidate (granted some time after initial post).... I'm not understanding the point of removing the call to _mollom_format_string because it is only sanitizing the data.

eshta’s picture

eshta CreditAttribution: eshta commented
Status: Needs review » Postponed (maintainer needs more info)
Primsi’s picture

Primsi CreditAttribution: Primsi commented
FileSize
mollom-1556682.patch1.23 KB

Rerolling #5.

eshta’s picture

eshta CreditAttribution: eshta commented
Status: Postponed (maintainer needs more info) » Needs review

-- changing status so that the patch gets run against testbots.

A small review:

+++ b/sites/all/modules/contrib/mollom/mollom.module
@@ -2580,8 +2580,21 @@ function _mollom_format_log(array $log) {
-        $message .= _mollom_format_string($entry['message'], $entry['arguments']);

This patch removes the only reference to _mollom_format_string(). The comments should be updated (they refer to it) and the _mollom_format_string() function should be removed.
---------
Also this is lacking any sort of test of the new conflicting arguments functionality.

Status: Needs review » Needs work

The last submitted patch, 13: mollom-1556682.patch, failed testing.

Primsi’s picture

Primsi CreditAttribution: Primsi commented
Status: Needs work » Needs review
FileSize
mollom-1556682.patch2.94 KB

Rerolled the patch on dev branch and addressed the review issues.

eshta’s picture

eshta CreditAttribution: eshta commented

This is great work :-)

Would love to see some more confirmation from folks running MongoDB (will take some extra time for me to get this one up and running to test locally).

Primsi’s picture

Primsi CreditAttribution: Primsi commented
FileSize
watchdog_not_being-1556682-18_TEST_ONLY.patch1.43 KB
watchdog_not_being-1556682-18.patch4.37 KB

I added a test to the patch. Hope that covers it enough.

The last submitted patch, 18: watchdog_not_being-1556682-18_TEST_ONLY.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 18: watchdog_not_being-1556682-18.patch, failed testing.