Passing context as extra parameters (Additional data) #175

Merged
merged 1 commit into from Mar 26, 2013

Conversation

Projects
None yet
5 participants
Contributor

pborreli commented Mar 25, 2013

fixes #171

  • WIP

@stof stof and 3 others commented on an outdated diff Mar 25, 2013

src/Monolog/Handler/RavenHandler.php
$this->ravenClient->captureMessage(
- $record['formatted'],
+ $record['message'],
@stof

stof Mar 25, 2013

Contributor

This looks wrong to me. You should not pass an unformatted message (you may want to change the formatter though

@pborreli

pborreli Mar 25, 2013

Contributor

i first tried to change the format option of the LineFormatter but without any success but you are right, I will change it.

@dcramer

dcramer Mar 25, 2013

We likely need to add an option to send the formatted message.

@pborreli

pborreli Mar 25, 2013

Contributor

I've done a new RavenFormatter, based on LineFormatter removing the date and context and extra as we now send it inside extra raven parameter. I need to write some tests and will push it asap.

@Seldaek

Seldaek Mar 26, 2013

Owner

Why not override getDefaultFormatter and just returning a LineFormatter without useless args? Something like return new LineFormatter("[%channel%] %message%");

@pborreli

pborreli Mar 26, 2013

Contributor

that's what i did locally, it works but i'm not satisfied and tend to think too raven-php api should let us send both formatted and message to the api

@pborreli

pborreli Mar 26, 2013

Contributor

@Seldaek changed the format as your advice

@stof stof commented on an outdated diff Mar 25, 2013

src/Monolog/Handler/RavenHandler.php
array(), // $params - not used
- $this->logLevels[$record['level']], // $level
+ $options, // $level
@stof

stof Mar 25, 2013

Contributor

$options with a comment saying level ?

@stof stof and 1 other commented on an outdated diff Mar 25, 2013

src/Monolog/Handler/RavenHandler.php
@@ -59,10 +59,14 @@ public function __construct(Raven_Client $ravenClient, $level = Logger::DEBUG, $
*/
protected function write(array $record)
{
+ $options = array(
+ 'level' => $this->logLevels[$record['level']],
+ 'extra' => $record['context']
@stof

stof Mar 25, 2013

Contributor

what about $record['extra'] ?

@pborreli

pborreli Mar 25, 2013

Contributor

thought the context was the only additional value we could pass, just checked and indeed it makes sense to maintain this option but for $record['extra']

@stof

stof Mar 25, 2013

Contributor

The context is passed when calling the logger while the extra are added by processors

@pborreli

pborreli Mar 25, 2013

Contributor

looks like context can be anything, does it make sense to just merge extra and context record's arrays and send it to sentry as extra ?

@stof

stof Mar 25, 2013

Contributor

yeah, probably

Contributor

pborreli commented Mar 26, 2013

New version, tell me what you think please

@Seldaek Seldaek and 1 other commented on an outdated diff Mar 26, 2013

src/Monolog/Handler/RavenHandler.php
);
if ($record['level'] >= Logger::ERROR && isset($record['context']['exception'])) {
$this->ravenClient->captureException($record['context']['exception']);
}
}
+
+ /**
+ * {@inheritDoc}
+ */
+ protected function getDefaultFormatter()
+ {
+ return new LineFormatter('%channel%.%level_name%: %message%');
@Seldaek

Seldaek Mar 26, 2013

Owner

Is level_name useful considering the level is passed already to captureMessage?

@pborreli

pborreli Mar 26, 2013

Contributor

was pushing while you wrote this :)

Owner

Seldaek commented Mar 26, 2013

Looks better like that :)

cevou commented Mar 26, 2013

I also like this approach.

@Seldaek Seldaek added a commit that referenced this pull request Mar 26, 2013

@Seldaek Seldaek Merge pull request #175 from pborreli/issue-171
Passing context as extra parameters (Additional data)
434ef8b

@Seldaek Seldaek merged commit 434ef8b into Seldaek:master Mar 26, 2013

1 check passed

default The Travis build passed
Details
Contributor

pborreli commented Mar 26, 2013

thanks, btw maybe we should push raven version require-dev dependency

pborreli deleted the pborreli:issue-171 branch Mar 26, 2013

Owner

Seldaek commented Mar 26, 2013

If we should please send a PR :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment