Add record extra data to logdna entry metadata

[slepic]

Hi,

I am following on the PR #13

I have pushed two more alternatives for comparision #14 and #15

Let's move the discussion here so its not scattered on all the PRs

[nvanheuverzwijn]

@slepic I'll go ahead and merge #15. It feels like the expected behavior. Even in other monolog example, there is always a context and extra in the log output. It's never merged together.

If someone wants to change the behavior, they can write their own handler for their use-case or add a PR.

[slepic]

@nvanheuverzwijn sounds good to me, although I have 2 notes:

1. I kinda like the implementation here: https://github.com/nvanheuverzwijn/monolog-logdna/blob/2fc9554b0186d19963c1e2d910f4d469eabc1ef6/src/Monolog/Formatter/LogdnaMetadataMapper.php#L15

if it were

protected function getMetadata(LogRecord $record): array
{
  $meta = $record->context;
  if ($record->extra) {
    $meta['monolog.extra'] = $record->extra;
  }
  return $meta;
}

• it is possible that monolog.extra key exists in context, but very unlikely and so although it is a BC it is unlikey to actually impact anyone (it is probably non issue for those checking logdna manually / without automation over metadata)
• other than that, the compatibility with previous version is maintained
• no nesting occurs for context, which would be more natural to those not using any monolog processors (an possibly for those using processors as well)
• this would be similar to sentry (https://github.com/getsentry/sentry-php/blob/master/src/Monolog/Handler.php#L79) although they also set context as monolog.context separately.

2. In any case I am inclined to think that the paramater $ignoreEmptyContextAndExtra that I used in the implementation of #15 should actually be set to true by default as opposed to false which is coming from the parent implementation (https://github.com/Seldaek/monolog/blob/main/src/Monolog/Formatter/JsonFormatter.php#L48)

In fact, it should probably be removed from the constructor of LogdnaFormatter entirely and getMetadata should just behave as if it was true anyway.

If nothing is in context, an empty metadata property does not need to exist in logdna. Similarly with extra. And If we were to implement the alternative from my note 1) the same would go for empty monolog.extra property.

Anyway, If 2) is not implemented we should at least have an object conversion $meta = ['context' => (object) $context, 'extra' => (object) $extra], otherwise if they are empty, they will appear to be empty json arrays rather than empty json objects.

[nvanheuverzwijn]

@slepic I like your point 1. monolog.extra makes it clear and obvious that the extra key comes from monolog (which it does!). I think we definitely should do that for all the points you bring.

For point 2, I believe we don't need to implement what ignoreEmptyContextAndExtra is doing as it would be done by the JsonFormatter parent class. I'm not too sure thought. In any case, I agree with you. Sending empty array to logdna does not make much sense and we should always ignore context and extra when they are empty.

I'm not sure if we can remove it from the constructor because it's coming from the parent class ? I don't remember if we can do that at all in PHP.