Fix #753: Enable CDEF/calculated data source value alerting - #798
Open
bmfmancini wants to merge 2 commits into
Open
Fix #753: Enable CDEF/calculated data source value alerting#798bmfmancini wants to merge 2 commits into
bmfmancini wants to merge 2 commits into
Conversation
thold_modify_values_by_cdef() had two logic bugs that prevented CDEF-based thresholds from triggering: 1. Contradictory condition: The CDEF was only looked up from graph items when data_type != 1 OR cdef was empty, but only applied when data_type == 1. When a user explicitly set a CDEF on a threshold (data_type=1, cdef set), the lookup was skipped and $cdef stayed false, so the CDEF was never applied to the threshold values. 2. Double transformation: lastread was already CDEF-transformed by thold_poller_output (or thold_process.php for daemon mode) before being stored in the database. thold_modify_values_by_cdef() then applied the CDEF to lastread again, producing an incorrect value. The fix restructures the CDEF lookup to use the threshold's explicitly configured CDEF when data_type=1, falling back to auto-detection from graph items. It also removes the redundant lastread transformation, applying the CDEF only to the threshold values (thold_hi, thold_low, etc.) so the comparison between the already-transformed lastread and the threshold values is consistent.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #753
Problem
Thresholds configured against data sources that use CDEF (calculated) values do not trigger alerts. The threshold evaluation compares against the raw RRD value rather than the CDEF-transformed value, so alerts never fire when the threshold condition is only met after the CDEF calculation.
Root Cause
thold_modify_values_by_cdef()inthold_functions.phphad two logic bugs:1. Contradictory CDEF lookup condition
The original code:
When a user explicitly configures a CDEF on a threshold (
data_type=1,cdefset), the firstifevaluates tofalse || false = false, so the DB query is skipped and$cdefremainsfalse. The secondifthen checks$cdef !== falsewhich fails, so the CDEF is never applied to the threshold values (thold_hi,thold_low, etc.).This means the already-CDEF-transformed
lastreadwas compared against raw threshold values, causing alerts to never fire (or fire incorrectly).2. Double transformation of
lastreadlastreadis already CDEF-transformed bythold_poller_output()(orthold_process.phpin daemon mode) before being stored in the database.thold_modify_values_by_cdef()then applied the CDEF tolastreadagain, producing an incorrect (double-transformed) value.Fix
Restructured
thold_modify_values_by_cdef():data_type=1andcdefis set, falling back to auto-detection from graph items only whencdefis empty.lastreadtransformation —lastreadis already CDEF-transformed before storage. The CDEF is now applied only to the threshold values (thold_hi,thold_low,thold_warning_hi,thold_warning_low,time_hi,time_low, etc.) so the comparison is consistent.Testing
thold_poller_output→thold_check_threshold) and the daemon path (thold_process.php→thold_check_threshold) apply the CDEF tolastreadbefore storage, confirming the double-transformation was real.