Skip to content

logs to stream#1

Open
akaplya wants to merge 4 commits into2.3-developfrom
logs-to-stdout
Open

logs to stream#1
akaplya wants to merge 4 commits into2.3-developfrom
logs-to-stdout

Conversation

@akaplya
Copy link
Copy Markdown
Owner

@akaplya akaplya commented Nov 22, 2019

No description provided.

Comment thread pub/errors/processor.php
@mkdir($reportDirName, 0777, true);
}
$this->_setReportData($reportData);
// $reportDirName = dirname($this->_reportFile);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Maybe let's disable it by env variable? So Cloud can utilize this and disable writing reports to files.

Comment thread app/etc/di.xml
<arguments>
<argument name="formatter" xsi:type="object">Magento\Framework\Logger\Formatter\JsonFormatter</argument>
<argument name="stream" xsi:type="string">php://stdout</argument>
<argument name="level" xsi:type="const">Monolog\Logger::DEBUG</argument>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

As I understand, this means all messages of level "debug" and higher will be logged.
This makes sense for dev environment, but is usually configured differently in production. It would be good to have an ability to configure level of logging from env variable (or env.php).

{
/** @var \Psr\Log\LoggerInterface $logger */
$logger = $this->_objectManager->get(\Psr\Log\LoggerInterface::class);
$logger->error($exception, ['bootstrap' => $bootstrap]);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Looks like it duplicates logic of the client code.

// \Magento\Framework\App\Bootstrap::run()

$this->objectManager->get(LoggerInterface::class)->error($e->getMessage());
if (!$application->catchException($this, $e)) {
       throw $e;
}

/**
* Class Monolog
*/
class Monolog extends LoggerAbstract
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It probably shouldn't be "Monolog", as it just uses PSR logger interface. Not sure if it should be "PsrLogger" :) or something else.

*/
public function format(array $record)
{
if (isset($record['context']['exception']) && $record['context']['exception'] instanceof \Exception) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Btw, we should take into account \Throwable as well now. Or we'll miss errors.
I see that Monolog's JsonFormatter takes this into account when checking the $data.

if ($data instanceof Exception || $data instanceof Throwable) {
    return $this->normalizeException($data);
}

Comment thread app/etc/di.xml
<argument name="formatter" xsi:type="object">Magento\Framework\Logger\Formatter\JsonFormatter</argument>
<argument name="stream" xsi:type="string">php://stdout</argument>
<argument name="level" xsi:type="const">Monolog\Logger::DEBUG</argument>
<!-- <argument name="level" xsi:type="const">Monolog\Logger::ERROR</argument>-->
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Please remove commented code.

$reportId = abs((int)(microtime(true) * random_int(100, 1000)));
$this->directoryWrite->writeFile('report/api/' . $reportId, $this->serializer->serialize($reportData));
// $this->directoryWrite->create('report/api');
$reportId = (string)abs((int)(microtime(true) * random_int(100, 1000)));
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Please replace with $reportId = md5($reportData);

Comment thread pub/errors/processor.php
// }
// $this->_setReportData($reportData);

@file_put_contents($this->_reportFile, $this->serializer->serialize($reportData). PHP_EOL);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Reporting needed only in Development Env.

For production better to use NewRelic APM/OpenTracing sollution

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants