Implemented logging structure #304

everton-rosario · 2018-01-12T18:29:09Z

Implemented a simple internal logging structure.
Removed log4php
Fixed Composer install log4php problem #302
Fixed Using composer - error on log4php #245
Fixed Transformer error logging #240

Calls to Transformer have now stored all logging messages:

        $json_file = file_get_contents(__DIR__ . '/simple-rules.json');

        $instant_article = InstantArticle::create();
        $transformer = new Transformer();
        $transformer->loadRules($json_file);

        $html_file = file_get_contents(__DIR__ . '/simple.html');

        TransformerLog::setLevel(TransformerLog::DEBUG);
        $transformer->transformString($instant_article, $html_file);
        $result = $transformer->getLogs();

Jayad · 2018-01-15T02:16:53Z

composer.json

-        "symfony/css-selector": "2.8.* || ^3.0",
-        "facebook/graph-sdk": "~5.0",
-        "apache/log4php": "2.3.0"
+        "symfony/css-selector": "2.8.*",


Can we upgrade this to latest one(i guess 4.0.3) if no dependency with this version(2.8.*)?

Anyone is able to upgrade it, but we cannot move to 3.2 and up since it is not php 5.4 compatible.
We need to re-think the need of keeping php 5.4 compatibility

Jayad · 2018-01-15T02:17:24Z

src/Facebook/InstantArticles/Transformer/Transformer.php

+    /**
+     * @var string[] $logs The log messages generated during the transformation process.
+     */
+    private $logs = ['Possible log levels: OFF, ERROR, INFO or DEBUG'];


How about WARN or FATAL?

Warnings are all ready to be accessed from $transformer->getWarnings().

Fatals I see as the Exception handling being the best approach.

Jayad · 2018-01-15T02:19:57Z

README.md

@@ -82,7 +82,7 @@ If you are encountering problems, the following tips may help in troubleshooting

 - If content is missing from your transformed article, more likely than not there isn't a **Transformer Rule** matching an element in your source markup. See how to configure appropriate rules for your content in the [Transformer Rules documentation](https://developers.facebook.com/docs/instant-articles/sdk/transformer-rules).

- Set the `threshold` in the [configuration of the Logger](https://logging.apache.org/log4php/docs/configuration.html#PHP) to `DEBUG` to expose more details about the items processed by the Transformer.
+- Set the `threshold` in the transformer Logger level to `DEBUG` to expose more details about the items processed by the Transformer. `$transformer->setLogLevel(Transformer::LOG_DEBUG);`


Just Curious, What will be the advantage of using Logger implemented within the same code, instead of using the log4php library which already has lot of functionality.

Jayad · 2018-01-15T02:23:52Z

src/Facebook/InstantArticles/Client/ServerMessage.php

@@ -73,8 +73,6 @@ public static function fromLevel($level, $message)
        if ($validLevel) {
            return new self($level, $message);
        } else {
-            \Logger::getLogger('facebook-instantarticles-client')
-                ->info('Unknown message level "$level". Are you using the last SDK version?');


Will this file functions will be used over Transformer.php implementations?

No.
All the package /Client is on SDK for making it easier to call all APIs for the IA ingestion. See more here.

mburak

I'd put all the logging methods/logic in a different class 'logger' or something similar just as a matter of separation of concerns. And then inject that class in the transformer and use it. What do you think?

*[x] Implemented a simple internal logging structure. *[x] Removed log4php *[x] Fixed #302 *[x] Fixed #245 *[x] Fixed #240 Calls to Transformer have now stored all logging messages: ``` $json_file = file_get_contents(__DIR__ . '/simple-rules.json'); $instant_article = InstantArticle::create(); $transformer = new Transformer(); $transformer->loadRules($json_file); $html_file = file_get_contents(__DIR__ . '/simple.html'); $transformer->setLogLevel(Transformer::LOG_DEBUG); $transformer->transformString($instant_article, $html_file); $result = $transformer->getLogs(); ```

…g class

everton-rosario · 2018-01-16T13:23:24Z

Hey @mburak , good call on moving it away from Transformer class.

Check the new commit with this in place: e954e7e

mburak · 2018-01-16T14:01:24Z

hey @everton-rosario, it still looks a little bit coupled for me. What if instead of having the logs being added (that logic) in the Tranformer class we do that in the TransformerLog class (and maybe TranformerLogger is a better naming?).

So instead of having this:
$this->addLog( TransformerLog::INFO, 'Possible log levels: OFF, ERROR, INFO or DEBUG. To change it call method TransformerLog::setLevel("DEBUG").' );

we could have something like this:
TransformerLogger::logInfo($this->logs, 'Possible log levels: OFF, ERROR, INFO or DEBUG. To change it call method TransformerLog::setLevel("DEBUG").' );

so in this case we leave all the logging logic on the TransformerLogger class. I'm also adding an abstraction here so we don't use the different enums for each level and we just have a different method for each level logging.
What do you think?

diegoquinteiro

<3

(consider @mburak's comments, though)

everton-rosario · 2018-01-16T17:32:19Z

@mburak This is not a Logger tool. It was intended and built only for transformer, thats why I coupled it to the Transformer instance.
If we handle it as a Logger tool, we will have all transformations altogether into same log.

Here, with this simple structure, we have a log structure that isolates only transformation debug info, to improve depuration on the transformer.

Since it has all logging about transformation only, users of the SDK can optionally use their own logging system to output the transformation Log that occurred if they intend to, leaving it as optional.

Also I don't see this logging system being used to anything else but Transformation debug.

mburak · 2018-01-16T17:40:27Z

@everton-rosario gotcha. We could still handle each transformation in different logs even when having a decoupled logger. You pass the log array to the logger so the transformer still keeps the log state.
Anyway, I understand that's not the intention of this. Go for it!

everton-rosario · 2018-01-16T17:45:15Z

Issue filed so we can treat it evenly not only for the logs, but also for the warnings:
#306

everton-rosario requested review from diegoquinteiro and pestevez January 12, 2018 18:29

facebook-github-bot added the CLA Signed label Jan 12, 2018

everton-rosario force-pushed the logging branch 2 times, most recently from a343db3 to c804a0d Compare January 12, 2018 19:11

Jayad reviewed Jan 15, 2018

View reviewed changes

everton-rosario mentioned this pull request Jan 15, 2018

Transformer error logging #240

Closed

mburak reviewed Jan 15, 2018

View reviewed changes

everton-rosario force-pushed the logging branch from c804a0d to 51d5261 Compare January 15, 2018 15:40

everton-rosario force-pushed the logging branch from 51d5261 to b65ce8c Compare January 15, 2018 18:18

@mburak topics to move code away from Transformer and create a Loggin…

e954e7e

…g class

diegoquinteiro approved these changes Jan 16, 2018

View reviewed changes

everton-rosario merged commit 7108507 into master Jan 16, 2018

everton-rosario deleted the logging branch January 16, 2018 17:49

alexander-schranz mentioned this pull request Feb 20, 2018

Fix symfony requirement for css selector #319

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented logging structure #304

Implemented logging structure #304

everton-rosario commented Jan 12, 2018 •

edited

Loading

Jayad Jan 15, 2018

everton-rosario Jan 15, 2018

Jayad Jan 15, 2018

everton-rosario Jan 15, 2018

Jayad Jan 15, 2018

Jayad Jan 15, 2018

everton-rosario Jan 15, 2018

mburak left a comment

everton-rosario commented Jan 16, 2018

mburak commented Jan 16, 2018

diegoquinteiro left a comment •

edited

Loading

everton-rosario commented Jan 16, 2018

mburak commented Jan 16, 2018

everton-rosario commented Jan 16, 2018

Implemented logging structure #304

Implemented logging structure #304

Conversation

everton-rosario commented Jan 12, 2018 • edited Loading

Jayad Jan 15, 2018

Choose a reason for hiding this comment

everton-rosario Jan 15, 2018

Choose a reason for hiding this comment

Jayad Jan 15, 2018

Choose a reason for hiding this comment

everton-rosario Jan 15, 2018

Choose a reason for hiding this comment

Jayad Jan 15, 2018

Choose a reason for hiding this comment

Jayad Jan 15, 2018

Choose a reason for hiding this comment

everton-rosario Jan 15, 2018

Choose a reason for hiding this comment

mburak left a comment

Choose a reason for hiding this comment

everton-rosario commented Jan 16, 2018

mburak commented Jan 16, 2018

diegoquinteiro left a comment • edited Loading

Choose a reason for hiding this comment

everton-rosario commented Jan 16, 2018

mburak commented Jan 16, 2018

everton-rosario commented Jan 16, 2018

everton-rosario commented Jan 12, 2018 •

edited

Loading

diegoquinteiro left a comment •

edited

Loading