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

Update to upstream 64a9abb8be (LabelValuesFor signature change) #651

Merged
merged 13 commits into from
Jun 17, 2024

Conversation

colega
Copy link
Contributor

@colega colega commented Jun 17, 2024

This brings the changes from upstream up to 64a9abb8be which includes changes to LabelValuesFor method: prometheus/prometheus#14280

This intentionally doesn't update to latest main, which will have more changes because of the compactor interface refactor: prometheus/prometheus#14143

bboreham and others added 13 commits June 4, 2024 13:54
Give more clues when troubleshooting.

Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
It's quite common during the compaction cycle to hold series IDs for
series that aren't in the TSDB head anymore.

We shouldn't fail if that happens, as the caller has no way to figure
out which one of the IDs doesn't exist.

Fixes prometheus/prometheus#14278

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
Signed-off-by: parnavh <parnav100sach@gmail.com>
* docs: clarify default Docker command line parameters

Signed-off-by: Rens Groothuijsen <l.groothuijsen@alumni.maastrichtuniversity.nl>

* docs: move Docker command line parameters section and refer to Dockerfile

Signed-off-by: Rens Groothuijsen <l.groothuijsen@alumni.maastrichtuniversity.nl>

* Add link to Dockerfile in documentation

Co-authored-by: Ayoub Mrini <ayoubmrini424@gmail.com>
Signed-off-by: Rens Groothuijsen <l.groothuijsen@alumni.maastrichtuniversity.nl>

---------

Signed-off-by: Rens Groothuijsen <l.groothuijsen@alumni.maastrichtuniversity.nl>
Co-authored-by: Ayoub Mrini <ayoubmrini424@gmail.com>
* chore: use HumanizeDuration from prometheus/common

Signed-off-by: Sergey <freak12techno@gmail.com>

* chore: fixed linting

Signed-off-by: Sergey <freak12techno@gmail.com>

* chore: review fixes

---------

Signed-off-by: Sergey <freak12techno@gmail.com>
headIndexReader.LabelNamesFor: skip not found series
[ENHANCEMENT] HTTP API: Add url to errors logged while sending response
…insensitive comparison (#14170)

* Converted string to standarized form
* Added golang.org/x/text in Go dependencies
* Added test cases for FastRegexMatcher
* Added benchmark for toNormalizedLower

Signed-off-by: RA <ranveeravhad777@gmail.com>
Adjust the default GOGC value to 75. This is less of a memory savings,
but has less impact on CPU use.

Signed-off-by: SuperQ <superq@gmail.com>
The only call we have to LabelValuesFor() has an index.Postings, and we
expand it to pass to this method, which will iterate over the values.

That's a waste of resources: we can iterate on the index.Postings
directly.

If there's any downstream implementation that has a slice of series,
they can always do an index.ListPostings from them: doing that is
cheaper than expanding an abstract index.Postings.

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
@CLAassistant
Copy link

CLAassistant commented Jun 17, 2024

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
3 out of 8 committers have signed the CLA.

✅ colega
✅ bboreham
✅ roidelapluie
❌ parnavh
❌ rgroothuijsen
❌ freak12techno
❌ Ranveer777
❌ SuperQ
You have signed the CLA already but the status is still pending? Let us recheck it.

@colega
Copy link
Contributor Author

colega commented Jun 17, 2024

This introduces a performance regression in LabelNamesFor fixed later in prometheus/prometheus@5efc8dd

So this shouldn't go into the Mimir's weekly release without that.

}

_, ok := m.values[s]
return ok
}

// toNormalisedLower normalise the input string using "Unicode Normalization Form D" and then convert
// it to lower case.
func toNormalisedLower(s string) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be mentioned in the PR description as a fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are we doing that?

The PR will be a merge commit, and it has a list of all the commits we brought from upstream. I'm not sure what's the utility of copying that to the description.

Copy link
Contributor

@aknuds1 aknuds1 Jun 17, 2024

Choose a reason for hiding this comment

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

Normally we tend to mention changes relevant to Mimir in these sync PRs. Makes it easier for reviewers, and to know what should make it into the Mimir changelog when the time comes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Normally we tend to mention changes relevant to Mimir in these sync PRs.

I think that is a mistake, because it's hard to tell here which changes are relevant to Mimir and which are not.

We should mention the changes that are relevant to Mimir when we're vendoring the used modules in Mimir.

Makes it easier for reviewers, and to know what should make it into the Mimir changelog when the time comes.

Not sure why it's easier, it's double work, we still have to review the changes when vendoring to Mimir.

@colega colega force-pushed the prometheus-main-64a9abb8be branch from 6c55dbd to 6af02ae Compare June 17, 2024 08:26
@colega colega merged commit 26c8bac into main Jun 17, 2024
15 of 16 checks passed
@colega colega deleted the prometheus-main-64a9abb8be branch June 17, 2024 09:06
colega added a commit to grafana/mimir that referenced this pull request Jun 17, 2024
Updates to grafana/mimir-prometheus#651

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
colega added a commit to grafana/mimir that referenced this pull request Jun 17, 2024
* Update mimir-prometheus to 26c8bac

Updates to grafana/mimir-prometheus#651

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

* Update CHANGELOG.md

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

---------

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
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.