Thanos Querier Rules API merges rules with the same name but different fields

[s-urbaniak]

Thanos, Prometheus and Golang version used:

Thanos: 0.14.0
Prometheus: 2.20.0
Golang: golang-1.14

Object Storage Provider:

N/A

What happened:

Given the following rules file structure in Prometheus:

$ jq '.data.groups[] | { name: .name, file: .file} | select(.name == "general.rules")' rules.json
{
  "name": "general.rules",
  "file": "/etc/prometheus/rules/prometheus-k8s-rulefiles-0/openshift-cluster-machine-approver-machineapprover-rules.yaml"
}
{
  "name": "general.rules",
  "file": "/etc/prometheus/rules/prometheus-k8s-rulefiles-0/openshift-monitoring-prometheus-k8s-rules.yaml"
}
{
  "name": "general.rules",
  "file": "/etc/prometheus/rules/prometheus-k8s-rulefiles-0/openshift-sdn-networking-rules.yaml"
}

Thanos querier rules /api/v1/rules deduplicates by name field only, yielding either:

$ jq '.data.groups[] | { name: .name, file: .file} | select(.name == "general.rules")' rules-thanos.json
{
  "name": "general.rules",
  "file": "/etc/prometheus/rules/prometheus-k8s-rulefiles-0/openshift-monitoring-prometheus-k8s-rules.yaml"
} 

or, non-deterministically, any other file name from the above Prometheus examples.

This is according to the specification https://github.com/thanos-io/thanos/blob/master/docs/proposals/202003_thanos_rules_federation.md#rules-api:

"Deduplication happens first at the rule group level. The identifier is the group name."

What you expected to happen:

That is about to be discussed. Either:

1. we add the file field as part of the deduplication logic.
2. we sort and explicitly add all file names, i.e. comma-separated in the resulting group.
3. additional ideas?

How to reproduce it (as minimally and precisely as possible):

Deploy the examples above with the same group names but different file fields.

Full logs to relevant components:

not relevant

Anything else we need to know:

once we reach consensus I am happy to augment

[s-urbaniak]

cc @bwplotka @metalmatze @squat @kakkoyun @lilic @simonpasquier @paulfantom @brancz

[s-urbaniak]

on personal note: I vote for option 1.: we add the file field as part of the deduplication logic.

[brancz]

file being part of the deduplication scheme seems like the correct approach to me

[simonpasquier]

option 1 looks ok to me. This is how Prometheus itself indexes rules:

https://github.com/prometheus/prometheus/blob/ce838ad6fcbd14b0cf9971a4d093ad672e1469fe/rules/manager.go#L1047-L1055

[bwplotka]

Let's be explicit and treat name + file as key (: