Pretty-print values in the PCP DynamicMeter code

Several improvements to the way values are displayed in the
PCP platform DynamicMeter implementation:
- handle the initial 'caption' setting as with regular meters,
  this required a new meter callback because we no longer have
  just a single meter caption for the DynamicMeter case
- if no label is provided for a metric in a configuration file
  use the short form metric name as a fallback
- honour the suffix setting in the configuration file
- convert metric values to the canonical units for htop (kbyte
  and seconds), and use Meter_humanUnit when it makes sense to
  do so.

Also improves the handling of fatal string error messages in a
couple of places, thanks to BenBE for the review feedback.

8 files changed, 129 insertions, 54 deletions

diff --git a/DynamicMeter.c b/DynamicMeter.c
index 7b39fdaa..3b567a53 100644
--- a/DynamicMeter.c
+++ b/DynamicMeter.c
@@ -70,12 +70,27 @@ static void DynamicMeter_display(const Object* cast, RichString* out) {
    Platform_dynamicMeterDisplay(meter, out);
 }
 
+static const char* DynamicMeter_getCaption(const Meter* this) {
+   const ProcessList* pl = this->pl;
+   const DynamicMeter* meter = Hashtable_get(pl->dynamicMeters, this->param);
+   if (meter)
+      return meter->caption ? meter->caption : meter->name;
+   return this->caption;
+}
+
 static void DynamicMeter_getUiName(const Meter* this, char* name, size_t length) {
    const ProcessList* pl = this->pl;
    const DynamicMeter* meter = Hashtable_get(pl->dynamicMeters, this->param);
    if (meter) {
-      const char* uiName = meter->caption ? meter->caption : meter->name;
-      xSnprintf(name, length, "%s", uiName);
+      const char* uiName = meter->caption;
+      if (uiName) {
+         int len = strlen(uiName);
+         if (len > 2 && uiName[len-2] == ':')
+            len -= 2;
+         xSnprintf(name, length, "%.*s", len, uiName);
+      } else {
+         xSnprintf(name, length, "%s", meter->name);
+      }
    }
 }
 
@@ -87,6 +102,7 @@ const MeterClass DynamicMeter_class = {
    },
    .init = DynamicMeter_init,
    .updateValues = DynamicMeter_updateValues,
+   .getCaption = DynamicMeter_getCaption,
    .getUiName = DynamicMeter_getUiName,
    .defaultMode = TEXT_METERMODE,
    .maxItems = 0,
diff --git a/Meter.c b/Meter.c
index 0052773c..5e3a00d0 100644
--- a/Meter.c
+++ b/Meter.c
@@ -153,11 +153,12 @@ ListItem* Meter_toListItem(const Meter* this, bool moving) {
 /* ---------- TextMeterMode ---------- */
 
 static void TextMeterMode_draw(Meter* this, int x, int y, int w) {
+   const char* caption = Meter_getCaption(this);
    attrset(CRT_colors[METER_TEXT]);
-   mvaddnstr(y, x, this->caption, w - 1);
+   mvaddnstr(y, x, caption, w - 1);
    attrset(CRT_colors[RESET_COLOR]);
 
-   int captionLen = strlen(this->caption);
+   int captionLen = strlen(caption);
    x += captionLen;
    w -= captionLen;
    if (w <= 0)
@@ -174,10 +175,11 @@ static void TextMeterMode_draw(Meter* this, int x, int y, int w) {
 static const char BarMeterMode_characters[] = "|#*@$%&.";
 
 static void BarMeterMode_draw(Meter* this, int x, int y, int w) {
+   const char* caption = Meter_getCaption(this);
    w -= 2;
    attrset(CRT_colors[METER_TEXT]);
    int captionLen = 3;
-   mvaddnstr(y, x, this->caption, captionLen);
+   mvaddnstr(y, x, caption, captionLen);
    x += captionLen;
    w -= captionLen;
    attrset(CRT_colors[BAR_BORDER]);
@@ -306,9 +308,10 @@ static void GraphMeterMode_draw(Meter* this, int x, int y, int w) {
       GraphMeterMode_pixPerRow = PIXPERROW_ASCII;
    }
 
+   const char* caption = Meter_getCaption(this);
    attrset(CRT_colors[METER_TEXT]);
    int captionLen = 3;
-   mvaddnstr(y, x, this->caption, captionLen);
+   mvaddnstr(y, x, caption, captionLen);
    x += captionLen;
    w -= captionLen;
 
@@ -393,8 +396,9 @@ static void LEDMeterMode_draw(Meter* this, int x, int y, ATTR_UNUSED int w) {
 #endif
       y + 2;
    attrset(CRT_colors[LED_COLOR]);
-   mvaddstr(yText, x, this->caption);
-   int xx = x + strlen(this->caption);
+   const char* caption = Meter_getCaption(this);
+   mvaddstr(yText, x, caption);
+   int xx = x + strlen(caption);
    int len = RichString_sizeVal(out);
    for (int i = 0; i < len; i++) {
       int c = RichString_getCharVal(out, i);
diff --git a/Meter.h b/Meter.h
index 40cad332..06c87388 100644
--- a/Meter.h
+++ b/Meter.h
@@ -53,6 +53,7 @@ typedef void(*Meter_Done)(Meter*);
 typedef void(*Meter_UpdateMode)(Meter*, int);
 typedef void(*Meter_UpdateValues)(Meter*);
 typedef void(*Meter_Draw)(Meter*, int, int, int);
+typedef const char*(*Meter_GetCaption)(const Meter*);
 typedef void(*Meter_GetUiName)(const Meter*, char*, size_t);
 
 typedef struct MeterClass_ {
@@ -62,6 +63,7 @@ typedef struct MeterClass_ {
    const Meter_UpdateMode updateMode;
    const Meter_UpdateValues updateValues;
    const Meter_Draw draw;
+   const Meter_GetCaption getCaption;
    const Meter_GetUiName getUiName;
    const int defaultMode;
    const double total;
@@ -84,6 +86,8 @@ typedef struct MeterClass_ {
 #define Meter_updateValues(this_)      As_Meter(this_)->updateValues((Meter*)(this_))
 #define Meter_getUiNameFn(this_)       As_Meter(this_)->getUiName
 #define Meter_getUiName(this_,n_,l_)   As_Meter(this_)->getUiName((const Meter*)(this_),n_,l_)
+#define Meter_getCaptionFn(this_)      As_Meter(this_)->getCaption
+#define Meter_getCaption(this_)        (Meter_getCaptionFn(this_) ? As_Meter(this_)->getCaption((const Meter*)(this_)) : (this_)->caption)
 #define Meter_defaultMode(this_)       As_Meter(this_)->defaultMode
 #define Meter_attributes(this_)        As_Meter(this_)->attributes
 #define Meter_name(this_)              As_Meter(this_)->name
diff --git a/pcp/PCPDynamicMeter.c b/pcp/PCPDynamicMeter.c
index 511d964e..05b71d5c 100644
--- a/pcp/PCPDynamicMeter.c
+++ b/pcp/PCPDynamicMeter.c
@@ -42,6 +42,7 @@ static PCPDynamicMetric* PCPDynamicMeter_lookupMetric(PCPDynamicMeters* meters,
    metric = &meter->metrics[n-1];
    memset(metric, 0, sizeof(PCPDynamicMetric));
    metric->name = metricName;
+   metric->label = String_cat(name, ": ");
    metric->id = meters->offset + meters->cursor;
    meters->cursor++;
 
@@ -65,13 +66,14 @@ static void PCPDynamicMeter_parseMetric(PCPDynamicMeters* meters, PCPDynamicMete
       /* use derived metrics in dynamic meters for simplicity */
       char* error;
       if (pmRegisterDerivedMetric(metric->name, value, &error) < 0) {
-         char note[1024];
-         xSnprintf(note, sizeof(note),
-                   "failed to parse expression in %s at line %u\n%s\n%s",
-                   path, line, error, pmGetProgname());
+         char* note;
+         xAsprintf(&note,
+                   "%s: failed to parse expression in %s at line %u\n%s\n",
+                   pmGetProgname(), path, line, error);
          free(error);
          errno = EINVAL;
          CRT_fatalError(note);
+         free(note);
       }
    } else {
       /* this is a property of a dynamic metric - the metric expression */
@@ -97,7 +99,9 @@ static void PCPDynamicMeter_parseMetric(PCPDynamicMeters* meters, PCPDynamicMete
          else if (String_eq(value, "white"))
              metric->color = DYNAMIC_WHITE;
       } else if (String_eq(p, "label")) {
-         free_and_xStrdup(&metric->label, value);
+         char* label = String_cat(value, ": ");
+         free_and_xStrdup(&metric->label, label);
+         free(label);
       } else if (String_eq(p, "suffix")) {
          free_and_xStrdup(&metric->suffix, value);
       }
@@ -112,12 +116,13 @@ static void PCPDynamicMeter_validateMeterName(char* key, const char *path, unsig
    if (end) {
       *end = '\0';
    } else {
-      char note[1024];
-      xSnprintf(note, sizeof(note),
-                "No closing brace on meter name at %s line %u\n\"%s\"",
-                path, line, key);
+      char* note;
+      xAsprintf(&note,
+                "%s: no closing brace on meter name at %s line %u\n\"%s\"",
+                pmGetProgname(), path, line, key);
       errno = EINVAL;
       CRT_fatalError(note);
+      free(note);
    }
 
    while (*p) {
@@ -131,12 +136,13 @@ static void PCPDynamicMeter_validateMeterName(char* key, const char *path, unsig
       p++;
    }
    if (*p != '\0') { /* badness */
-      char note[1024];
-      xSnprintf(note, sizeof(note),
-                "Invalid meter name at %s line %u\n\"%s\"",
-                path, line, key);
+      char* note;
+      xAsprintf(&note,
+                "%s: invalid meter name at %s line %u\n\"%s\"",
+                pmGetProgname(), path, line, key);
       errno = EINVAL;
       CRT_fatalError(note);
+      free(note);
    } else { /* overwrite closing brace */
       *p = '\0';
    }
@@ -182,7 +188,9 @@ static void PCPDynamicMeter_parseFile(PCPDynamicMeters* meters, const char* path
          PCPDynamicMeter_validateMeterName(key+1, path, lineno);
          meter = PCPDynamicMeter_new(meters, key+1);
       } else if (value && String_eq(key, "caption")) {
-         free_and_xStrdup(&meter->super.caption, value);
+         char* caption = String_cat(value, ": ");
+         free_and_xStrdup(&meter->super.caption, caption);
+         free(caption);
       } else if (value && String_eq(key, "description")) {
          free_and_xStrdup(&meter->super.description, value);
       } else if (value && String_eq(key, "type")) {
@@ -272,39 +280,66 @@ void PCPDynamicMeter_updateValues(PCPDynamicMeter* this, Meter* meter) {
 
       PCPDynamicMetric* metric = &this->metrics[i];
       const pmDesc* desc = Metric_desc(metric->id);
-      pmAtomValue atom;
+      pmAtomValue atom, raw;
 
-      if (!Metric_values(metric->id, &atom, 1, desc->type)) {
+      if (!Metric_values(metric->id, &raw, 1, desc->type)) {
          bytes--; /* clear the separator */
          continue;
       }
-      /* TODO: pretty-print the values - pmConvScale, etc */
+
+      pmUnits conv = desc->units;  /* convert to canonical units */
+      if (conv.dimSpace)
+         conv.scaleSpace = PM_SPACE_KBYTE;
+      if (conv.dimTime)
+         conv.scaleTime = PM_TIME_SEC;
+      if (desc->type == PM_TYPE_STRING)
+         atom = raw;
+      else if (pmConvScale(desc->type, &raw, &desc->units, &atom, &conv) < 0) {
+         bytes--; /* clear the separator */
+         continue;
+      }
+
+      size_t saved = bytes;
       switch (desc->type) {
          case PM_TYPE_STRING:
             bytes += xSnprintf(buffer + bytes, size - bytes, "%s", atom.cp);
             free(atom.cp);
             break;
          case PM_TYPE_32:
-            bytes += xSnprintf(buffer + bytes, size - bytes, "%d", atom.l);
+            bytes += conv.dimSpace ?
+               Meter_humanUnit(buffer + bytes, atom.l, size - bytes) :
+               xSnprintf(buffer + bytes, size - bytes, "%d", atom.l);
             break;
          case PM_TYPE_U32:
-            bytes += xSnprintf(buffer + bytes, size - bytes, "%u", atom.ul);
+            bytes += conv.dimSpace ?
+               Meter_humanUnit(buffer + bytes, atom.ul, size - bytes) :
+               xSnprintf(buffer + bytes, size - bytes, "%u", atom.ul);
             break;
          case PM_TYPE_64:
-            bytes += xSnprintf(buffer + bytes, size - bytes, "%lld", (long long) atom.ll);
+            bytes += conv.dimSpace ?
+               Meter_humanUnit(buffer + bytes, atom.ll, size - bytes) :
+               xSnprintf(buffer + bytes, size - bytes, "%lld", (long long) atom.ll);
             break;
          case PM_TYPE_U64:
-            bytes += xSnprintf(buffer + bytes, size - bytes, "%llu", (unsigned long long) atom.ull);
+            bytes += conv.dimSpace ?
+               Meter_humanUnit(buffer + bytes, atom.ull, size - bytes) :
+               xSnprintf(buffer + bytes, size - bytes, "%llu", (unsigned long long) atom.ull);
             break;
          case PM_TYPE_FLOAT:
-            bytes += xSnprintf(buffer + bytes, size - bytes, "%f", (double) atom.f);
+            bytes += conv.dimSpace ?
+               Meter_humanUnit(buffer + bytes, atom.f, size - bytes) :
+               xSnprintf(buffer + bytes, size - bytes, "%.2f", (double) atom.f);
             break;
          case PM_TYPE_DOUBLE:
-            bytes += xSnprintf(buffer + bytes, size - bytes, "%f", atom.d);
+            bytes += conv.dimSpace ?
+               Meter_humanUnit(buffer + bytes, atom.d, size - bytes) :
+               xSnprintf(buffer + bytes, size - bytes, "%.2f", atom.d);
             break;
          default:
             break;
       }
+      if (saved != bytes && metric->suffix)
+         bytes += xSnprintf(buffer + bytes, size - bytes, "%s", metric->suffix);
    }
    if (!bytes)
       xSnprintf(buffer, size, "no data");
@@ -316,52 +351,74 @@ void PCPDynamicMeter_display(PCPDynamicMeter* this, ATTR_UNUSED const Meter* met
    for (unsigned int i = 0; i < this->totalMetrics; i++) {
       PCPDynamicMetric* metric = &this->metrics[i];
       const pmDesc* desc = Metric_desc(metric->id);
-      pmAtomValue atom;
+      pmAtomValue atom, raw;
       char buffer[64];
-      int len;
 
-      if (!Metric_values(metric->id, &atom, 1, desc->type))
+      if (!Metric_values(metric->id, &raw, 1, desc->type))
          continue;
-      nodata = 0;
+
+      pmUnits conv = desc->units;  /* convert to canonical units */
+      if (conv.dimSpace)
+         conv.scaleSpace = PM_SPACE_KBYTE;
+      if (conv.dimTime)
+         conv.scaleTime = PM_TIME_SEC;
+      if (desc->type == PM_TYPE_STRING)
+         atom = raw;
+      else if (pmConvScale(desc->type, &raw, &desc->units, &atom, &conv) < 0)
+         continue;
+
+      nodata = 0;  /* we will use this metric so *some* data will be added */
 
       if (i > 0)
          RichString_appendnAscii(out, CRT_colors[metric->color], " ", 1);
 
-      if (metric->label) {
-         len = xSnprintf(buffer, sizeof(buffer), "%s ", metric->label);
-         RichString_appendnAscii(out, CRT_colors[METER_TEXT], buffer, len);
-      }
+      if (metric->label)
+         RichString_appendAscii(out, CRT_colors[METER_TEXT], metric->label);
 
-      /* TODO: pretty-print the values - pmConvScale, etc */
-      len = 0;
+      int len = 0;
       switch (desc->type) {
          case PM_TYPE_STRING:
             len = xSnprintf(buffer, sizeof(buffer), "%s", atom.cp);
             free(atom.cp);
             break;
          case PM_TYPE_32:
-            len = xSnprintf(buffer, sizeof(buffer), "%d", atom.l);
+            len = conv.dimSpace ?
+               Meter_humanUnit(buffer, atom.l, sizeof(buffer)) :
+               xSnprintf(buffer, sizeof(buffer), "%d", atom.l);
             break;
          case PM_TYPE_U32:
-            len = xSnprintf(buffer, sizeof(buffer), "%u", atom.ul);
+            len = conv.dimSpace ?
+               Meter_humanUnit(buffer, atom.ul, sizeof(buffer)) :
+               xSnprintf(buffer, sizeof(buffer), "%u", atom.ul);
             break;
          case PM_TYPE_64:
-            len = xSnprintf(buffer, sizeof(buffer), "%lld", (long long) atom.ll);
+            len = conv.dimSpace ?
+               Meter_humanUnit(buffer, atom.ll, sizeof(buffer)) :
+               xSnprintf(buffer, sizeof(buffer), "%lld", (long long) atom.ll);
             break;
          case PM_TYPE_U64:
-            len = xSnprintf(buffer, sizeof(buffer), "%llu", (unsigned long long) atom.ull);
+            len = conv.dimSpace ?
+               Meter_humanUnit(buffer, atom.ull, sizeof(buffer)) :
+               xSnprintf(buffer, sizeof(buffer), "%llu", (unsigned long long) atom.ull);
             break;
          case PM_TYPE_FLOAT:
-            len = xSnprintf(buffer, sizeof(buffer), "%f", (double)atom.f);
+            len = conv.dimSpace ?
+               Meter_humanUnit(buffer, atom.f, sizeof(buffer)) :
+               xSnprintf(buffer, sizeof(buffer), "%.2f", (double) atom.f);
             break;
          case PM_TYPE_DOUBLE:
-            len = xSnprintf(buffer, sizeof(buffer), "%f", atom.d);
+            len = conv.dimSpace ?
+               Meter_humanUnit(buffer, atom.d, sizeof(buffer)) :
+               xSnprintf(buffer, sizeof(buffer), "%.2f", atom.d);
             break;
          default:
             break;
       }
-      if (len)
+      if (len) {
          RichString_appendnAscii(out, CRT_colors[metric->color], buffer, len);
+         if (metric->suffix)
+            RichString_appendAscii(out, CRT_colors[METER_TEXT], metric->suffix);
+      }
    }
    if (nodata)
       RichString_writeAscii(out, CRT_colors[METER_VALUE_ERROR], "no data");
diff --git a/pcp/meters/mysql b/pcp/meters/mysql
index 07ae11e9..a9e75e46 100644
--- a/pcp/meters/mysql
+++ b/pcp/meters/mysql
@@ -6,10 +6,8 @@
 caption = MySQL I/O
 recv.metric = mysql.status.bytes_received
 recv.color = green
-recv.label = recv
 sent.metric = mysql.status.bytes_sent
 sent.color = blue
-sent.label = sent
 
 [mysql_keys]
 caption = MySQL keys
diff --git a/pcp/meters/postfix b/pcp/meters/postfix
index 66876c5a..cda68377 100644
--- a/pcp/meters/postfix
+++ b/pcp/meters/postfix
@@ -18,4 +18,3 @@ bounce.color = red
 bounce.label = bnc
 hold.metric = sum(postfix.queues.hold)
 hold.color = yellow
-hold.label = hold
diff --git a/pcp/meters/redis b/pcp/meters/redis
index b9c084be..a72f7278 100644
--- a/pcp/meters/redis
+++ b/pcp/meters/redis
@@ -13,10 +13,8 @@ caption = Redis mem
 description = Redis memory
 lua.metric = redis.used_memory_lua
 lua.color = magenta
-lua.label = lua:
 used.metric = redis.used_memory
 used.color = blue
-used.label = used:
 
 [redisclient]
 caption = Redis clients
diff --git a/pcp/meters/tcp b/pcp/meters/tcp
index cebc658d..c95736f3 100644
--- a/pcp/meters/tcp
+++ b/pcp/meters/tcp
@@ -13,7 +13,6 @@ active.color = blue
 active.label = act
 syn.metric = network.tcpconn.syn_sent + network.tcpconn.syn_recv + network.tcpconn.last_ack
 syn.color = cyan
-syn.label = syn
 wait.metric = network.tcpconn.time_wait
 wait.color = red
 wait.label = tim