Log4php
  1. Log4php
  2. LOG4PHP-110

mongo database appender for log4php

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.0
    • Fix Version/s: 2.1.0
    • Component/s: Code, Documentation, Tests
    • Labels:
      None
    • Environment:

      PHP

      Description

      mongo database appender for log4php with unit test coverage

      1. throwable.path
        9 kB
        Vladimír Gorej
      2. src.tgz
        4 kB
        Vladimír Gorej

        Activity

        Hide
        Vladimír Gorej added a comment -

        Permission granted to include this code under apache license

        Show
        Vladimír Gorej added a comment - Permission granted to include this code under apache license
        Hide
        Vladimír Gorej added a comment -

        My final throwable patch; This patch is needed to include last version of Mongo appender into source code of log4php. ThrowableInformation class completelly refactored..Logger refrence is required to get custom renderer for Throwable(Exception). Patch has full coverage of unit tests for refactored code.

        Show
        Vladimír Gorej added a comment - My final throwable patch; This patch is needed to include last version of Mongo appender into source code of log4php. ThrowableInformation class completelly refactored..Logger refrence is required to get custom renderer for Throwable(Exception). Patch has full coverage of unit tests for refactored code.
        Hide
        Vladimír Gorej added a comment -

        Last version of Mongo database appender with integration test. This version needs the throwable.patch to be included into log4php appenders.

        Show
        Vladimír Gorej added a comment - Last version of Mongo database appender with integration test. This version needs the throwable.patch to be included into log4php appenders.
        Hide
        Christian Grobmeier added a comment -

        Throwable patch: I saw that you are using the Logger-Object to determine the Hierarchy. But you can get the Hierarchy with: Logger:getHierarchy because its static. I changed this and now we don't need the Logger-Reference itself anymore. You don't need to update your patch, I have already done it - just wanted to inform you

        More comments soon, cheers!

        Show
        Christian Grobmeier added a comment - Throwable patch: I saw that you are using the Logger-Object to determine the Hierarchy. But you can get the Hierarchy with: Logger:getHierarchy because its static. I changed this and now we don't need the Logger-Reference itself anymore. You don't need to update your patch, I have already done it - just wanted to inform you More comments soon, cheers!
        Hide
        Christian Grobmeier added a comment -

        I am not sure if we discussed this already... but what was the reason to exclude "arrays"? With the current patch one can only use an exception for a throwable. This makes sense from wording, but isn't there a need for putting arrays in there? Any other comments?

        Show
        Christian Grobmeier added a comment - I am not sure if we discussed this already... but what was the reason to exclude "arrays"? With the current patch one can only use an exception for a throwable. This makes sense from wording, but isn't there a need for putting arrays in there? Any other comments?
        Hide
        Vladimír Gorej added a comment -

        Hello Christian,

        The reason for getting rid of arrays as constructor param is that, i was studuying the log4j and log4php and I did not find any reasonable argument why arrays should be passed to the ThrowableInformation class as an constructor argument. The only throwable representantation in PHP is Exception and all its descendants. I see no reason why anybody would want to log arrays as throwables (Exception).

        My seconds argument concerns - public function Logger::forcedLog($fqcn, $caller, $level, $message)

        We can only tell if $caller is throwable (Exception) or not, by testing its type..if the $caller is array there is no way to tel if it is some custom user array or array that was
        created by conversion from Exception.

        If the caller is of type Exception the worflow is pretty straightforward.

        This is the link to original ThrowableInformation class : http://svn.apache.org/viewvc/logging/log4j/trunk/src/main/java/org/apache/log4j/spi/ThrowableInformation.java?view=markup
        Maybe developers implemented constructor public ThrowableInformation(final String[] r) just for testing purposes. I can look at it event deeper if you have still some doubts.

        Show
        Vladimír Gorej added a comment - Hello Christian, The reason for getting rid of arrays as constructor param is that, i was studuying the log4j and log4php and I did not find any reasonable argument why arrays should be passed to the ThrowableInformation class as an constructor argument. The only throwable representantation in PHP is Exception and all its descendants. I see no reason why anybody would want to log arrays as throwables (Exception). My seconds argument concerns - public function Logger::forcedLog($fqcn, $caller, $level, $message) We can only tell if $caller is throwable (Exception) or not, by testing its type..if the $caller is array there is no way to tel if it is some custom user array or array that was created by conversion from Exception. If the caller is of type Exception the worflow is pretty straightforward. This is the link to original ThrowableInformation class : http://svn.apache.org/viewvc/logging/log4j/trunk/src/main/java/org/apache/log4j/spi/ThrowableInformation.java?view=markup Maybe developers implemented constructor public ThrowableInformation(final String[] r) just for testing purposes. I can look at it event deeper if you have still some doubts.
        Hide
        Christian Grobmeier added a comment -

        Throwable patch will be included in next rv. I wanted to add it now, but ASF SVN is down at the moment - so tomorrow

        After careful consideration I agree that exception is the way to go, even when we have a slight drawback in backwards compatibility.

        However, while code review I have added a TODO in
        LoggerThrowableInforamtion::getStringRepresentation

        Vladimir, could you comment this please:

        if ($renderer instanceof LoggerRendererDefault)

        { $renderer = new LoggerRendererException(); }

        It feels wrong too me somehow

        Next step is the addition of the Mongo Database Appender - thanks for allt he work so far!

        Christian

        Show
        Christian Grobmeier added a comment - Throwable patch will be included in next rv. I wanted to add it now, but ASF SVN is down at the moment - so tomorrow After careful consideration I agree that exception is the way to go, even when we have a slight drawback in backwards compatibility. However, while code review I have added a TODO in LoggerThrowableInforamtion::getStringRepresentation Vladimir, could you comment this please: if ($renderer instanceof LoggerRendererDefault) { $renderer = new LoggerRendererException(); } It feels wrong too me somehow Next step is the addition of the Mongo Database Appender - thanks for allt he work so far! Christian
        Hide
        Vladimír Gorej added a comment -

        Thanks for the info Christian.

        Ok now the tricky part:

        1. $renderer = $this->logger->getHierarchy()>getRendererMap()>getByClassName(get_class($this->throwable));
        2. if ($renderer instanceof LoggerRendererDefault)

        { 3. $renderer = new LoggerRendererException(); 4. }

        I aggree that this 4 lines of code are kind a tricky...I wanted to implement a little hack for the users to supply custom renderers for custom
        exceptions. Use case example:

        Let us say we have this exception: class DaoException extends Exception { }

        Clinet of log4php api wants to render DaoException in other way that every other Exception.
        He implements and assigns DaoExceptionRenderer to the renderer map

        First line of code - we try to find custom exception renderer for current throwable context
        Second/Third line - if custom exception renderer doesn't exist and the rederer map returns default object renderer then render DaoException as standard exception (LoggerRendererException)

        • if it exists, render DaoException with custom exception renderer (DaoExceptionRenderer)

        I hope it is clear now, what I itended to do.

        Show
        Vladimír Gorej added a comment - Thanks for the info Christian. Ok now the tricky part: 1. $renderer = $this->logger->getHierarchy() >getRendererMap() >getByClassName(get_class($this->throwable)); 2. if ($renderer instanceof LoggerRendererDefault) { 3. $renderer = new LoggerRendererException(); 4. } I aggree that this 4 lines of code are kind a tricky...I wanted to implement a little hack for the users to supply custom renderers for custom exceptions. Use case example: Let us say we have this exception: class DaoException extends Exception { } Clinet of log4php api wants to render DaoException in other way that every other Exception. He implements and assigns DaoExceptionRenderer to the renderer map First line of code - we try to find custom exception renderer for current throwable context Second/Third line - if custom exception renderer doesn't exist and the rederer map returns default object renderer then render DaoException as standard exception (LoggerRendererException) if it exists, render DaoException with custom exception renderer (DaoExceptionRenderer) I hope it is clear now, what I itended to do.
        Hide
        Christian Grobmeier added a comment -

        Throwable patch committed with 955838

        I understand what you are doing with the renderer check but left the todo in. Even when its correct I want to think if there is an improvment possible later

        THanks for this one!

        Show
        Christian Grobmeier added a comment - Throwable patch committed with 955838 I understand what you are doing with the renderer check but left the todo in. Even when its correct I want to think if there is an improvment possible later THanks for this one!
        Hide
        Vladimír Gorej added a comment -

        Hello Christian,

        Is the mongo appender going to make it to the log4php ? I kinda forgot about this issue..coz we were talking about throwable.path which was required for mongo appender to work. Mongo appender is attached
        in this issue and it's called src.tgz.

        Cheers

        Show
        Vladimír Gorej added a comment - Hello Christian, Is the mongo appender going to make it to the log4php ? I kinda forgot about this issue..coz we were talking about throwable.path which was required for mongo appender to work. Mongo appender is attached in this issue and it's called src.tgz. Cheers
        Hide
        Christian Grobmeier added a comment -

        Hi Vladimir,

        yes, I want to add the appender to the core. The throwable part is already in. Just worked on it today but got an weird error with Mongo. Maybe you have an idea.

        Calling this:
        $collection = $db->selectCollection('logs');
        $collection->drop()

        will cause a bus error in my env. Its not related to your code, but it currently prevents me to commit. Any ideas are appreciated.

        Cheers

        Show
        Christian Grobmeier added a comment - Hi Vladimir, yes, I want to add the appender to the core. The throwable part is already in. Just worked on it today but got an weird error with Mongo. Maybe you have an idea. Calling this: $collection = $db->selectCollection('logs'); $collection->drop() will cause a bus error in my env. Its not related to your code, but it currently prevents me to commit. Any ideas are appreciated. Cheers
        Hide
        Vladimír Gorej added a comment -

        Hello Christian,

        I have never encountered such an error while running my integration tests. I am not even familiar with this kind of php mongo driver error. I will try to reproduce it tommorow an try to run the tests again. Maybe I will manage to rewrite the testcase without dropping the collection...

        Show
        Vladimír Gorej added a comment - Hello Christian, I have never encountered such an error while running my integration tests. I am not even familiar with this kind of php mongo driver error. I will try to reproduce it tommorow an try to run the tests again. Maybe I will manage to rewrite the testcase without dropping the collection...
        Hide
        Christian Grobmeier added a comment -

        I figured out that this will only work with newest Mongo 1.6.2 and PHP Driver 1.0.9. Now everything is fine.

        But:
        Mongo class needs a prefix in the url: mongodb://

        And second:

        1) LoggerAppenderMongoDBTest::testMongoDBException
        MongoException: The MongoCursor object has not been correctly initialized by its constructor

        /log4php-trunk/src/test/php/appenders/LoggerAppenderMongoDBTest.php:0

        Something is still wrong. Even when I add the prefix as mentioned above, this error occurs. Any idea? Which versions are you using?

        Show
        Christian Grobmeier added a comment - I figured out that this will only work with newest Mongo 1.6.2 and PHP Driver 1.0.9. Now everything is fine. But: Mongo class needs a prefix in the url: mongodb:// And second: 1) LoggerAppenderMongoDBTest::testMongoDBException MongoException: The MongoCursor object has not been correctly initialized by its constructor /log4php-trunk/src/test/php/appenders/LoggerAppenderMongoDBTest.php:0 Something is still wrong. Even when I add the prefix as mentioned above, this error occurs. Any idea? Which versions are you using?
        Hide
        Christian Grobmeier added a comment -

        $appender->activateOptions(); is necessary. Then it works.

        Btw dropCollection() is deprecated, we should use drop() instead.

        Would you like to create an updated patch for that or should i proceed?

        cheers

        Show
        Christian Grobmeier added a comment - $appender->activateOptions(); is necessary. Then it works. Btw dropCollection() is deprecated, we should use drop() instead. Would you like to create an updated patch for that or should i proceed? cheers
        Hide
        Vladimír Gorej added a comment -

        Hello,

        It's bussy week for me, I'll try to provide corrections till the end of this working week. Is it ok for you ?

        Show
        Vladimír Gorej added a comment - Hello, It's bussy week for me, I'll try to provide corrections till the end of this working week. Is it ok for you ?
        Hide
        Vladimír Gorej added a comment -

        Hello Christian,

        I have provided fixes for the problems mentioned above.

        http://github.com/char0n/log4php-MongoDB

        Tested against MongoDB 1.6.2, mongo php driver 1.0.9, log4php 2.0.0 ( with throwable patch)

        Show
        Vladimír Gorej added a comment - Hello Christian, I have provided fixes for the problems mentioned above. http://github.com/char0n/log4php-MongoDB Tested against MongoDB 1.6.2, mongo php driver 1.0.9, log4php 2.0.0 ( with throwable patch)
        Hide
        Vladimír Gorej added a comment -

        Hello,

        just little notice, appender has new codename log4mongo-php and has been moved to
        http://github.com/log4mongo/log4mongo-php

        Show
        Vladimír Gorej added a comment - Hello, just little notice, appender has new codename log4mongo-php and has been moved to http://github.com/log4mongo/log4mongo-php
        Hide
        Christian Grobmeier added a comment -

        Committed with r1070984. I have added some modifications to make it fit better to the other classes. However, this one was really good and great work - thanks very much! And sorry for committing this so late. Next patches will go into log4php easier

        Please review the phpdoc comments - I have added your name directly into the docs. Can you please let me know if you are fine with removing it or would you like to keep it this way (or similar)

        Cheers,
        Christian

        Show
        Christian Grobmeier added a comment - Committed with r1070984. I have added some modifications to make it fit better to the other classes. However, this one was really good and great work - thanks very much! And sorry for committing this so late. Next patches will go into log4php easier Please review the phpdoc comments - I have added your name directly into the docs. Can you please let me know if you are fine with removing it or would you like to keep it this way (or similar) Cheers, Christian
        Hide
        Ivan Habunek added a comment -

        Not to sound ungrateful, but I have several issues with this patch.

        The first thing I noticed is that log4php test suite crashes with a fatal error if MongoDB extension is not loaded. This should be handled in a way that the tests are skipped if required extensions are not loaded (see LoggerAppenderPDOTest.php).

        Second, there are several issues with the layout class LoggerMongoDBBsonLayout.

        • It does not follow naming conventions - it should be called LoggerLayoutBson.
        • The name suggests it formats data in BSON format, but it doesnt; it returns ArrayObject objects. Maybe it should be called LogerLayoutArray or similar. BTW, why use ArrayObject instead of the regular PHP array?

        This raises an interesting issue. The above layout, as it is now, is not compatible with other appenders because it doesn't return a string representation of an event, like all other layouts do. Should this be allowed at all? Perhaps a better solution would be for LoggerAppenderMongoDB not to use a layout, but to convert the Logging event into an array internally.

        Third, the tests do not follow conventions (one test class pre tested class). This refers to LoggerAppenderMongoDBLayoutTest.php which should be integrated into the appender test class or the layout test class.

        To conclude, I think this needs a little work before being included in a stable release. I'll try to do a full code review when I get a little free time. I think we can fix this before v2.1 release.

        Show
        Ivan Habunek added a comment - Not to sound ungrateful, but I have several issues with this patch. The first thing I noticed is that log4php test suite crashes with a fatal error if MongoDB extension is not loaded. This should be handled in a way that the tests are skipped if required extensions are not loaded (see LoggerAppenderPDOTest.php). Second, there are several issues with the layout class LoggerMongoDBBsonLayout. It does not follow naming conventions - it should be called LoggerLayoutBson. The name suggests it formats data in BSON format, but it doesnt; it returns ArrayObject objects. Maybe it should be called LogerLayoutArray or similar. BTW, why use ArrayObject instead of the regular PHP array? This raises an interesting issue. The above layout, as it is now, is not compatible with other appenders because it doesn't return a string representation of an event, like all other layouts do. Should this be allowed at all? Perhaps a better solution would be for LoggerAppenderMongoDB not to use a layout, but to convert the Logging event into an array internally. Third, the tests do not follow conventions (one test class pre tested class). This refers to LoggerAppenderMongoDBLayoutTest.php which should be integrated into the appender test class or the layout test class. To conclude, I think this needs a little work before being included in a stable release. I'll try to do a full code review when I get a little free time. I think we can fix this before v2.1 release.
        Hide
        Christian Grobmeier added a comment -

        Ivan, I already have changed a good portion of stuff like the ones you mentioned. I think you simply can fix what you mentioned if you like.

        On ArrayObject: before your time I have opened LOG4PHP-69. ArrayObject is from the SPL. However, we should check if performance is good or better with simple Arrays.

        On the rest, I agree with you. Lets improve it.

        Show
        Christian Grobmeier added a comment - Ivan, I already have changed a good portion of stuff like the ones you mentioned. I think you simply can fix what you mentioned if you like. On ArrayObject: before your time I have opened LOG4PHP-69 . ArrayObject is from the SPL. However, we should check if performance is good or better with simple Arrays. On the rest, I agree with you. Lets improve it.
        Hide
        Ivan Habunek added a comment -

        I don't think there's a concern about performance in this case. The layout creates an ArrayObject from an array in LoggerMongoDBBsonLayout:68. Then the appender casts it back into an array when appending in LoggerAppenderMongoDB:132. The ArrayObject is not used anywhere in between.

        Also, docs should be written before including this in a stable build. And I'm kind of sick of writing documentation at the moment.

        Show
        Ivan Habunek added a comment - I don't think there's a concern about performance in this case. The layout creates an ArrayObject from an array in LoggerMongoDBBsonLayout:68. Then the appender casts it back into an array when appending in LoggerAppenderMongoDB:132. The ArrayObject is not used anywhere in between. Also, docs should be written before including this in a stable build. And I'm kind of sick of writing documentation at the moment.
        Hide
        Vladimír Gorej added a comment -

        Hi,

        I am reacting to Ivan Habunek comment. I am commiter to log4mongo-php repository.

        I made a new branch with suggested corrections. The branch can be seen here: https://github.com/log4mongo/log4mongo-php/tree/refactor/

        • It does not follow naming conventions - it should be called LoggerLayoutBson.
          > Remaned to LoggerLayoutBson
        • The name suggests it formats data in BSON format, but it doesnt; it returns ArrayObject objects. Maybe it should be called LogerLayoutArray or similar. BTW, why use ArrayObject instead of the regular PHP array?
          > The layout now returns string in bson format. I decided to use ArrayObject, becase I saw a ticket in this jira, that everything should be reviewed and SPL should be used where appropriate. My second argument is that this ArrayObject is passed though many function and method calls. ArrayObject garantes that only object handle is passed and no array copies are created while passing it as argument to function of method. If i would to use array, every method/function call would create copy of passed array.

        LoggerLayoutBson is now fully compatible with your other layouts. LoggerAppenderMongoDBLayout should not be dropped, because it allows api user to creates a custom layout and modify mongo record as he wishes. For example:

        class LoggerMongoDbBsonIpLayout extends LoggerMongoDbBsonLayout {

        public function format(LoggerLoggingEvent $event)

        { $document = parent::format($event); $document['ip'] = $event->getMDC('ip'); return json_encode($document); }

        }

        • Third, the tests do not follow conventions (one test class pre tested class). This refers to LoggerAppenderMongoDBLayoutTest.php which should be integrated into the appender test class or the layout test class.
          > This last part i did not get. Please explain further what exactly you want me to do with the tests.

        Thanks for feedback

        Show
        Vladimír Gorej added a comment - Hi, I am reacting to Ivan Habunek comment. I am commiter to log4mongo-php repository. I made a new branch with suggested corrections. The branch can be seen here: https://github.com/log4mongo/log4mongo-php/tree/refactor/ It does not follow naming conventions - it should be called LoggerLayoutBson. > Remaned to LoggerLayoutBson The name suggests it formats data in BSON format, but it doesnt; it returns ArrayObject objects. Maybe it should be called LogerLayoutArray or similar. BTW, why use ArrayObject instead of the regular PHP array? > The layout now returns string in bson format. I decided to use ArrayObject, becase I saw a ticket in this jira, that everything should be reviewed and SPL should be used where appropriate. My second argument is that this ArrayObject is passed though many function and method calls. ArrayObject garantes that only object handle is passed and no array copies are created while passing it as argument to function of method. If i would to use array, every method/function call would create copy of passed array. LoggerLayoutBson is now fully compatible with your other layouts. LoggerAppenderMongoDBLayout should not be dropped, because it allows api user to creates a custom layout and modify mongo record as he wishes. For example: class LoggerMongoDbBsonIpLayout extends LoggerMongoDbBsonLayout { public function format(LoggerLoggingEvent $event) { $document = parent::format($event); $document['ip'] = $event->getMDC('ip'); return json_encode($document); } } Third, the tests do not follow conventions (one test class pre tested class). This refers to LoggerAppenderMongoDBLayoutTest.php which should be integrated into the appender test class or the layout test class. > This last part i did not get. Please explain further what exactly you want me to do with the tests. Thanks for feedback
        Hide
        Ivan Habunek added a comment -

        Hi Vladimir,

        I have decided to drop the LoggerLayoutBson class for now, since it does not fit in with the rest of the layouts which return strings. Other appenders would fail if they were used with this layout.

        In it's place, I have added the LoggerLoggingEventBsonifier helper class, from your github repository, and updated the appender to use this helper instead of the layout class. I made minor changes to the helper - replaced the ArrayObject with standard array since I don't see why an ArrayObject was needed here.

        Maybe we can let the users create their own "bsonifier" objects and control the data layout that way? What do you think?

        Ivan

        Show
        Ivan Habunek added a comment - Hi Vladimir, I have decided to drop the LoggerLayoutBson class for now, since it does not fit in with the rest of the layouts which return strings. Other appenders would fail if they were used with this layout. In it's place, I have added the LoggerLoggingEventBsonifier helper class, from your github repository, and updated the appender to use this helper instead of the layout class. I made minor changes to the helper - replaced the ArrayObject with standard array since I don't see why an ArrayObject was needed here. Maybe we can let the users create their own "bsonifier" objects and control the data layout that way? What do you think? Ivan
        Hide
        Vladimír Gorej added a comment -

        Hello Ivan,

        Before I comment further, have you checked the branch of git repo I have created a cuple of months ago ?
        You can find it here: https://github.com/log4mongo/log4mongo-php/tree/refactor

        This branch addresses all issues raised in previous comments. By reading your last comment i guess you did not check it. Please review it. It fixes all issues you have problem with, including layout class returning string, test, suggested refactoring, arrays vs arrayobjects.

        Show
        Vladimír Gorej added a comment - Hello Ivan, Before I comment further, have you checked the branch of git repo I have created a cuple of months ago ? You can find it here: https://github.com/log4mongo/log4mongo-php/tree/refactor This branch addresses all issues raised in previous comments. By reading your last comment i guess you did not check it. Please review it. It fixes all issues you have problem with, including layout class returning string, test, suggested refactoring, arrays vs arrayobjects.
        Hide
        Ivan Habunek added a comment -

        Hi Vladimir,

        Sorry for the slow progress of this issue, but we have been busy trying to push the 2.1 release. We appreciate the work you put into this, but there is very few of us working on log4php at the moment.

        I have seen your solution on github and I have taken most of the code from it. However, I think it has several issues.

        Firstly, there are two appenders which are almost identical. LoggerAppenderMongoDBLayout and LoggerAppenderMongoDB. I understand what they do, but it seems unnecessary.

        Secondly, the performance of LoggerAppenderMongoDBLayout is not optimal. When using it in combination with LoggerLayoutBson, the following happens:

        • LoggerLoggingEventBsonifier converts the event to an array
        • LoggerLayoutBson converts this array to json (includes converting the MongoDate object to timestamp)
        • LoggerAppenderMongoDBLayout converts the json back into an array (includes converting the timestamp back to MongoDate)

        The event is first converted to an array, then to json, then back to an array before it is finally logged. You will agree that this is not the best solution.

        I think the best approach to this problem is to have one appender: LoggerAppenderMongoDB which does not use a layout (because layouts produce strings, and here we need an array). This appender can have a protected method which converts the logging event to an array (taken from LoggerLoggingEventBsonifier). If a user wishes to modify the format of the logged array, they can simply extend LoggerAppenderMongoDB and override that method.

        I have created and commited the following changes to the repository:

        • Moved the conversion code from LoggerLoggingEventBsonifier to LoggerAppenderMongoDB::format()
        • Removed the LoggerLoggingEventBsonifier
        • Merged the tests from LoggerLoggingEventBsonifierTest into LoggerAppenderMongoDBTest

        I would appreciate if you would review them and see if this meets all your requirements.

        Best regards,
        Ivan

        Show
        Ivan Habunek added a comment - Hi Vladimir, Sorry for the slow progress of this issue, but we have been busy trying to push the 2.1 release. We appreciate the work you put into this, but there is very few of us working on log4php at the moment. I have seen your solution on github and I have taken most of the code from it. However, I think it has several issues. Firstly, there are two appenders which are almost identical. LoggerAppenderMongoDBLayout and LoggerAppenderMongoDB. I understand what they do, but it seems unnecessary. Secondly, the performance of LoggerAppenderMongoDBLayout is not optimal. When using it in combination with LoggerLayoutBson, the following happens: LoggerLoggingEventBsonifier converts the event to an array LoggerLayoutBson converts this array to json (includes converting the MongoDate object to timestamp) LoggerAppenderMongoDBLayout converts the json back into an array (includes converting the timestamp back to MongoDate) The event is first converted to an array, then to json, then back to an array before it is finally logged. You will agree that this is not the best solution. I think the best approach to this problem is to have one appender: LoggerAppenderMongoDB which does not use a layout (because layouts produce strings, and here we need an array). This appender can have a protected method which converts the logging event to an array (taken from LoggerLoggingEventBsonifier). If a user wishes to modify the format of the logged array, they can simply extend LoggerAppenderMongoDB and override that method. I have created and commited the following changes to the repository: Moved the conversion code from LoggerLoggingEventBsonifier to LoggerAppenderMongoDB::format() Removed the LoggerLoggingEventBsonifier Merged the tests from LoggerLoggingEventBsonifierTest into LoggerAppenderMongoDBTest I would appreciate if you would review them and see if this meets all your requirements. Best regards, Ivan
        Hide
        Ivan Habunek added a comment -

        The appender has been included in the 2.1 release.
        If any additional work or modifications are required, a new issue should be raised.

        Show
        Ivan Habunek added a comment - The appender has been included in the 2.1 release. If any additional work or modifications are required, a new issue should be raised.

          People

          • Assignee:
            Christian Grobmeier
            Reporter:
            Vladimír Gorej
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development