Skip to content
This repository was archived by the owner on Jan 8, 2020. It is now read-only.

Added MongoDB log writer#1825

Merged
weierophinney merged 1 commit into
zendframework:masterfrom
jmikola:mongo-log-writer
Jul 12, 2012
Merged

Added MongoDB log writer#1825
weierophinney merged 1 commit into
zendframework:masterfrom
jmikola:mongo-log-writer

Conversation

@jmikola

@jmikola jmikola commented Jul 9, 2012

Copy link
Copy Markdown
Contributor

This is my first PR, so feel free to scrutinize this thoroughly.

While implementing this, I had a question about the $event arguments for the write() method. Are all of the values either scalars or an array of scalars? I'm assuming that based on the DB writer's mapEventIntoColumn() and eventIntoColumn() methods, as array values are handled specially (but only one level in). This obviously isn't a problem for Mongo, although I was curious if objects or other types might end up in $event and could benefit from special processing (e.g. converting DateTime instances to MongoDates).

Comment thread tests/Zend/Log/Writer/MongoDBTest.php Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this test?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha, I was testing whether any() still complained if an expectation was never invoked. It only happened in conjunction with with(), but recent version of PHPUnit must have resolved the bug.

@travisbot

Copy link
Copy Markdown

This pull request passes (merged 90f1019c into 65f48f6).

@travisbot

Copy link
Copy Markdown

This pull request passes (merged 513ada7e into 65f48f6).

@Maks3w

Maks3w commented Jul 9, 2012

Copy link
Copy Markdown
Member

@jmikola

jmikola commented Jul 10, 2012

Copy link
Copy Markdown
Contributor Author

Will do when I'm back in the office tomorrow morning.

@b-durand

Copy link
Copy Markdown
Contributor

@jmikola Nice, thanks :)

What is the benefit between convert DateTime to ISO and MongoDates? We need to add a Formater to format the data (dates, ...). I will review that component soon.

@jmikola

jmikola commented Jul 10, 2012

Copy link
Copy Markdown
Contributor Author

@b-durand: Essentially the same reason you'd want a date column type in SQL. Converting to a MongoDate will let you work with the value as a date object for querying or, better yet, the aggregation framework (for generating reports and doing log analysis).

Is there any documentation about the structure of the $event arrays, or is it mostly free form apart from a message and priority?

@jmikola

jmikola commented Jul 10, 2012

Copy link
Copy Markdown
Contributor Author

@Maks3w: I updated the doc headers. Can you review?

@travisbot

Copy link
Copy Markdown

This pull request passes (merged 246788d8 into 888f7b4).

Comment thread library/Zend/Log/Writer/MongoDB.php Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is not necessary

@Maks3w

Maks3w commented Jul 10, 2012

Copy link
Copy Markdown
Member

@ezimuel Could you do the final review of this PR?

@Maks3w

Maks3w commented Jul 10, 2012

Copy link
Copy Markdown
Member

@jmikola

jmikola commented Jul 10, 2012

Copy link
Copy Markdown
Contributor Author

@Maks3w: Thanks. Looking at the Logger code, $timestamp is going to end up as a string based on the Logger's configured format. This makes converting back to a DateTime or integer timestamp unreliable (e.g. if the format string is M j).

I think the responsibility of date formatting fall on the Writer class, which may or may not elect to use some functionality in a formatter. For a DB-based logger, that's really the only way you'll be able to properly record a log entry's timestamp in the appropriate field type.

@Maks3w

Maks3w commented Jul 10, 2012

Copy link
Copy Markdown
Member

@jmikola You can open a discussion about that in the Mailing-List.

@b-durand @ezimuel ping

Comment thread library/Zend/Log/Writer/MongoDB.php Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The message of exception can be more explicit like MongoCollection can be empty or must be defined.

@b-durand

Copy link
Copy Markdown
Contributor

If you need to format the event (array), you need to define a Formatter to solve type problem like date or scalar. You should have a separation of concerns : writer knows how send event to a backend, and formatter processes data to validate the "rules" of the backend.

You open a way to use DateTime (as default), or MongoDate if you have an advantage.

@jmikola

jmikola commented Jul 10, 2012

Copy link
Copy Markdown
Contributor Author

The formatter isn't helpful as it currently converts event arrays into strings for writers that work with single strings (e.g. syslog, email). @Maks3w pointed out that the Logger class converts the DateTime object to a string (using the date format option), so we only have a date string to work with in the writer. I opened a thread on the mailing list to propose using formatters for date formatting. In that case, DB writers would still not need a formatter, and they could insert a native date type (better for storage and querying).

@travisbot

Copy link
Copy Markdown

This pull request passes (merged 738de73 into a4d6ece).

@weierophinney

Copy link
Copy Markdown
Member

@jmikola As noted on the ML, your proposal makes sense. Go ahead and submit that as part of this PR.

@b-durand

Copy link
Copy Markdown
Contributor

@weierophinney on the ML contributor? I have not seen this topic.

@Maks3w

Maks3w commented Jul 11, 2012

Copy link
Copy Markdown
Member

@b-durand

Copy link
Copy Markdown
Contributor

@Maks3w thanks :)
@jmikola Writes on the ML contributor more than dedicated ML like "core" as suggested by Matthew. Can you send me a gist of prototype of your formatter service to solve this problem of date please?
@weierophinney Yes, I confirms that sounds good on the screen :)

@b-durand

Copy link
Copy Markdown
Contributor

@jmikola (short story of actual code) I used to generate the DateTime timestamp, because I prefer it to the date function. And, an event should be a simple type as a string to be able to write easly on anywhere (stderr, file, etc).

In Java, everything is an object: the event, a priority for the event, etc. It was not my initial intention of low complexity with all object. But, it can make sense here :)

@jmikola

jmikola commented Jul 12, 2012

Copy link
Copy Markdown
Contributor Author

@b-durand: Yup, I'm subscribed to both lists now, and will be sure to use contributors next time.

If this PR is all right, you can merge it whenever you like. I'd prefer to submit another PR with my proposed formatter changes (which will include Logger and writer class changes) and we can work through it using PR comments. I think that'd be more productive than attempting to do it in a gist, as I'm going to implement it in tandem with test alterations.

@weierophinney weierophinney merged commit 738de73 into zendframework:master Jul 12, 2012
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants