Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate Logging Infrastructure #1458

Closed
wants to merge 11 commits into from
Closed

Conversation

ryanrath
Copy link
Contributor

Description

This PR swaps all Pear Log usage w/ Monolog.

  • Added a new CCR\Logging class that takes the place of CCR\Log and utilizes Monolog.
  • Maintained usage of general log settings in portal_settings.ini
  • Removed usage of the custom trace level for now, but this can be re-added if we create a fork of Monolog as we did w/ Log.
  • Updated / Added doc blocks to reflect using Monolog vs. Pear.
  • A few minor updates to some of our tests as Monolog uppercases the level name.
  • The one big difference between Pear / Monolog I found was that Monolog does not support non-string log messages out of the box. To resolve this I wrapped each of these instances w/ a json_encode but we have a number of other options when addressing this. We could stick w/ converting it pre-log call and use print_r($message, true) or a custom conversion function. If we choose to fork Monolog for a new Trace level then we could also update Logger::addRecord to utilize the same extractMessage function that Pear's Log class does.

Motivation and Context

This PR came about from looking at what would be required to update XDMoD to support integrating Symfony Flex which requires PHP7. After speaking with Jeff about his students work towards the Centos8 / PHP7 goal he pointed me to https://docs.google.com/document/d/1fIr5ZzTrYdfO3aWP_sNouC-iojNfePqn3axMapJVsu4 which documents some of the currently known blockers for migrating XDMoD to Centos8 / PHP7. I started experimenting w/ a local docker image based on Jeff's students work and noticed that Pear Log required MDB2 which is not compatible w/ PHP7. After a brief conversation he had mentioned that we should probably replace our Logging prior to moving to PHP7, which got me thinking about how hard it would be to actually do. Turns out that it wasn't hard at all, just tedious. I also have branches ready to go for AppKernels, XSEDE and SUPREMM.

Tests performed

All automated tests for OpenXDMoD fresh install & upgrade

Checklist:

  • The pull request description is suitable for a Changelog entry
  • The milestone is set correctly on the pull request
  • The appropriate labels have been added to the pull request

This is in preperation for ugprading to Centos8
Just removing some now unused `use` statements & adding / updating documentation
where needed.
Monolog does not support logging of arrays, so I've updated these calls to
utilize json_encode.
@ryanrath ryanrath added maintenance / code quality Improvements and code cleanup. Not a new feature or enhancement to existing functionality. Category:Infrastructure Internal infrastructure updates/changes labels Oct 12, 2020
@ryanrath ryanrath added this to the 9.5.0 milestone Oct 12, 2020
@jpwhite4
Copy link
Member

jpwhite4 commented Oct 16, 2020

There are a large number of code changes that appear to just change CCR::Log::X to CCR::Logging::X. Why the namespace change?

to clarify: why not just change the internals of CCR::Log. The existing code would not then need to be modified and the code changes would be much more manageable.

<EDIT 1> For example:

- $logger = Log::factory('batch-export', $logConf);
+ $logger = Logging::singleton('batch-export', array('console' => array(), 'mysql' => array()));

This code change would not be necessary at all if you write a function called factory that accepts the old style logConf varialbe)

@@ -80,15 +81,15 @@
'mail' => false,
'consoleLogLevel' => $logLevel
);
$logger = Log::factory('batch-export', $logConf);
$logger = Logging::singleton('batch-export', array('console' => array(), 'mysql' => array()));
Copy link
Member

Choose a reason for hiding this comment

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

Why not just write a Log::factory function that does what we need? Then you wouldn't need to change most of this code.

Copy link
Member

Choose a reason for hiding this comment

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

Also this code change leaves an unused variable $logConf - I highly suggest making a Log::factory() that accepts the original log configuration. then this file would not need to change at all.

Copy link
Member

Choose a reason for hiding this comment

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

Also the old code used $logLevel as the console log level. The new code seems to ignore the logLevel setting. Again this bug would not exist if you make a Log::factory() that accepts the original log configuration

@@ -154,7 +155,11 @@

if ( NULL !== $scriptOptions['verbosity'] ) $conf['consoleLogLevel'] = $scriptOptions['verbosity'];

$logger = Log::factory('etl_aggregation_table_manager', $conf);
$logger = Logging::factory('etl_aggregation_table_manager', array(
Copy link
Member

Choose a reason for hiding this comment

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

Why don't you make the Logging::factory function accept the $conf varialbe in its current form? Also why change the namespace?


// =====================================================================

// NOTE: "process_start_time" is needed for log summary.
$logger->notice(array(
$logger->notice(json_encode(array(
Copy link
Member

Choose a reason for hiding this comment

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

Why didn't you write a wrapper function that calls the monolog logger with a string? This would significantly reduce the amount of code changes (and hence the 'attack' surface of mistakes creeping in) it would also make it significantly easier to review the code and would have been less work.

case 'console':
$level = array_key_exists('level', $typeConfig)
? $typeConfig['level']
: self::getLevel(\xd_utilities\getConfiguration('logger', 'default_level_console'), Logger::NOTICE);
Copy link
Member

Choose a reason for hiding this comment

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

The old code defaulted to WARNING if the log level setting was absent from the configuration file:

   protected static function getDefaultLogLevel($logType)
    {
        $option = 'default_level_' . $logType;

        try {
            $levelName = self::getConfiguration($option);
            $level = constant(get_class() . '::' . strtoupper($levelName));
        } catch (Exception $e) {
            $level = self::WARNING;
        }

        return $level;
    }

@ryanrath
Copy link
Contributor Author

ryanrath commented Nov 9, 2020

This PR is being replaced by #1461

@ryanrath ryanrath closed this Nov 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category:Infrastructure Internal infrastructure updates/changes maintenance / code quality Improvements and code cleanup. Not a new feature or enhancement to existing functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants