Fixed #748 support for nagios range based syntax in performance data

Author: mickem

include/nscapi/protobuf/functions_perfdata.cpp

@@ -67,22 +67,41 @@ struct perf_builder : parsers::perfdata::builder {
   void set_unit(const std::string &value) override {
     if (lastPerf) lastPerf->mutable_float_value()->set_unit(value);
   }
+  void set_warning_range(const std::string &range) override {
+    if (lastPerf) lastPerf->mutable_float_value()->set_warning_range(range);
+  }
+  void set_critical_range(const std::string &range) override {
+    if (lastPerf) lastPerf->mutable_float_value()->set_critical_range(range);
+  }
   void next() override {}
 };
 
 void parse_float_perf_value(std::stringstream &ss, const PB::Common::PerformanceData_FloatValue &val) {
   ss << str::xtos_non_sci(val.value());
   if (!val.unit().empty()) ss << val.unit();
-  if (!val.has_warning() && !val.has_critical() && !val.has_minimum() && !val.has_maximum()) {
+  // `*_range` fields preserve the original Nagios range syntax for
+  // warning/critical (issue #748). When set they take precedence over the
+  // numeric `warning`/`critical` siblings, which carry only the lower
+  // bound for back-compat. min/max are spec'd as single values so they're
+  // numeric-only.
+  const bool has_warning = val.has_warning() || !val.warning_range().empty();
+  const bool has_critical = val.has_critical() || !val.critical_range().empty();
+  if (!has_warning && !has_critical && !val.has_minimum() && !val.has_maximum()) {
     return;
   }
   ss << ";";
-  if (val.has_warning()) ss << str::xtos_non_sci(val.warning().value());
-  if (!val.has_critical() && !val.has_minimum() && !val.has_maximum()) {
+  if (!val.warning_range().empty())
+    ss << val.warning_range();
+  else if (val.has_warning())
+    ss << str::xtos_non_sci(val.warning().value());
+  if (!has_critical && !val.has_minimum() && !val.has_maximum()) {
     return;
   }
   ss << ";";
-  if (val.has_critical()) ss << str::xtos_non_sci(val.critical().value());
+  if (!val.critical_range().empty())
+    ss << val.critical_range();
+  else if (val.has_critical())
+    ss << str::xtos_non_sci(val.critical().value());
   if (!val.has_minimum() && !val.has_maximum()) {
     return;
   }

include/nscapi/protobuf/functions_perfdata_test.cpp

@@ -408,3 +408,50 @@ TEST(PerfDataBuildTest, build_with_no_string_value) {
   const auto result = nscapi::protobuf::functions::build_performance_data(line, nscapi::protobuf::functions::no_truncation);
   EXPECT_EQ("'test'=", result);
 }
+
+// =================================================================
+// Threshold-range round-trip (issue #748)
+//
+// External-script stdout like
+//   'FOO'=10;4:5;6:9
+// used to round-trip as
+//   'FOO'=10;4;6
+// because trim_to_double truncated at the colon and the protobuf had no
+// place to carry the original syntax. These tests pin the fixed behaviour
+// end-to-end: parse a string into protobuf, format it back, and compare.
+// =================================================================
+
+TEST(PerfDataRangeRoundTrip, simple_warning_range) { EXPECT_EQ("'FOO'=10;4:5;6:9", do_parse("'FOO'=10;4:5;6:9")); }
+
+TEST(PerfDataRangeRoundTrip, open_lower_bound) { EXPECT_EQ("'x'=1;10:;:20", do_parse("'x'=1;10:;:20")); }
+
+TEST(PerfDataRangeRoundTrip, inverted_and_infinity) { EXPECT_EQ("'x'=1;@10:20;~:30", do_parse("'x'=1;@10:20;~:30")); }
+
+TEST(PerfDataRangeRoundTrip, range_with_unit) { EXPECT_EQ("'x'=5s;4:6;7:8", do_parse("'x'=5s;4:6;7:8")); }
+
+TEST(PerfDataRangeRoundTrip, mixed_numeric_warning_range_critical) {
+  // Numeric warning, range critical: both must survive separately.
+  EXPECT_EQ("'x'=10;5;6:9", do_parse("'x'=10;5;6:9"));
+}
+
+TEST(PerfDataRangeRoundTrip, range_with_min_max) {
+  // min/max are spec'd as single numbers, not ranges - keep them numeric.
+  EXPECT_EQ("'x'=50%;@10:90;:95;0;100", do_parse("'x'=50%;@10:90;:95;0;100"));
+}
+
+// Builder-level test: setting the range explicitly via the protobuf
+// makes the formatter emit it verbatim, regardless of what was put in the
+// numeric warning/critical sibling.
+TEST(PerfDataBuildTest, build_range_takes_precedence_over_numeric) {
+  PB::Commands::QueryResponseMessage::Response::Line line;
+  auto* perf = line.add_perf();
+  perf->set_alias("metric");
+  perf->mutable_float_value()->set_value(50);
+  perf->mutable_float_value()->mutable_warning()->set_value(4);  // would be the lower bound
+  perf->mutable_float_value()->set_warning_range("4:5");
+  perf->mutable_float_value()->mutable_critical()->set_value(6);
+  perf->mutable_float_value()->set_critical_range("6:9");
+
+  const auto result = nscapi::protobuf::functions::build_performance_data(line, nscapi::protobuf::functions::no_truncation);
+  EXPECT_EQ("'metric'=50;4:5;6:9", result);
+}

include/parsers/perfdata.hpp

@@ -18,6 +18,15 @@ struct builder {
   virtual void set_maximum(double value) = 0;
   virtual void set_unit(const std::string &value) = 0;
 
+  // Threshold-range setters (Nagios range syntax like "4:5", ":10",
+  // "@0:90", "~:10"). Defaulted to no-ops so existing builders compile
+  // unchanged; the parser still calls set_warning / set_critical with the
+  // lower bound for back-compat. Builders that need to preserve the full
+  // syntax (notably the protobuf builder used by external-script paths)
+  // override these to carry the string through. See GitHub issue #748.
+  virtual void set_warning_range(const std::string & /*range*/) {}
+  virtual void set_critical_range(const std::string & /*range*/) {}
+
   virtual void next() = 0;
 
   virtual void add_string(std::string alias, std::string value) = 0;
@@ -37,6 +46,25 @@ inline double trim_to_double(std::string s) {
   }
 }
 
+// True if the threshold field uses Nagios range syntax (anything beyond a
+// simple number with optional UOM). The Nagios plugin development
+// guidelines define ranges as:
+//   value       single value, treated as 0..value
+//   low:high    alert if outside [low, high]
+//   :high       alert if value > high
+//   low:        alert if value < low
+//   @low:high   alert if value is inside [low, high] (inverted)
+//   ~ marker    -infinity (e.g. ~:10 = alert when > 10)
+// We don't try to parse the range here - we only detect that it IS a
+// range so the parser preserves the original string. The numeric float
+// (lower bound) is still set so consumers that only read the float don't
+// regress.
+inline bool is_threshold_range(const std::string &s) {
+  if (s.empty()) return false;
+  if (s[0] == '@' || s[0] == '~') return true;
+  return s.find(':') != std::string::npos;
+}
+
 inline void parse(std::shared_ptr<builder> builder, const std::string &perff) {
   std::string perf = perff;
   // TODO: make this work with const!
@@ -115,8 +143,18 @@ inline void parse(std::shared_ptr<builder> builder, const std::string &perff) {
       builder->set_value(trim_to_double(fitem.second.substr(0, pend)));
       builder->set_unit(fitem.second.substr(pend));
     }
-    if (items.size() >= 2 && !items[1].empty()) builder->set_warning(trim_to_double(items[1]));
-    if (items.size() >= 3 && !items[2].empty()) builder->set_critical(trim_to_double(items[2]));
+    // Warning / critical can be range syntax (e.g. "4:5"). Set the numeric
+    // lower bound for back-compat AND, when the field is a range, forward
+    // the original string so the formatter can round-trip it (issue #748).
+    if (items.size() >= 2 && !items[1].empty()) {
+      builder->set_warning(trim_to_double(items[1]));
+      if (is_threshold_range(items[1])) builder->set_warning_range(items[1]);
+    }
+    if (items.size() >= 3 && !items[2].empty()) {
+      builder->set_critical(trim_to_double(items[2]));
+      if (is_threshold_range(items[2])) builder->set_critical_range(items[2]);
+    }
+    // min/max are single values per the Nagios spec - no range syntax.
     if (items.size() >= 4 && !items[3].empty()) builder->set_minimum(trim_to_double(items[3]));
     if (items.size() >= 5 && !items[4].empty()) builder->set_maximum(trim_to_double(items[4]));
     builder->next();

include/parsers/perfdata_test.cpp

@@ -34,6 +34,8 @@ struct perf_entry {
   double minimum = -1.0;
   double maximum = -1.0;
   std::string unit;
+  std::string warning_range;
+  std::string critical_range;
   bool has_warning = false;
   bool has_critical = false;
   bool has_minimum = false;
@@ -72,6 +74,8 @@ struct test_builder final : parsers::perfdata::builder {
     current_.has_maximum = true;
   }
   void set_unit(const std::string& value) override { current_.unit = value; }
+  void set_warning_range(const std::string& range) override { current_.warning_range = range; }
+  void set_critical_range(const std::string& range) override { current_.critical_range = range; }
   void next() override {
     entries.push_back(current_);
     current_ = perf_entry();
@@ -377,3 +381,73 @@ TEST_F(PerfDataParserTest, ValueIsOnlyUnit) {
   ASSERT_EQ(1, b->entries.size());
   EXPECT_DOUBLE_EQ(0.0, b->entries[0].value);
 }
+
+// ==============================================================
+// is_threshold_range
+// ==============================================================
+
+TEST(IsThresholdRange, PlainNumberIsNotARange) { EXPECT_FALSE(parsers::perfdata::is_threshold_range("10")); }
+TEST(IsThresholdRange, NumberWithUnitIsNotARange) { EXPECT_FALSE(parsers::perfdata::is_threshold_range("10s")); }
+TEST(IsThresholdRange, EmptyIsNotARange) { EXPECT_FALSE(parsers::perfdata::is_threshold_range("")); }
+TEST(IsThresholdRange, ColonInsideIsRange) { EXPECT_TRUE(parsers::perfdata::is_threshold_range("4:5")); }
+TEST(IsThresholdRange, ColonPrefixIsRange) { EXPECT_TRUE(parsers::perfdata::is_threshold_range(":10")); }
+TEST(IsThresholdRange, ColonSuffixIsRange) { EXPECT_TRUE(parsers::perfdata::is_threshold_range("10:")); }
+TEST(IsThresholdRange, InvertedPrefixIsRange) { EXPECT_TRUE(parsers::perfdata::is_threshold_range("@10:20")); }
+TEST(IsThresholdRange, NegativeInfinityPrefixIsRange) { EXPECT_TRUE(parsers::perfdata::is_threshold_range("~:10")); }
+
+// ==============================================================
+// parse — threshold ranges (issue #748)
+// ==============================================================
+
+// The parser must preserve the original range string AND set the numeric
+// lower bound so consumers that read only the float don't regress.
+TEST_F(PerfDataParserTest, RangeWarningPreservesColon) {
+  parsers::perfdata::parse(b, "'FOO'=10;4:5;6:9");
+  ASSERT_EQ(1, b->entries.size());
+  EXPECT_EQ("FOO", b->entries[0].alias);
+  EXPECT_DOUBLE_EQ(10.0, b->entries[0].value);
+  // Numeric back-compat: lower bound of the range.
+  EXPECT_TRUE(b->entries[0].has_warning);
+  EXPECT_DOUBLE_EQ(4.0, b->entries[0].warning);
+  EXPECT_TRUE(b->entries[0].has_critical);
+  EXPECT_DOUBLE_EQ(6.0, b->entries[0].critical);
+  // Full range syntax preserved.
+  EXPECT_EQ("4:5", b->entries[0].warning_range);
+  EXPECT_EQ("6:9", b->entries[0].critical_range);
+}
+
+TEST_F(PerfDataParserTest, RangeWarningOpenBounds) {
+  // 10: = "alert if value < 10", :20 = "alert if value > 20"
+  parsers::perfdata::parse(b, "'x'=1;10:;:20");
+  ASSERT_EQ(1, b->entries.size());
+  EXPECT_EQ("10:", b->entries[0].warning_range);
+  EXPECT_EQ(":20", b->entries[0].critical_range);
+}
+
+TEST_F(PerfDataParserTest, RangeInvertedAndInfinity) {
+  parsers::perfdata::parse(b, "'x'=1;@10:20;~:30");
+  ASSERT_EQ(1, b->entries.size());
+  EXPECT_EQ("@10:20", b->entries[0].warning_range);
+  EXPECT_EQ("~:30", b->entries[0].critical_range);
+}
+
+TEST_F(PerfDataParserTest, RangeWithUnitSurvives) {
+  parsers::perfdata::parse(b, "'x'=5s;4:6;7:8");
+  ASSERT_EQ(1, b->entries.size());
+  EXPECT_DOUBLE_EQ(5.0, b->entries[0].value);
+  EXPECT_EQ("s", b->entries[0].unit);
+  EXPECT_EQ("4:6", b->entries[0].warning_range);
+  EXPECT_EQ("7:8", b->entries[0].critical_range);
+}
+
+// Important regression guard: plain-numeric thresholds must NOT set the
+// range fields. Otherwise we'd pollute the wire with empty-looking ranges
+// for every existing producer.
+TEST_F(PerfDataParserTest, PlainNumericThresholdsDoNotSetRange) {
+  parsers::perfdata::parse(b, "'x'=1;5;10");
+  ASSERT_EQ(1, b->entries.size());
+  EXPECT_DOUBLE_EQ(5.0, b->entries[0].warning);
+  EXPECT_DOUBLE_EQ(10.0, b->entries[0].critical);
+  EXPECT_TRUE(b->entries[0].warning_range.empty());
+  EXPECT_TRUE(b->entries[0].critical_range.empty());
+}

libs/protobuf/common.proto

@@ -157,6 +157,16 @@ option optimize_for = LITE_RUNTIME;
 			OptionalFloat minimum = 6;
 			// The highest possible value
 			OptionalFloat maximum = 7;
+			// Original Nagios range syntax for the warning threshold, e.g.
+			// "4:5", ":10", "@0:90" or "~:10". When non-empty, formatters
+			// emit this verbatim and ignore the numeric `warning` sibling.
+			// Parsers set both: the float is the lower bound of the range
+			// (for back-compat with consumers that only read the numeric
+			// field) and this string carries the full syntax through the
+			// pipeline. See GitHub issue #748.
+			string warning_range = 8;
+			// Same semantics as warning_range, for the critical threshold.
+			string critical_range = 9;
 		}
 		// The name of the value
 		string alias = 1;

modules/CheckHelpers/CheckHelpers.cpp

@@ -496,12 +496,20 @@ struct filter_obj {
     if (data.has_string_value()) return data.string_value().value();
     return "";
   }
+  // %(warn) / %(crit) template substitutions: prefer the original Nagios
+  // range syntax (e.g. "4:5") when present, fall back to the numeric
+  // lower bound for plain-number thresholds (issue #748). Without this,
+  // `render_perf` would replace a "4:5" warning with just "4".
   std::string get_warn() const {
-    if (data.has_float_value() && data.float_value().has_warning()) return str::xtos(data.float_value().warning().value());
+    if (!data.has_float_value()) return "";
+    if (!data.float_value().warning_range().empty()) return data.float_value().warning_range();
+    if (data.float_value().has_warning()) return str::xtos(data.float_value().warning().value());
     return "";
   }
   std::string get_crit() const {
-    if (data.has_float_value() && data.float_value().has_critical()) return str::xtos(data.float_value().critical().value());
+    if (!data.has_float_value()) return "";
+    if (!data.float_value().critical_range().empty()) return data.float_value().critical_range();
+    if (data.float_value().has_critical()) return str::xtos(data.float_value().critical().value());
     return "";
   }
   std::string get_max() const {

modules/WEBServer/legacy_command_controller.cpp

@@ -63,13 +63,24 @@ void legacy_command_controller::handle_query(Mongoose::Request &request, boost::
         json::object perf;
         perf["alias"] = p.alias();
         if (p.has_float_value()) {
+          const auto &fv = p.float_value();
           json::object float_value;
-          float_value["value"] = p.float_value().value();
-          if (p.float_value().has_minimum()) float_value["minimum"] = p.float_value().minimum().value();
-          if (p.float_value().has_maximum()) float_value["maximum"] = p.float_value().maximum().value();
-          if (p.float_value().has_warning()) float_value["warning"] = p.float_value().warning().value();
-          if (p.float_value().has_critical()) float_value["critical"] = p.float_value().critical().value();
-          float_value["unit"] = p.float_value().unit();
+          float_value["value"] = fv.value();
+          if (fv.has_minimum()) float_value["minimum"] = fv.minimum().value();
+          if (fv.has_maximum()) float_value["maximum"] = fv.maximum().value();
+          // Prefer the original Nagios range syntax over the numeric
+          // lower bound so the API doesn't silently drop "4:5"-style
+          // thresholds (issue #748). Same shape as the v2 queries
+          // endpoint in query_controller.cpp.
+          if (!fv.warning_range().empty())
+            float_value["warning"] = fv.warning_range();
+          else if (fv.has_warning())
+            float_value["warning"] = fv.warning().value();
+          if (!fv.critical_range().empty())
+            float_value["critical"] = fv.critical_range();
+          else if (fv.has_critical())
+            float_value["critical"] = fv.critical().value();
+          float_value["unit"] = fv.unit();
           perf["float_value"] = float_value;
         }
         if (p.has_string_value()) {

modules/WEBServer/query_controller.cpp

@@ -158,12 +158,25 @@ void query_controller::execute_query(std::string module, arg_vector args, Mongoo
         json::object pdata;
 
         if (p.has_float_value()) {
-          pdata["value"] = p.float_value().value();
-          if (p.float_value().has_minimum()) pdata["minimum"] = p.float_value().minimum().value();
-          if (p.float_value().has_maximum()) pdata["maximum"] = p.float_value().maximum().value();
-          if (p.float_value().has_warning()) pdata["warning"] = p.float_value().warning().value();
-          if (p.float_value().has_critical()) pdata["critical"] = p.float_value().critical().value();
-          pdata["unit"] = p.float_value().unit();
+          const auto &fv = p.float_value();
+          pdata["value"] = fv.value();
+          if (fv.has_minimum()) pdata["minimum"] = fv.minimum().value();
+          if (fv.has_maximum()) pdata["maximum"] = fv.maximum().value();
+          // Threshold fields carry Nagios range syntax when the original
+          // input used it (e.g. "4:5") - prefer that string over the
+          // numeric lower bound so the API faithfully reports what the
+          // upstream plugin emitted (issue #748). JSON consumers see
+          // `number` for plain thresholds and `string` for range syntax;
+          // the api.ts type mirrors that union.
+          if (!fv.warning_range().empty())
+            pdata["warning"] = fv.warning_range();
+          else if (fv.has_warning())
+            pdata["warning"] = fv.warning().value();
+          if (!fv.critical_range().empty())
+            pdata["critical"] = fv.critical_range();
+          else if (fv.has_critical())
+            pdata["critical"] = fv.critical().value();
+          pdata["unit"] = fv.unit();
         }
         if (p.has_string_value()) {
           pdata["value"] = p.string_value().value();