How I understand that is that we give the power to the users to define themselves a set of metrics and labels. The users would do that in text of the env var.

First, I would rather predefine a set of metrics for the users based on business scenarios and allow them filter and aggregate freely in external tools.

Provided we must do it this way it is done in the code, we must have a validation for the user input. Wrongly defined metric can cause panics and collisions with existing metrics. Moreover, we won't have any control over the naming scheme and for each customer we would need to do additional checks in the env vars to support them with their issues regarding this functionality.

As the PR description does not explain why we want to give such freedom to the users, I'd like to ask you to add that text with some examples.

Your understanding is correct: we want to provide the user with a possibility to configure the exposed metrics by selecting the aggregation labels, and filter the resources.

I updated the description.

Calling Stop twice would cause panic. Maybe you could use concurrency.Singnal or context.Context instead.

This is unfortunately the common central pattern. We do not call Stop or Start twice.

I am working on a better approach for handling periodic tasks, but that's a separate topic. I didn't want to introduce anything new in this PR.

You propose the name to have a _total suffix while the type of prometheus metric is a gauge. The naming convention uses _total for accumulating types (counters). Here, we work with gauge, so I would rather expect sth similar to: "num_something" or "something_current".

The metrics report the total number of vulnerabilities, aggregated by some set of properties. So _total makes perfect sense.

Maybe, but then choosing the gauge as a type makes no sense, as it is not total by definition but current as gauges represent a snapshot of something, not a time-span.

When explaining this metric to a person you would say something like: "For this cluster (or any other filter), we have currently ### CVEs affecting all the images in the system"

I think that:

• keeping is as a gauge is fine when renamed,
• changing it so counter and keeping the _total suffix is also fine, but it would require a different interpretation of the metric.

A counter is a cumulative metric that represents a single monotonically increasing counter whose value can only increase or be reset to zero on restart.

Gauge is the right metric type to represent the current total number of vulnerabilities, IMHO.

I think what Piotr is saying (and I would agree) is that by Prometheus conventions, all counter metrics shall be suffixed by _total. Since the metric here is a gauge, we should choose a different suffix to comply with Prometheus naming conventions.

Ah, I see now! What about _vulnerabilities?
For example:

• Cluster_vulnerabilities
• Cluster_Namespace_vulnerabilities

@vikin91 , @stehessel , please also suggest the better way of making the metric names, as they may collide:
Both Deployment=*3 and Deployment=?3 expressions produce the same metric name
Deployment_eq__3_vulnerabilities.
I see two ways:

• keep the expressions as is, but escape the non-alphanumeric characters (requires prometheus >=v3.0.0);
• replace the known characters with some mnemonics, e.g.: * to _x_, ? to _y_ etc.

My suggestion would be to scrap the labels and filters from the metric name all together. So something like rox_central_vulnerability_count{cluster="...", namespace="...", severity="..."}.

My idea, that I tried to share in the meeting on Wednesday, is that we should not use env vars for the configuration, but instead have a configmap. This configmap would allow the users to specify the name of metric and configure it as they like (i.e., set the labels in the first phase). Example:

# not necessarily a valid yaml syntax
- vulnMetrics:
   -  name: "whatever1"
       labels: ["cluster", "namespace"]
   - name: "beautiful weather"
        labels: ["cluster", "namespace", "severity"]
        # later, we could have here filters: "namespace='abc*'", "severity>2"

Next, we would read that config map and create the following metrics with our prefix and suffix:

• rox_central_num_vlunerabilities_whatever1_current
• rox_central_num_vlunerabilities_beautiful_weather_current

That would allow us to:

• Deterministic metric names and clear expectation regarding labels
• Easily validate user input (and limit it if needed) - also a solution to the duplicates problem
• Have as many metrics as we want without waiting for new release
• Use our own prefix (for filtering those metrics from diag-bundle)
• Use correct suffix to stick to the naming convention

Moreover, going forward into the future, the grammar for the filtering would be much simpler as it would be divided into parts - one for each label.

I like your idea, and I will try to implement it. But not with a ConfigMap, but with a storage configuration object.

I am not fully sure what is being aggregated here, but it feels that prometheus provides the constructs to achieve this.

