Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Added support for Loggly batch uploads #309

Merged
merged 6 commits into from Feb 25, 2014

Conversation

Projects
None yet
3 participants

Loggly supports batch processing of log messages via the bulk API (https://www.loggly.com/docs/api-sending-data/#bulk).

This patch for LogglyHandler - when used in conjunction with FingersCrossedHandler, BufferHandler, etc. - adds support for sending multiple messages to Loggly in a single request.

The generic JsonFormatter cannot be reused as Loggly expect batch logs to be uploaded as JSON-encoded strings with each event separated by new lines.

@Seldaek Seldaek commented on the diff Jan 21, 2014

src/Monolog/Handler/LogglyHandler.php
@@ -45,17 +48,38 @@ public function setTag($tag)
protected function write(array $record)
{
- $url = sprintf("http://%s/inputs/%s/", self::HOST, $this->token);
+ $this->send($record["formatted"], self::ENDPOINT_SINGLE);
+ }
+
+ public function handleBatch(array $records)
+ {
+ $level = $this->level;
+
+ $records = array_filter($records, function ($record) use ($level) {
+ return ($record['level'] >= $level);
+ });
+
+ if ($records) {
+ $this->send($this->getFormatter()->formatBatch($records), self::ENDPOINT_BATCH);
@Seldaek

Seldaek Jan 21, 2014

Owner

How about doing the \n concatenation here in a foreach just calling format() on the JsonFormatter? That way we don't need to introduce a LogglyFormatter. And you don't need to close pull requests to add/update commits by the way :)

@apancutt

apancutt Jan 21, 2014

That would create (potentially) unexpected behaviour, and break the separation of concern in the library design; the handler should not responsible for formatting. It would be expected that any <handler>::handleBatch() calls <formatter>::formatBatch() and implementing additional formatting in the LogglyHandler would cause confusion. That's my thinking, anyway.

Sorry about the duplicate pull requests. I spotted an error in the original PR and wasn't sure how long it would take to fix so I just closed it.

@Seldaek

Seldaek Jan 21, 2014

Owner

Hmm yeah, I see your point, but I see the newlines not really as message formatting but rather protocol stuff for the loggly handler. That said, it could actually be part of the JsonFormatter as a constructor option IMO. That could be useful for other cases as well like if you want to produce a log file with one-line-per-record json formatted records.

@Baachi Baachi commented on the diff Jan 21, 2014

src/Monolog/Formatter/JsonFormatter.php
return json_encode($records);
}
+
+ /**
+ * Use new lines to separate records instead of a
+ * JSON-encoded array.
+ *
+ * @param array $records
+ * @return string
+ */
+ protected function formatBatchNewlines(array $records)
+ {
+ $instance = $this;
@Baachi

Baachi Jan 21, 2014

Contributor

This snippet should do the same but is shorter:

return implode(PHP_EOL, array_map($records, array($this, 'format')));
@apancutt

apancutt Jan 21, 2014

On second thought, I have reverted that change. Reason being is memory efficiency; array_walk() will modify the records by reference.

@Baachi

Baachi Jan 21, 2014

Contributor

Ah i didn't know that.

The use of array_map() has been reverted for memory efficiency. Using array_walk() performs better as it modifies the array by reference.

This also fixes the failing unit test.

The LogglyFormatter has been reinstated, this time to allow for setting the timestamp parameter (see https://www.loggly.com/docs/automated-parsing/#json).

For minimal change risk, I propose reverting the changes to JsonFormatter and handle new line formatting as part of the LogglyFormatter.

Should I go ahead and revert the changes to JsonFormatter, or would you prefer to keep it as-is?

@Seldaek Seldaek commented on the diff Feb 19, 2014

src/Monolog/Formatter/LogglyFormatter.php
+ public function __construct($batch_mode = self::BATCH_MODE_NEWLINES)
+ {
+ parent::__construct($batch_mode);
+ }
+
+ /**
+ * Appends the 'timestamp' parameter for indexing by Loggly.
+ *
+ * @see https://www.loggly.com/docs/automated-parsing/#json
+ * @see \Monolog\Formatter\JsonFormatter::format()
+ */
+ public function format(array $record)
+ {
+ if (isset($record["datetime"]) && ($record["datetime"] instanceof \DateTime)) {
+ $record["timestamp"] = $record["datetime"]->format("c");
+ // @todo unset the 'datetime' parameter, retained for BC
@Seldaek

Seldaek Feb 19, 2014

Owner

It all looks good now apart from this todo line, not sure I get this. datetime is from the monolog record and loggly uses timestamp instead is that it?

@apancutt

apancutt Feb 25, 2014

Exactly. Loggly will only sync with a unix timestamp parameter. If the field is missing, the event date/time is assumed to be the date/time that the event request was received by Loggly.

Having both a timestamp and datetime parameter sent will cause confusion to users as it's likely that the times will vary slightly.

Some custom implementations may have created a dependency on the datetime parameter so it can't be safely removed. Would you consider it safe to remove for 1.8?

@Seldaek

Seldaek Feb 25, 2014

Owner

No I guess it's good to keep it for now, I'll add it to the 2.0 TODOs. Let's merge this then thanks!

Seldaek added a commit that referenced this pull request Feb 25, 2014

Merge pull request #309 from apancutt/master
Added support for Loggly batch uploads

@Seldaek Seldaek merged commit e8604a4 into Seldaek:master Feb 25, 2014

1 check passed

default The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment