agent/metrics: allow preinitializing supported metrics to zero

[roobre]

Description

With the introduction of metrics in #213, we missed an important misbehavior: If a proxy never calls Inc on a metric, it is impossible to tell whether the metric is zero, or is not supported by the proxy. To fix this, this PR give proxies the ability to pre-initialize supported metrics to zero.

This PR also introduces a workaround: As the HTTP proxy is not directly testable, the handler is, the test for the handler needs to duplicate this initialization. This workaround should be removed when testing for the HTTP proxy is improved.

Checklist:

• I have performed a self-review of my code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works.
• I have run linter locally (make lint) and all checks pass.
• I have run tests locally (make test) and all tests pass.
• I have run relevant e2e test locally (make e2e-xxx for agent, disruptors, kubernetes or cluster related changes)
• Any dependent changes have been merged and published in downstream modules
  

[pablochacin]

@roobre seems we don't have an issue for tracking this. Mind opening one?

As the HTTP proxy is not directly testable, the handler is, the test for the handler needs to duplicate this initialization. This workaround should be removed when testing for the HTTP proxy is improved.

[pablochacin]

Minor thing: I find this message confusing because foo was explicitly set. Maybe "foo should exist" makes clear why it failed.

[pablochacin]

I do not agree with this comment. Moreover, I think this method should be public as part of the contract of the Proxy.

[roobre]

I like more declaring which metrics are supported by making them available in the map returned by Metrics, rather than exposing that list directly.

In my view, consumers of the Proxy interface should not be interested on the "list of metrics this proxy supports", which would be this method, but rather on "is this metric supported?". I believe this question is answered better by the Metrics method containing or not a particular metric.

As an example, the way a user would ask the proxy if it has a metric would be this:

requests, hasMetric := d.proxy.Metrics()[MetricRequests]
if hasMetric && requests == 0 {
    return fmt.Errorf("disruptor did not receive any request")
}

If the supported list of metrics was part of the contract, I think the code would look more verbose:

supportedMetrics := d.proxy.SupportedMetrics()
if slices.contains(supportedMetrics, MetricRequests) && d.proxy.Metrics()[MetricRequests] == 0 {
    return fmt.Errorf("disruptor did not receive any request")
}

I think the first way leverages the built-in existence check on maps to convey this meaning, while the latter reimplements is as a is-contained-in-separate slice which I like a bit less.

[pablochacin]

I don't see why having the list of metrics available should change the code for checking the metric, therefore I don't think the "verbosity" argument is valid.

In any case, my point was that having the proxy to know the supported list of metrics is not in my opinion a hack around the limitations of testing but something we expect it to know. The hack, in any case, is testing this at the handler level and not the proxy level. Maybe this should be stated more clearly in the comment.

Regarding exposing it or not in the API, it is secondary. But I think it is valid because it is not immediately evident that the proxy.Metrics() method will return all supported metrics regardless of whether they have a non-zero value or not. We should make this more explicit in the documentation:

// Metrics returns a map of counter-type metrics. Implementations may return zero or more of the metrics defined
// below, as well as any number of implementation-defined metrics.

[roobre]

These are good points, I've clarified both comments to state more clearly how supported metrics are exposed and the purpose of supportedMetrics. LMK if these look better to you 🙂

[pablochacin]

In general, I think the approach is a reasonable solution until the underlying problem is addressed. I Only made two minor non-blocking comments.

[roobre]

Issue regarding HTTP proxy testability open here: #314