pmrep --ignore-unknown flag doesn't work, fails on unknown metrics

[tallpsmith]

The --ignore-unknown flag exists (option -5 / --ignore-unknown) but doesn't gracefully handle unknown metrics as documented.

Expected behavior:
When --ignore-unknown is set, unknown metrics should be tracked but not cause fatal errors (similar to pmstat behavior)

Actual behavior:
pmrep exits with error even when --ignore-unknown is set:

$ pmrep --ignore-unknown :vmstat
Invalid metric swap.used (PM_ERR_PMID Unknown or illegal metric identifier).

Root cause:
Three exception handlers in pmconfig.py (lines 644-648, 816-818, 1070-1075) don't check ignore_unknown_metrics() before calling sys.exit(1)

Solution:
Add per-metric failure tracking and check ignore_unknown_metrics() at all three exception points

[myllynen]

This is similar as the earlier #2055 but reveals also more details.

I added QA 1791 to test what is the current situation and for the enabled tests things would seem to be ok.

But in your example here the error is PM_ERR_PMID whereas Ken and me are seeing PM_ERR_NAME for unknown metrics. So we'd need more details on your config and setup to see what causing PM_ERR_PMID. Is this perhaps a Darwin vs Linux thing?

Another thing that caught my attention is that when doing something like

bad_derived = mem.util.free + no.such.metric

gives PM_ERR_BADDERIVE and this makes it probably impossible to differentiate this error from other derived metric formula errors which could also give PM_ERR_BADDERIVE. The related configuration options were not meant to ignore formula errors, only to make it easier using e.g. metricsets with archives which don't contain all the needed metrics for them.

[tallpsmith]

I discovered this working on #2453 . See commit tallpsmith@bdc4427

As macOS doesn't (yet) have the Swap metrics, I was just trying to play with the :vmstat config to try practicing adding some groupings. The Ignore didn't work until I fixed the bug (it's in that branch if you want to have a look).

I (Claude?) might have got the errors mixed up, current main branch on macOS shows this

% pmrep :vmstat
Invalid metric kernel.all.running (PM_ERR_NAME Unknown metric name).

 % pmrep --ignore-unknown :vmstat
Invalid metric mem.util.allcache (PM_ERR_NAME Unknown metric name).

FYI: Right now, the Ignore suppresses any missing columns, but I will be aiming to keep the column, but show ? to mimic how pmstat does it right now. This maintains the columns visually, while supporting when metrics are not there. I think that's a better way to handle a view, but that could just be my opinion. (@kmcdonell ?)

[tallpsmith]

@kmcdonell : Your requested examples:

on both main and my pmrep_group_headers branch:

% pmrep -s 1 sample.long.one foo.bar
Invalid metric sample.long.one (PM_ERR_NAME Unknown metric name).
% pmrep -5 -s 1 sample.long.one foo.bar
Not one known metric found.

I suspect I may have copy/pasted an incorrect error from a different issue.

[kmcdonell]

OK @tallpsmith that's really strange ... the second test case does not match what I'm seeing, namely

$ pmrep -5 -s 1 sample.long.one foo.bar
  s.l.one
         
        1

Can I ask you to please re-run this with a -D pmns before the -5, and post the output here?

[kmcdonell]

@tallpsmith I don't know about pmstat-style ? for missing metrics ... pmstat(1) is a real hack and was an early (and perhaps the first) PMAPI client that was a proof of concept to convince my boss at SGI that we should continue to invest in PCP rather than going down the infanticide path ... in a strange quirk of history, not only did PCP survive, but pmstat escaped and has been roaming in the wild ever since. So the ? mechanism was a quick hack rather than a well-thought out template to be followed.
Lots of people are now used to the pmrep behaviour, so I'd be loathed to change that as a default ... of course adding --some-other-option-because-we've-run-out-of-letters-in-the-alphabetic is always going to be an acceptable path to achieve your desired functional change without breaking default compatibility (at the undoubted cost of added code complexity).

I think I'll defer to @myllynen for the final word here.

[tallpsmith]

I agree that by default, pmrep should remain as is - maybe ideally barfing and outputting ALL unknown metrics (rather than just the first missing?)

If the --ignore-unknown is set, however, I think the default of pruning the missing metrics maybe worth discussing. Maybe I'm a visual guy, but the look and 'feel' of the view output should be maintained, even if some metrics cannot be displayed for some reason.

I'm open to yet-another-option added to continue to maintain existing behavior:

--ignore-unknown --display-unknown

or --keep-pauls-eyes-happy

[myllynen]

I think the difference you're seeing is caused by PMDA sample is installed in one case but not in the other.

The reason for --ignore-unknown not being helpful with partially available metrics for :vmstat relates to my above, perhaps a bit vague, comment about PM_ERR_BADDERIVE. That might be caused for many reasons and thus above libpcp it's not possible to determine when it's due to a unknown metric and when it's about something else.

In the new QA 1791 there are two steps commented out that are related to this. If you'd try --ignore-incompat then that should make it work. As speculated in the earlier issue about the case, this would seem at least require some man page tweak but even then the situation is probably not ideal.

[myllynen]

Thanks to Ken, I think the original issue here with incompatible and unknown metrics is solved now with some non-trivial libpcp changes and things should work consistently with both live and archive sources. QA 1791 was also updated accordingly so everything around that looks to be in order.

Wrt the idea of displaying all metrics even if not available/possible I do like the idea, especially with metricsets mimicking well-established tools like vmstat a missing column is not following the principle of least surprise. I'm not sure who or when would have time to work on this but I think it'd be worth us to file a separate RFE for this.

Ken pointed out elsewhere that there might be a far-from-ideal way to avoid the issue in some cases. One could check with defined() if the value can be displayed and if not then print something suitable to keep the column included. It's certainly not a generic solution and littering all the pmrep config files with those checks doesn't sound tempting either, I'm just mentioning that for completeness sake.

Given the above, I suggest that we'll file the RFE and then mention it here for reference and close this one. I can do that myself at some point next week if not already done by then. Thanks.

[myllynen]

I've now created the below RFE for the feature proposal, closing this one as the original issue here is solved. Thanks.

#2508