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

Sort labelset to ensure compatibility with thanos-receive #2784

Closed
wants to merge 5 commits into from

Conversation

fstolba
Copy link

@fstolba fstolba commented Aug 7, 2024

We're using thanos-receive which requires labels to be sorted lexicographically. Trying to submit a metric without first sorting the label list results in the write request being rejected, yielding the message Out of order labels in the label set.

This PR fixes this behaviour.

Please note I only tested this on thanos-remote, could use testing on other implementations.

@fstolba fstolba requested a review from a team August 7, 2024 08:56
Copy link

linux-foundation-easycla bot commented Aug 7, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: emdneto / name: Emídio Neto (5e062f7, 852d9dc)
  • ✅ login: fstolba / name: fst0 (186c4d7)
  • ✅ login: lzchen / name: Leighton Chen (e9423e6)
  • ✅ login: xrmx / name: Riccardo Magliocchetti (71189f6)

@emdneto emdneto marked this pull request as draft August 13, 2024 11:38
@xrmx
Copy link
Contributor

xrmx commented Aug 19, 2024

@fstolba You need to sign the CLA in order to have your changes integrated

@dsarazin
Copy link

Hello guys,

i also faced this issue and confirm that the commit proposed by @fstolba fixes this timeseries error !

image

@fstolba
Copy link
Author

fstolba commented Sep 15, 2024

Sorry for the delay, CLA is now signed.

@xrmx xrmx marked this pull request as ready for review September 16, 2024 07:20
Copy link
Member

@emdneto emdneto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can happen not only for Thanos but also for every Prometheus-like backend. Maybe it's worth having a comment and a simple test to prevent someone from removing that. prometheus/prometheus#11505

Could you please add changelog and fix lint?

@emdneto emdneto requested a review from a team as a code owner September 20, 2024 11:27
@xrmx
Copy link
Contributor

xrmx commented Sep 20, 2024

@fstolba Have you looked into adding a test (or an assert to an existing one) to check that the labels are sorted?

@giuseka
Copy link

giuseka commented Oct 22, 2024

I tested the solution proposed in #2784 with my thanos receiver, and it works for me. I print the labels and the order is correct.

@lzchen
Copy link
Contributor

lzchen commented Oct 24, 2024

@fstolba

Please add a CHANGELOG entry.

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

Successfully merging this pull request may close these issues.

6 participants