group (metric_name{namespace="abc"}) by (severity) or sum (metric_name{namespace="abc"}) by (severity)

I would appreciate an explanation why we cannot do it this way.

We do not want to expose everything for later aggregation in prometheus because of volumes. We want, though, to aggregate something according to the user configuration. So this is a mixed approach: prometheus will aggregate on top of what is aggregated here.

Explain how and why it matters.

Why not using the prometheus type for label?

Which one? I see only type Labels map[string]string.

Sorry, but this needs to be more explanatory. From the examples in the PR description, I see that you offer a positive number as the value of the metric, so it must be the rather "the number of discovered CVEs". But that is still imprecise (does it contain duplicates? how large is the time window?), so I'd like to ask you to improve this help message.

I added a more detailed description.

The new description example:

# HELP rox_central_Cluster_Namespace_Severity_total The total number of discovered CVEs aggregated by Cluster,Namespace,Severity and gathered every 1h0m0s
# TYPE rox_central_Cluster_Namespace_Severity_total gauge

Why add the keys in the metric name as well as the labels? Wouldn't a hard coded metric name with dynamic labels be sufficient? That would also make it easier to create Grafana dashboards.

Let's consider this configuration:
Cluster=*prod,IsFixable=true,Severity=CRITICAL_VULNERABILITY_SEVERITY|Cluster=*prod,IsFixable=true,Severity=HIGH_VULNERABILITY_SEVERITY|Severity.

A single hardcoded metric would not work, because the expectation is to get two different aggregations. But indeed, the expression arguments add complexity for further aggregation. The only disadvantage of not including the arguments is, probably, a potential confusion of implicit filtering, as the metric name won't tell about the applied filter. But the metric HELP will, so removing the arguments from the metric name should be fine. Thanks.

But why do we need to define multiple metrics here? In this example, the user could just define a single base metric

Cluster=*prod,IsFixable=true,Severity

and then use Prometheus to get the other derived metrics.

To filter out the LOW and MODERATE vulns for one metric and include them for the other metric.

Another example: a coarse metric for all clusters (like the default setting), and a fine metric for some specific cluster, which includes components. Cluster,Namespace,Severity|Cluster=reservation,Namespace,ComponentName=*apache*,Severity,CVSS.

But are these real world use cases or are they contrived? Note that if push comes to shove, one can also use relabelling rules in Prometheus to drop metrics. So I don't see a big issue offering just a single metric that is configurable via labels - seems flexible enough to avoid crashing Prometheus, and yet simple enough to explain to customers.

Another option to avoid names like rox_central_Cluster_Namespace_Severity_total would be to add an identifier label that is derived from the query config like rox_central_vulnerability_count{identifier="<ID>", ...}.

Metric name is not provided by the user.

I think we should point out here that the filtering logic is a performance optimization. I wonder if it is a pre-mature one - it seems to complicate the code significantly, and the metric volume could otherwise be controlled by the labels.

Not saying we should not implementing filtering - also considering it's already done now - but I would not have implemented it myself, and I don't think it provides much additional value over the label selection alone.

Move this to the tracker. Make the ticker part of the tracker as well.

Is there any reason why the aggregator cannot be the member s (serviceImpl)?

The only reason is the pattern of the code around.

Something does not fit here right. The aggregator is a part of the config service, so I would expect that the config service would be started here, not the aggregator. Why cannot we keep the hierarchy here?

The aggregator is not a part of the config service. The latter allows for reconfiguring the former in runtime.

Cannot we reuse the type that is built-in prometheus library?

Please suggest a type.

Why this is not a constructor, like New(...) *aggregator?

Because the package is called common.

This is type MetricsConfiguration map[MetricName]map[Label]Expression and it has its own type-alias, but the other does not have it. Why cannot we use the map[MetricName]map[Label]Expression as type here?

Because MetricsConfiguration has methods and is used in other places as such.

Why GetGatheringPeriodMinutes() cannot return time.Duration?

This is a storage field getter, the storage type is uint32.

There are many temp variables here just to collect the result. What you could do is

and drop the labelExpression and related length checks

This would assign to a nil map and panic. A metric with no labels should not be added to the result.

I think the name does not represent what this pkg is doing.

The package is an assembly of tools for trackers to use. Please suggest a better name, and/or how to split the package.