Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remote read adding external labels might lead to unsorted response #12605

Open
GiedriusS opened this issue Jul 26, 2023 · 2 comments
Open

Remote read adding external labels might lead to unsorted response #12605

GiedriusS opened this issue Jul 26, 2023 · 2 comments

Comments

@GiedriusS
Copy link
Contributor

GiedriusS commented Jul 26, 2023

What did you do?

/api/v1/read is supposed to always give a sorted series set as a response. Consider the following:

External labels are region: foo.

Series without any external labels:


aaa{instance="bbb", job="etcd-prometheus-exporter", service="etcd-prometheus-exporter", toposphere="test1"} 1
aaa{instance="bbb", job="etcd-prometheus-exporter", region="bar", service="etcd-prometheus-exporter", toposphere="test1"} 0
aaa{instance="bbb", job="etcd-prometheus-exporter", region="bar", service="etcd-prometheus-exporter", toposphere="test2"} 0
aaa{instance="bbb", job="etcd-prometheus-exporter", region="foo", service="etcd-prometheus-exporter", toposphere="test1"} 1
aaa{instance="bbb", job="etcd-prometheus-exporter", region="foo", service="etcd-prometheus-exporter", toposphere="test2"} 1

lbls = MergeLabels(labelsToLabelsProto(series.Labels(), lbls), sortedExternalLabels)
here is where Prometheus adds region: foo if it does not exist while iterating.

What did you expect to see?

I expected to see an error? that the response cannot contain the same label set. Not sure, though.

What did you see instead? Under which circumstances?

The result is the following:


aaa{instance="bbb", job="etcd-prometheus-exporter", region="foo", service="etcd-prometheus-exporter", toposphere="test1"} 1
aaa{instance="bbb", job="etcd-prometheus-exporter", region="bar", service="etcd-prometheus-exporter", toposphere="test1"} 0
aaa{instance="bbb", job="etcd-prometheus-exporter", region="bar", service="etcd-prometheus-exporter", toposphere="test2"} 0
aaa{instance="bbb", job="etcd-prometheus-exporter", region="foo", service="etcd-prometheus-exporter", toposphere="test1"} 1
aaa{instance="bbb", job="etcd-prometheus-exporter", region="foo", service="etcd-prometheus-exporter", toposphere="test2"} 1

System information

No response

Prometheus version

No response

Prometheus configuration file

No response

Alertmanager version

No response

Alertmanager configuration file

No response

Logs

No response

GiedriusS added a commit to GiedriusS/prometheus that referenced this issue Sep 10, 2023
Add test for bug prometheus#12605 for starters.

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
GiedriusS added a commit to GiedriusS/prometheus that referenced this issue Sep 10, 2023
Add test for bug prometheus#12605 for starters.

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
@bwplotka
Copy link
Member

Thanks!

I think the bug is valid - Prometheus should detect and deal with cases of external labels being duplicated in the scraped labels.

The answer "how to deal" is suggested by the honor label settings https://prometheus.io/docs/prometheus/latest/configuration/configuration/#scrape_config

Note that any globally configured "external_labels" are unaffected by this setting. In communication with external systems, they are always applied only when a time series does not have a given label yet and are ignored otherwise.

Unfortunately, unless we ignore those series or stop attaching external labels, we break streaming efficiency. Even worse - we only know results will be unsorted after we streamed something already.

Not sure yet what's the most explicit way out of this. Some ideas:

A) Add unsorted series field to message stream and stream those series there. Callers could do k-way merge or so.
B) ignore those series and send warning (surprising)
C) error the request and ask the caller to use the non streaming method
D) send external labels in separate fields instead of attaching them.

GiedriusS added a commit to GiedriusS/prometheus that referenced this issue Sep 11, 2023
Add test for bug prometheus#12605 for starters.

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
@GiedriusS
Copy link
Contributor Author

The problem is that external labels are not part of honor_labels. I wish if there was a way to automatically add external labels to the TSDB. This would immediately solve the problems with the remote read interface.

I have also checked that with federated Prometheus, Prometheus itself sorts the retrieved response thus hiding this problem. If in the future we want to have the same streaming approach in federated Prometheus, we will need to solve this somehow.

Sending external labels separately from the TSDB series still doesn't allow to use of this interface in a streaming way because adding those labels later on to the retrieved series might lead to the same problem i.e. the set becomes unsorted and thus we need to buffer+resort.

Just wanted to remind any other readers of this that we care about sortedness from /api/v1/read because in Thanos (and others?) essentially we want to know when we have received all copies of the same time series. Sorted output allows us to use k-way to merge everything together easily.

Some hybrid approach might be feasible here -> if an unsorted response is detected then we could send some hint or something. But overall that feels like a hack.

As far as I can tell, the only short-term solution is to buffer everything in /api/v1/read, even with the XOR format. And then later on we could think of a way to attach all external labels to the TSDB + mark that somehow so as to know whether that was enabled? Maybe something crazy like this could work:

global:
  add_external_labels_as_labels: true

?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants