[memcached] Additional stats metrics

[truthbk]

Why

This PR adds support for memcached "items" statistics as reported by stats items. It also attempts to simplify the inclusion of additional optional metrics. To do so, it introduces a map of stats arguments (ie. items) to a list of rate metrics, gauge metrics, and if needed a raw stat output handler.

In the particular case of the items statistics, we need an output handler because we need to parse the raw key value (with form "items:<slab_id>:<metric_name>" to extract the <slab_id> and actual metric name. The actual value remains untouched - no need to operate over it. We can then use the slab_id to tag the relevant metrics. All handlers are expected to return a tuple of metric_name, tags, and actual value.

Enabling optional metrics requires configuring the optional_metrics flag in the memcached yaml.

Open Questions

• should each of the optional metrics get its own feature flag?
• currently namespacing optional metrics as follows: memcached.<stats_arg>.<metric_name> - yes? no?

Reference

https://github.com/memcached/memcached/blob/master/doc/protocol.txt

[degemer]

Memcached CI is failing on a coverage issue, it's probably legitimate. https://travis-ci.org/DataDog/dd-agent/jobs/129498510

[degemer]

A few confirmations/nitpicks:

• The exceptions are not re-raised for the optional metrics, so no service check is sent. Is it because the optional metrics are really optional and could fail while the others would still be there ?
• Maybe you could add a bit more documentation in the check conf files, specifying what are the collected optional metrics.

Looks good otherwise.

For the feature flag question, I'd say that if it was possible to regroup the optional metrics by type, it would be then possible to hide each type behind one feature flag. But I don't know really know memcached, so it may not be the best idea.
Idem for the second question, you have to know memcached to be able to answer.

[truthbk]

@degemer the reason I don't re-raise the exception for the optional metrics is precisely to avoid having the optional metrics trigger a failed service check if the default metrics succeed. It felt redundant. In principle, items stats wouldn't fail if the default metrics don't but I could foresee a few scenarios (like memcached not reporting all possible subsets of optional metrics due to some configuration directive, etc).

I think the right thing is probably to namespace the metrics, so I will make changes in that direction. We can then revisit and if we don't agree it's better, I'll revert.

[truthbk]

@degemer I'm actually having a bit of a hard-time deciding if I want to make some of those gauges into rates as well.... So I'll take a look at that as well.

[degemer]

Shouldn't this be if options and options.get(arg, False) ? (default: no optional metrics ?)

[truthbk]

@degemer Not really, I supposed that if you called the method without setting the options dict, then you wanted all options (otherwise you wouldn't call the _get_optional_metrics() method). On the other hand if you do set the options dict, then only collect what is set to True there.

I enforce the logic you're thinking about in check() where we only call _get_optional_metrics() if options are set in the YAML.

Let me know if you disagree.

[degemer]

It makes sense, and the default of options is {} anyway, so 👍

[degemer]

LGTM apart from the small remarks.

One question though: why didn't you enable coverage_report for the optional metrics tests ?

[truthbk]

@degemer because I forgot... lol

[degemer]

Feel free to merge once the CI is 💚