Clean re-implementation of PR #92: relative date filters, export improvements, and duplicate suppression fixes - #121
Conversation
Agent-Logs-Url: https://github.com/rsyslog/loganalyzer/sessions/95914bfd-8533-44ae-8d55-d98bb7e9195e Co-authored-by: alorbach <1675286+alorbach@users.noreply.github.com>
Agent-Logs-Url: https://github.com/rsyslog/loganalyzer/sessions/95914bfd-8533-44ae-8d55-d98bb7e9195e Co-authored-by: alorbach <1675286+alorbach@users.noreply.github.com>
…esult.cache to .gitignore Agent-Logs-Url: https://github.com/rsyslog/loganalyzer/sessions/95914bfd-8533-44ae-8d55-d98bb7e9195e Co-authored-by: alorbach <1675286+alorbach@users.noreply.github.com>
There was a problem hiding this comment.
5 issues found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/include/functions_filters.php">
<violation number="1" location="src/include/functions_filters.php:350">
P2: Positive day offsets are accepted by the regex but never applied, so accepted inputs like `1T12:00` produce an incorrect timestamp.</violation>
</file>
<file name="src/export.php">
<violation number="1" location="src/export.php:237">
P2: Update `period_end_ts` before the duplicate `continue`; otherwise trailing suppressed duplicates can produce an incorrect export end timestamp in the filename.</violation>
<violation number="2" location="src/export.php:261">
P2: Check `timereported[EVTIME_TIMESTAMP]` before using it; the current guard only checks the parent key and can produce undefined-index warnings.</violation>
</file>
<file name="src/index.php">
<violation number="1" location="src/index.php:353">
P2: The timestamp guard is incomplete: it checks `timereported` but not `timereported[EVTIME_TIMESTAMP]`, which can trigger runtime warnings on records missing that nested key.</violation>
</file>
<file name="src/include/functions_common.php">
<violation number="1" location="src/include/functions_common.php:1337">
P2: Positive relative day offsets are parsed but ignored. If the input is positive, it silently evaluates to today instead of the intended future date.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
…ds, period_end_ts in duplicate path Agent-Logs-Url: https://github.com/rsyslog/loganalyzer/sessions/3fe9f4cb-f857-4b60-a17f-9756aa9d478e Co-authored-by: alorbach <1675286+alorbach@users.noreply.github.com>
|
@gemini review |
@alorbach I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
Code Review
This pull request enhances duplicate message suppression for both the main display and export functionality, introducing a configurable maximum timestamp distance for identifying duplicates. It also adds support for relative date syntax in time filters, updates export filenames to include date ranges, and includes UI updates to show the count of suppressed records. Review feedback focuses on improving the consistency and safety of array access when retrieving event timestamps to prevent potential PHP notices and improve readability.
| if ( GetConfigSetting("SuppressDuplicatedMessages", 0, CFGLEVEL_USER) == 1 && isset($logArray[SYSLOG_MESSAGE]) ) | ||
| { | ||
| $szCurrentMessage = $logArray[SYSLOG_MESSAGE]; | ||
| $szCurrentTs = (isset($logArray['timereported']) && is_array($logArray['timereported']) && isset($logArray['timereported'][EVTIME_TIMESTAMP])) ? $logArray['timereported'][EVTIME_TIMESTAMP] : 0; |
There was a problem hiding this comment.
The variable $szCurrentTs is calculated using a ternary operator that checks is_array($logArray['timereported']). This check is redundant if isset($logArray['timereported']) is already true, as the subsequent access ['timereported'][EVTIME_TIMESTAMP] would fail if it were not an array. Simplify this to improve readability.
$szCurrentTs = (isset($logArray['timereported'][EVTIME_TIMESTAMP])) ? $logArray['timereported'][EVTIME_TIMESTAMP] : 0;| if ( isset($logArray['timereported'][EVTIME_TIMESTAMP]) ) | ||
| $content['period_end_ts'] = $logArray['timereported'][EVTIME_TIMESTAMP]; |
There was a problem hiding this comment.
The nested key check isset($logArray['timereported'][EVTIME_TIMESTAMP]) is correct, but for consistency with other parts of the codebase and to avoid potential notices if timereported is not an array, ensure is_array($logArray['timereported']) is also checked.
if ( isset($logArray['timereported']) && is_array($logArray['timereported']) && isset($logArray['timereported'][EVTIME_TIMESTAMP]) )
$content['period_end_ts'] = $logArray['timereported'][EVTIME_TIMESTAMP];| if ( !isset($content['period_start_ts']) && isset($logArray['timereported'][EVTIME_TIMESTAMP]) ) | ||
| $content['period_start_ts'] = $logArray['timereported'][EVTIME_TIMESTAMP]; | ||
| if ( isset($logArray['timereported'][EVTIME_TIMESTAMP]) ) | ||
| $content['period_end_ts'] = $logArray['timereported'][EVTIME_TIMESTAMP]; |
There was a problem hiding this comment.
Similar to the duplicate suppression logic, the timestamp tracking should explicitly verify that timereported is an array before accessing the nested key to prevent potential PHP notices.
if ( !isset($content['period_start_ts']) && isset($logArray['timereported']) && is_array($logArray['timereported']) && isset($logArray['timereported'][EVTIME_TIMESTAMP]) )
$content['period_start_ts'] = $logArray['timereported'][EVTIME_TIMESTAMP];
if ( isset($logArray['timereported']) && is_array($logArray['timereported']) && isset($logArray['timereported'][EVTIME_TIMESTAMP]) )
$content['period_end_ts'] = $logArray['timereported'][EVTIME_TIMESTAMP];Agent-Logs-Url: https://github.com/rsyslog/loganalyzer/sessions/2cf4027a-cbdb-407e-8a06-bf092eaf68f5 Co-authored-by: alorbach <1675286+alorbach@users.noreply.github.com>
|
@gemini review |
@alorbach I have started the AI code review. It will take a few minutes to complete. |
|
Warning Gemini is experiencing higher than usual traffic and was unable to create the review. Please try again in a few hours by commenting |
There was a problem hiding this comment.
1 issue found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/index.php">
<violation number="1" location="src/index.php:356">
P2: The duplicate timestamp window uses `<` instead of `<=`, so records exactly at `DuplicateRecordMaxTsDistance` are not suppressed.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
… distance check Agent-Logs-Url: https://github.com/rsyslog/loganalyzer/sessions/1e048796-a278-4c79-ac25-855036f80f54 Co-authored-by: alorbach <1675286+alorbach@users.noreply.github.com>
|
@gemini review |
@alorbach I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
Code Review
This pull request introduces several enhancements to the log viewer, including a more robust duplicate message suppression system for both the main view and export functionality. It adds a new configuration setting, DuplicateRecordMaxTsDistance, to control the time window for duplicates and updates the UI to display the count of suppressed records. Additionally, the PR implements support for relative date syntax (e.g., 'T00:00:00', '-1T') in time filters and improves export file naming by including the period's start and end timestamps. Feedback focuses on optimizing configuration lookups within loops and ensuring the new timestamp distance logic is applied consistently across both the main view and the export process.
| $DuplicateRecordMaxTsDistance = GetConfigSetting("DuplicateRecordMaxTsDistance", PHP_INT_MAX, CFGLEVEL_USER); | ||
| // --- | ||
|
|
||
| // --- Init duplicate suppression state | ||
| $szLastMessage = ""; | ||
| $szLastMessageTimestamp = 0; | ||
| $duplicateCount = 0; | ||
| $duplicateCountTotal = 0; | ||
| // --- | ||
|
|
||
| //Loop through the messages! | ||
| do | ||
| { | ||
| // --- Extra stuff for suppressing messages | ||
| if ( | ||
| GetConfigSetting("SuppressDuplicatedMessages", 0, CFGLEVEL_USER) == 1 | ||
| && | ||
| isset($logArray[SYSLOG_MESSAGE]) | ||
| ) | ||
| if ( GetConfigSetting("SuppressDuplicatedMessages", 0, CFGLEVEL_USER) == 1 && isset($logArray[SYSLOG_MESSAGE]) ) |
There was a problem hiding this comment.
The call to GetConfigSetting for SuppressDuplicatedMessages inside the loop is inefficient as it performs multiple checks (session, user config, global config) on every iteration. It should be cached in a local variable outside the loop, similar to how $DuplicateRecordMaxTsDistance is handled at line 336.
$DuplicateRecordMaxTsDistance = GetConfigSetting("DuplicateRecordMaxTsDistance", PHP_INT_MAX, CFGLEVEL_USER);
$SuppressDuplicatedMessages = GetConfigSetting("SuppressDuplicatedMessages", 0, CFGLEVEL_USER) == 1;
// ---
// --- Init duplicate suppression state
$szLastMessage = "";
$szLastMessageTimestamp = 0;
$duplicateCount = 0;
$duplicateCountTotal = 0;
// ---
//Loop through the messages!
do
{
// --- Extra stuff for suppressing messages
if ( $SuppressDuplicatedMessages && isset($logArray[SYSLOG_MESSAGE]) )|
|
||
| if ( !isset($szLastMessage) ) // Only set lastmgr | ||
| $szLastMessage = $logArray[SYSLOG_MESSAGE]; | ||
| if ( $szLastMessage !== "" && $szLastMessage == $logArray[SYSLOG_MESSAGE] ) |
There was a problem hiding this comment.
The duplicate suppression logic in the export loop is missing the timestamp distance check (DuplicateRecordMaxTsDistance) that was implemented for the main view. To ensure consistency and adhere to the documentation in config.sample.php (line 108), the export suppression should also respect this distance. This requires reading the DuplicateRecordMaxTsDistance setting outside the loop and tracking the timestamp of the last non-suppressed message (szLastMessageTimestamp) to calculate the difference.
index.php:356: changed$tsDiff < $DuplicateRecordMaxTsDistanceto$tsDiff <= $DuplicateRecordMaxTsDistanceso records exactly at the configured distance are also suppressedOriginal prompt
Summary
This is a clean re-implementation of the features proposed in PR #92 (by @rusak47), which has grown stale and has merge conflicts. All 7 bugs identified by automated review must be fixed in this implementation. Do not cherry-pick the old PR code directly — re-implement each feature cleanly against the current
master.Features to Implement
1. Relative Date/Time Syntax for
datefrom/datetoFiltersFile:
src/include/functions_filters.php→GetTimeStampFromTimeString()File:
src/include/functions_common.php→GetEventTime()Add support for compact relative date expressions in addition to existing absolute formats:
T00:00:00T12:30\-1Tor-1T\-2T01:30or-2T01:30Implementation notes:
else ifbranch using regex:/(-?[0-9]{1,2})?T([0-9]{0,2}):?([0-9]{0,2}):?([0-9]{0,2})/mktime($hh, $mm, $ss), then apply the day offset viastrtotime("$days days", $szTime)if the day component is a negative integer.2. Export All Matched Pages (not just current page)
File:
src/export.phpFile:
src/include/config.sample.phpAdd a new config setting
$CFG['ExportAllMatchPages'](default0).When enabled, the export loop must iterate through all matching records, not just the current page's worth. When disabled, it keeps the existing behavior (current page only).
Implementation notes:
export.php:$content['ExportAllMatchPages'] = GetConfigSetting("ExportAllMatchPages", 0, CFGLEVEL_USER) == 1;$counter, handle theERROR_MSG_SKIPMESSAGEskip loop, then break only if!$content['ExportAllMatchPages'] && $counter >= $content['CurrentViewEntriesPerPage']. Otherwise continue until$ret != SUCCESS.while ($ret == SUCCESS)(remove the counter-based limit from the while condition).src/include/config.sample.phpwith a comment.3. Timestamped Export Filenames
File:
src/export.phpReplace the static filename
ExportMessageswith one that includes the actual time range of exported records:ExportMessages_20230101T120000-20230101T130000Implementation notes:
$content['period_start_ts'](first record's timestamp) and$content['period_end_ts'](last record's timestamp) during the export loop. Only setperiod_start_tsonce (on the first record)."ExportMessages_" . date('Ymd\THis', $content['period_start_ts']) . "-" . date('Ymd\THis', $content['period_end_ts']).isset($content['period_start_ts'])— fall back toExportMessagesif no records were exported.timereportedfield must be guarded withisset($logArray['timereported'])before accessing[EVTIME_TIMESTAMP]— this applies to all timestamp accesses in the export loop.4. Separate Export Duplicate Suppression from View Suppression
File:
src/export.phpFile:
src/include/config.sample.phpAdd
$CFG['ExportSuppressDuplicatedMessages'](default0) — distinct from the existing$CFG['SuppressDuplicatedMessages'](which controls the view).Implementation notes:
export.php, read:$content['SuppressDuplicatedMessages'] = GetConfigSetting("ExportSuppressDuplicatedMessages", 0, CFGLEVEL_USER) == 1;config.sample.php.5. Fix Duplicate Message Suppression in View (
src/index.php)File:
src/index.phpThe existing "Suppress duplicated messages" logic is broken. Re-implement it cleanly, fixing all 7 known bugs:
Bug fixes required:
timereportednot guarded — Before accessing$logArray["timereported"][EVTIME_TIMESTAMP], always checkisset($logArray["timereported"]). If absent, treat the timestamp as0(no timestamp-based distance check).Pending duplicate count dropped at boundary — After the main
do...whileloop exits, if$duplicateCount > 0(there are unseen pending duplicates from a trailing run), inject the suppression summary row before the loop ends. This flush must happen outside the loop body.Off-by-one in suppression threshold — Change
if ($duplicateCount > 1)toif ($duplicateCount > 0)— a single duplicate should still be reported/counted.Initialization — Initialize
$szLastMessage = "",$szLastMessageTimestamp = 0,$duplicateCount = 0,$duplicateCountTotal = 0before the loop.DuplicateRecordMaxTsDistanceconfig — Rea...This pull request was created from Copilot chat.