add a priority label #288
add a priority label #288
Conversation
Signed-off-by: Roel Arents <roel.arents@kadaster.nl>
|
This Pull Request is stale because it has been open for 60 days with |
There was a problem hiding this comment.
Hi, Thanks for raising this PR and apologies for the long/late response.
I'd like to understand a little more about the use case you're depicting here, is the priority usage to be for Priority in version checking.
Or is it so that you can perform prometheus queries sorted by said label and prioritize there?
If the former, I'm not sure (as commented) if it's the right thing in exposing said priority label as this could increase cardinality and have adverse effects on upstream storage engines.
If the latter, I'd be inclined to run multiple version-checkers OR simply use metric_relabel_configs for those images that need a priority label defined.
| @@ -140,6 +140,7 @@ func (m *Metrics) buildLabels(namespace, pod, container, containerType, imageURL | |||
| "image": imageURL, | |||
| "current_version": currentVersion, | |||
| "latest_version": latestVersion, | |||
| "priority": priority, | |||
There was a problem hiding this comment.
Do we need priority to be output as a metric label? What is the intended use case of this? I'm concerned that this could increase cardinality unnecessarily? I'd favour logging over exposing as a metrics label.
| func TestCache(t *testing.T) { | ||
| m := New(logrus.NewEntry(logrus.New())) | ||
|
|
||
| for i, typ := range []string{"init", "container"} { | ||
| version := fmt.Sprintf("0.1.%d", i) | ||
| m.AddImage("namespace", "pod", "container", typ, "url", true, version, version) | ||
| m.AddImage("namespace", "pod", "container", typ, "url", true, version, version, defaultPriority) | ||
| } | ||
|
|
There was a problem hiding this comment.
If we do need this, (see comment in metrics.go) Can we have a test that covers the setting and reflection of a different priority and validate that the metric set reflects the value given.
| @@ -97,6 +98,7 @@ func (c *Controller) checkContainer(ctx context.Context, log *logrus.Entry, pod | |||
| container.Name, containerType, | |||
| result.ImageURL, result.IsLatest, | |||
| result.CurrentVersion, result.LatestVersion, | |||
| strconv.Itoa(result.Priority), | |||
There was a problem hiding this comment.
as with metrics.go - I'm not entirely sure this would be needed.
|
Thx for checking! Our use case is the latter. We have ~100 images and with prometheus/grafana we want the most important ones on top. We use priorities from zero (default) to 10. Regarding the extra label: a label is added, but with a stable value. That should not increase cardinality, I think. Same amount of time series. |
|
Thanks for the response, and sorry for the delay in getting back to you. I'm not 100% sure on this one - I can see the usecase of wanting things like metrics_relabel_configs:
- source_labels: [image]
regex: "grafana\:(.*)"
target_label: priority
replacement: "1000"
action: replaceSimilar could also be done based off namespace and any other pods/containers within the existing set of metrics exposed by version-checker without any code changes. |
|
I agree metric_relabel_configs could be used here. I don't know if it would be more manageable and meaningful. The config would be moved to the general prometheus config instead of the version-checker config. |
In our use case we have many images and we'd like to be able to prioritize better. In order to sort by arbitrary priority, this PR adds a
prioritylabel (set as an annotation).