The logging is not reliable.

After debugging i found out that the loggingMode variable of the SensorRunner is wrongly defined.

/**
   * Result logging mode.
   *
   * @var bool
   */
  protected $loggingMode = FALSE;

/**
   * Sets result logging mode.
   *
   * @param string $mode
   *   (NULL, on_request, all)
   */
  public function setLoggingMode($mode) {
    $this->loggingMode = $mode;
  }

And this is the correct instanciation in monitoring.module.

$runner = new SensorRunner($sensors_info);
$runner->setLoggingMode(variable_get('monitoring_sensor_call_logging', 'on_request'));
$runner->verbose($get_verbose_output);

While monitoring.admin.inc lacks setLoggingMode().

CommentFileSizeAuthor
#5 monitoring_logging_2157905_3.patch3.25 KBmiro_dietiker
#2 monitoring_logging_2157905_2.patch2.57 KBmiro_dietiker
#1 monitoring_logging_2157905.patch2.58 KBmiro_dietiker

Comments

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Active » Needs review
Issue tags: +Needs tests
StatusFileSize
new monitoring_logging_2157905.patch2.58 KB

Providing patch that makes logging behaviour reliable and as expected again.

Also with some rename of saveResults to logResults.

I also thought about initializing the loggingMode member variable within __constructor to the default (configured) value. However, sounds like similarly unclean OOP wise (?).

I'm confused the idea of moving logging into the SensorRunner was to have consistent logging results.

The different run contexts with logging settings need testing though.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
StatusFileSize
new monitoring_logging_2157905_2.patch2.57 KB

The last submitted patch, 1: monitoring_logging_2157905.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Yes, as discussed, this should be set to the default automatically. Constructor is fine for now, in 8.x, we might want to think about a runner factory service or so.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
StatusFileSize
new monitoring_logging_2157905_3.patch3.25 KB

As discussed, moving the runner configuration into constructor.

Default NULL replaced with 'none'.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Needs review » Fixed

Fixed.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Title: Unreliable logging » Add tests: Unreliable logging
Status: Fixed » Needs work
ytsurk’s picture

ytsurk
Location Switzerland
commented

this is partiallly tested - look for this in monitoring.api.test
so - i'll extend this ?

    // Set log_calls sensor settings to false - that should prevent logging.
    $settings = monitoring_sensor_settings_get('test_sensor');
    $settings['log_calls'] = FALSE;
    monitoring_sensor_settings_save('test_sensor', $settings);
    monitoring_sensor_run('test_sensor', TRUE);
    $logs = $this->loadSensorData('test_sensor');
    $this->assertEqual(count($logs), 1);

    // Now change the status - that should result in the call being logged.
    $test_sensor_result_data = array(
      'sensor_status' => SensorResultInterface::STATUS_WARNING,
    );
    variable_set('test_sensor_result_data', $test_sensor_result_data);
    monitoring_sensor_run('test_sensor', TRUE);
    $logs = $this->loadSensorData('test_sensor');
    $this->assertEqual(count($logs), 2);
    $log = array_pop($logs);
    $this->assertEqual($log->sensor_status, SensorResultInterface::STATUS_WARNING);

    // Set the logging strategy to "Log all events".
    variable_set('monitoring_sensor_call_logging', 'all');
    // Running the sensor with 'log_calls' settings FALSE must record the call.
    $settings = monitoring_sensor_settings_get('test_sensor');
    $settings['log_calls'] = FALSE;
    monitoring_sensor_settings_save('test_sensor', $settings);
    monitoring_sensor_run('test_sensor', TRUE);
    $logs = $this->loadSensorData('test_sensor');
    $this->assertEqual(count($logs), 3);

    // Set the logging strategy to "No logging".
    variable_set('monitoring_sensor_call_logging', 'none');
    // Despite log_calls TRUE we should not log any call.
    $settings = monitoring_sensor_settings_get('test_sensor');
    $settings['log_calls'] = TRUE;
    monitoring_sensor_settings_save('test_sensor', $settings);
    $logs = $this->loadSensorData('test_sensor');
    monitoring_sensor_run('test_sensor', TRUE);
    $this->assertEqual(count($logs), 3);
miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

Yes, that's a good start. Missing is at least:

First, call the sensor a second time and make sure there's no extra log record.

Additionally, with mode "all", make the sensor caching and make sure that on a regular run (with cached result) there's no extra log record.