Merge upstream prometheus/prometheus at `37b408c` #639

gotjosh · 2024-05-30T16:25:23Z

Note to reviewers:

Please pay close attention to this diff, in particular to the following files:

Unmerged paths:
  (use "git add/rm <file>..." as appropriate to mark resolution)
        deleted by us:   .github/workflows/ci.yml
        both modified:   model/rulefmt/rulefmt.go
        both modified:   promql/engine_test.go
        both modified:   rules/alerting.go
        both modified:   rules/group.go
        both modified:   rules/manager.go
        both modified:   rules/manager_test.go
        both modified:   rules/recording.go
        both modified:   rules/rule.go
        both modified:   tsdb/index/index_test.go

The change that affected the tsdb/index/index_test.go and promql/engine_test.go is prometheus/prometheus#14071 -- I'll need an in-depth review of those two files and that PR as I modified to the best of my ability with what I understood.

Relevant

Feature: Allow configuration of a rule evaluation delay prometheus/prometheus#14061 (the upstreaming of "rule evaluation delay" which is now called "query offset")
optimize regex matching for empty label values in posting match prometheus/prometheus#14075 (a small tsdb optimisation, not worth of a changelog)
promql.ActiveQueryTracker: Unmap mmapped file when done prometheus/prometheus#14000 (query logger internals, not worth of a changelog)
Fix head stats and hooks when replaying a corrupted snapshot prometheus/prometheus#14079 (seems changelog worthy, TSDB experts, help me out?)
Make DBReadOnly more RO prometheus/prometheus#13218 (maybe, need to double check in Mimir)
Check context every 128 labels instead of 100 prometheus/prometheus#14118 (optimisation, not worth of a changelog)
tsdb/index: Refactor Reader tests prometheus/prometheus#14071 (test change, because we have non-upstreamed changes in this area)
Improve zeroOrOneCharacterStringMatcher by using utf8.DecodeRuneInString prometheus/prometheus#14090 (optimisation, not worth a changelog)
Enable perfsprint linter and fix up code issues prometheus/prometheus#14091 (optimisations (?), not worth a changelog)
tsdb: check for context cancel before regex matching postings prometheus/prometheus#14096 (optimisation, not worth a changelog)
Bugfix: quote label name in matchers when needed prometheus/prometheus#14068 (bugfix (?), will ask oleg, not sure if we have enabled support for utf-8 already in Mimir)
Relabeling: small optimization to hashmod prometheus/prometheus#13669 (optimisation, not worth a changelog)

Not Relevant

[Test] Extract most PromQL test code into separate packages prometheus/prometheus#13999 (test change)
otlp: Remove OTel feature gate registration from copied translation package prometheus/prometheus#13932 (OTel translator)
Clarify batch_send_deadline docs prometheus/prometheus#13758 (doc change)
tsdb/index/postings: fix missing lock unlock prometheus/prometheus#14103 (bugfix for code we never imported)
bugfix: allow opting-out of multi-cluster setups prometheus/prometheus#14062 (prometheus mixin)
[TEST] Rules: Sleep 15ms to fit Windows behaviour better prometheus/prometheus#14100 (test change)
docs: [ovh sd] Added missing label for OVH dedicated server in SD prometheus/prometheus#13970 (service discovery)
web/api: export defaultStatsRenderer prometheus/prometheus#14121 (struct made public, not relevant but might trigger a code change)
Converts existing native histogram unit tests to the PromQL testing framework prometheus/prometheus#13638 (test change)
added some lines prometheus.md and main.go prometheus/prometheus#14120 (doc change)
fix(api): Send warnings only if the limit is really exceeded prometheus/prometheus#14116 (API message when limit is hit, we don't use them in Mimir IIRC)
fix: correct the typo in azuread sdk auth prometheus/prometheus#14106 (doc string change)
doc: Clarify the limits of dumping/backfilling via OpenMetrics prometheus/prometheus#14129 (doc change)
Document running golangci-lint and make it work on arm64 prometheus/prometheus#12339 (doc change)
.gitpod.Dockerfile: Auto-fetch Go and goyacc versions prometheus/prometheus#14130 (dockerfile)
docs/configuration: clarify OpenStack metadata labels prometheus/prometheus#14149 (doc change)
Bump golangci-lint-action to 6.0.1 prometheus/prometheus#14154 (linter bump)
Upgrade to golangci-lint v1.59.0 prometheus/prometheus#14153 (linter bump)
Enable additional Go metrics prometheus/prometheus#14148 (new go metrics, not relevant for mimir as they are not imported but we might want to add them if we don't have them?)

Signed-off-by: Mickael Carl <mcarl@apple.com>

…d for transient file writes. use it in loadDataAsQueryable to make sure the RO Head doesn't truncate or cut new chunks in data/chunks_head/. add a -sandbox-dir-root flag to "promtool tsdb dump/dump-openmetrics" to control the root of that sandbox dirrectory. Signed-off-by: machine424 <ayoubmrini424@gmail.com>

…e discovery doc Signed-off-by: Jiekun <zhujiekun@52tt.com>

Allow users to opt-out of the multi-cluster setup for Prometheus dashboard, in environments where it isn't applicable. Refer: prometheus/prometheus#13180. Signed-off-by: Pranshu Srivastava <rexagod@gmail.com>

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

This adds some more test cases for unicode values, and also a benchmark for zeroOrOneCharacterStringMatcher.Matches() Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

This replaces the custom `moreThanOneRune` function with the standard `utf8.DecodeRuneInString(s)` that can be used to figure out the size of the first rune. Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

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

Signed-off-by: Neeraj Gartia <neerajgartia211002@gmail.com>

…to promql testing framework Signed-off-by: Neeraj Gartia <neerajgartia211002@gmail.com>

Signed-off-by: Neeraj Gartia <neerajgartia211002@gmail.com>

On Windows, Go will sleep 15ms if you ask for less. TestAsyncRuleEvaluation compares actual delay to the nominal time, so using 15ms should work better on Windows, and be hardly noticeable elsewhere. Signed-off-by: Bryan Boreham <bjboreham@gmail.com>

Signed-off-by: Oleksandr Redko <oleksandr.red+github@gmail.com>

Enable perfsprint linter and fix up code issues

bugfix: allow opting-out of multi-cluster setups

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

…gMatcher-Matches Improve `zeroOrOneCharacterStringMatcher` by using `utf8.DecodeRuneInString`

tsdb/index: Refactor Reader tests Co-authored-by: Björn Rabenstein <github@rabenste.in> Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com> --------- Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com> Co-authored-by: Björn Rabenstein <github@rabenste.in>

[TEST] Rules: Sleep 15ms to fit Windows behaviour better

docs: [ovh sd] Added missing label for OVH dedicated server in SD

…cordingly. Signed-off-by: kushagra Shukla <kushalshukla110@gmail.com> Signed-off-by: kushagra Shukla <kushalshukla110@gmail.com>

defaultStatsRenderer->DefaultStatsRenderer Add short docstrings. I'd like to use the stats renderer to peek at the statistics in grafana/mimir#7966 However I cannot call the original function without this export afterwards. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Converts existing native histogram unit tests to the PromQL testing framework

Follow up on prometheus/prometheus#14096 As promised, I bring a benchmark, which shows a very small improvement if context is checked every 128 iterations of label instead of every 100. It's much easier for a computer to check modulo 128 than modulo 100. This is a very small 0-2% improvement but I'd say this is one of the hottest paths of the app so this is still relevant. Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

Make DBReadOnly more RO

bboreham · 2024-06-03T12:49:29Z

tsdb: check for context cancel before regex matching postings prometheus/prometheus#14096 (optimisation, not worth a changelog)

This came out of a major incident, so it is quite important, but also it has already been merged here as #633.

…alse positives. Signed-off-by: gotjosh <josue.abreu@gmail.com>

gotjosh · 2024-06-03T13:10:55Z

model/rulefmt/rulefmt.go

+	AlignEvaluationTimeOnInterval bool       `yaml:"align_evaluation_time_on_interval,omitempty"`
+}
+
+func (g *RuleGroup) UnmarshalYAML(value *yaml.Node) error {


I'm looking for opinions here - is this enough to produce a "migration"? Users only write YAML directly to object storage before the ruler picks it up when encoding/decoding, it'll always show you the equivalent value of evaluation_delay as rule_offset.

Perhaps, not any sort of modification and just supporting both values (whilst having preference for rule_offset is an approach we want?)

I like the way you did it, but I'd fail if both fields are found.

as for being backwards compatible I think it's enough.

Maybe just always set them to the same value so that the code can use either (I haven't reviewed the rest of this PR); this also unblocks rollbacks from newer ruler to older ruler.

Maybe just always set them to the same value so that the code can use either

I agree on this. Looks safer, even if once we'll upgrade mimir-prometheus in mimir we'll remove the usage of EvaluationDelay (but better safe than sorry).

I've made the change to preserve both in a8dec7c

I like the way you did it, but I'd fail if both fields are found.

I think failing on both sounds sensible but seems more of a Mimir validation than a Prometheus one. We do have a function to validate rule groups for Mimir at an API level and I think this validation belongs there: https://github.com/grafana/mimir/blob/main/pkg/mimirtool/rules/rules.go#L255

Maybe just always set them to the same value so that the code can use either (I haven't reviewed the rest of this PR); this also unblocks rollbacks from newer ruler to older ruler.

I'm not sure you meant set them always to the same value if neither of them exist which is the alternative I had in mind but this is what I did:

If either evaluation_delay or query_offset is set, set the other to the same value.

If both of them have values (technically not possible with the validation in Mimir) output them as is

In mimir, we'll only use query_offset

Let me know what you think.

I think there would be two typical use cases:

Always load rules from code. In which case the user will set the delay and we correctly unmarshal into the offset now.

GET the rule from API, update and POST. The offset may or may not be returned on the POST, but both should work as expected I think

LGTM

If both of them have values (technically not possible with the validation in Mimir) output them as is

this is the bit I had in mind - if they conflict, then just choose one and set both to it. It's more an argument in principle, than solving for a specific problem right now. A group with different offset and delay is an invalid group. Not feeling strongly about it and I don't need to look at it again.

perhaps there can be some confusion if

the tenant gets a rule group which has both evalution_delay and query_offset

They set only evaluation_delay in the json/yaml we returned.

post back the same json without removing query_offset

if the client only pays attention to evaluation_delay, they won't ever understand why the delay is not being changed in Mimir. If we set them both to the same value, then the client would at least notice that the changes aren't reflected in Mimir.

Somewhat contrived, but I imagine that's how some scripting languages clients might work. Don't want to block this further, sorry

In Mimir, we have to make it work with older clients not supporting query_offset. For this reason, we can't stop exporting evaluation_delay, but we also want to export query_offset. I want a client to be able to successfully upload and exported config. For this reason, I'm not convinced the following is an option:

I like the way you did it, but I'd fail if both fields are found.
I think failing on both sounds sensible but seems more of a Mimir validation than a Prometheus one

I would keep it simple: export both fields, just fail the validation if they're both set but to different values.

aknuds1

I think there might be some mistakes in index_test.go, please see comments.

tsdb/index/index_test.go

- Don't change t to innerT - Bring back `t.Cleanup` with `reader.Close()` - Duplicate the createFileReader functions Signed-off-by: gotjosh <josue.abreu@gmail.com>

Signed-off-by: gotjosh <josue.abreu@gmail.com>

tsdb/index/index_test.go

Signed-off-by: gotjosh <josue.abreu@gmail.com>

aknuds1

Looks good from my side.

Maybe you could get input from @bboreham on prometheus/prometheus#14079?

bboreham · 2024-06-04T12:58:17Z

What is your question?

aknuds1 · 2024-06-04T13:01:13Z

@bboreham from the PR description:

prometheus/prometheus#14079 seems changelog worthy, TSDB experts, help me out?

bboreham · 2024-06-04T13:08:07Z

Yes, put it in the changelog.

dimitarvdimitrov · 2024-06-04T16:35:06Z

model/rulefmt/rulefmt.go

+		aux.QueryOffset = g.EvaluationDelay
+	}
+
+	aux.EvaluationDelay = nil


will this break clients that expect eval delay to be there? I'm thinking about grafana specifically

old versions of mimirtool might also expect evaluation_delay to be set

I agree with Dimitar here. We need to keep exporting evaluation_delay otherwise old clients break.

Good shout, I've made the change to preserve both in a8dec7c

config/config.go

pracucci · 2024-06-05T07:25:41Z

model/rulefmt/rulefmt.go

+	AlignEvaluationTimeOnInterval bool       `yaml:"align_evaluation_time_on_interval,omitempty"`
+}
+
+func (g *RuleGroup) UnmarshalYAML(value *yaml.Node) error {


Maybe just always set them to the same value so that the code can use either

I agree on this. Looks safer, even if once we'll upgrade mimir-prometheus in mimir we'll remove the usage of EvaluationDelay (but better safe than sorry).

pracucci · 2024-06-05T07:26:42Z

model/rulefmt/rulefmt.go

+		aux.QueryOffset = g.EvaluationDelay
+	}
+
+	aux.EvaluationDelay = nil


I agree with Dimitar here. We need to keep exporting evaluation_delay otherwise old clients break.

rules/manager_test.go

tsdb/head_read_test.go

tsdb/ooo_head_read_test.go

This reverts commit c9bb46a.

This reverts commit 8d8d930.

Signed-off-by: gotjosh <josue.abreu@gmail.com>

krajorama · 2024-06-06T10:58:21Z

promql/query_logger.go

+func (f *mmapedFile) Close() error {
+	err := f.m.Unmap()
+	if err != nil {
+		err = fmt.Errorf("mmapedFile: unmapping: %w", err)


Note : there was a fix to this later: prometheus/prometheus#13948

Actually fix is further down to an existing code path, but line numbers didn't line up

We should vendor this fix too.

Let's get this PR merged first

Signed-off-by: gotjosh <josue.abreu@gmail.com>

Signed-off-by: Marco Pracucci <marco@pracucci.com>

…nce when reading the setting Signed-off-by: Marco Pracucci <marco@pracucci.com>

Signed-off-by: Marco Pracucci <marco@pracucci.com>

gotjosh · 2024-06-06T17:28:36Z

Thank you all for the help 🙏

mickael-carl and others added 30 commits December 16, 2023 22:42

makefile: let golangci-lint also run on arm64

34072ff

Signed-off-by: Mickael Carl <mcarl@apple.com>

docs: add a quick note on linting in contributing guidelines

420b324

Signed-off-by: Mickael Carl <mcarl@apple.com>

docs: [ovh sd] Added missing label for OVH dedicated server in servic…

0cd3a22

…e discovery doc Signed-off-by: Jiekun <zhujiekun@52tt.com>

bugfix: allow opting-out of multi-cluster setups

8742768

Allow users to opt-out of the multi-cluster setup for Prometheus dashboard, in environments where it isn't applicable. Refer: prometheus/prometheus#13180. Signed-off-by: Pranshu Srivastava <rexagod@gmail.com>

promql.ActiveQueryTracker: Unmap mmapped file when done

aabe4d6

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

Benchmark zeroOrOneCharacterStringMatcher.Matches

fdfc6d4

This adds some more test cases for unicode values, and also a benchmark for zeroOrOneCharacterStringMatcher.Matches() Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

Use utf8.DecodeRuneInString(s)

bcff505

This replaces the custom `moreThanOneRune` function with the standard `utf8.DecodeRuneInString(s)` that can be used to figure out the size of the first rune. Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

Add invalid utf8 test cases to regexp

dbe88fa

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

Check utf8.RuneError result

8b4c945

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

adds TestNativeHistogramRate func to promql test framework

548bd9d

Signed-off-by: Neeraj Gartia <neerajgartia211002@gmail.com>

adds test for native histogram rate func in promql testing framework

8b838a0

Signed-off-by: Neeraj Gartia <neerajgartia211002@gmail.com>

adds test for sum, count, stddev, stdvar, quantile and fraction func …

adf5a36

…to promql testing framework Signed-off-by: Neeraj Gartia <neerajgartia211002@gmail.com>

some nits

6119124

Signed-off-by: Neeraj Gartia <neerajgartia211002@gmail.com>

removes the added tests from engine_test.go

661856c

Signed-off-by: Neeraj Gartia <neerajgartia211002@gmail.com>

Enable perfsprint linter and fix up code

f10c345

Signed-off-by: Oleksandr Redko <oleksandr.red+github@gmail.com>

Merge pull request #14091 from alexandear/enable-perfsprint-linter

806073a

Enable perfsprint linter and fix up code issues

Merge remote-tracking branch 'origin/main' into arve/query-logger-munmap

bf8d88f

Merge pull request #14062 from rexagod/multicluster-opt-out

0b1a0c0

bugfix: allow opting-out of multi-cluster setups

Fix flaky test

0f01d4b

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

Merge pull request #14090 from colega/improve-zeroOrOneCharacterStrin…

1e0b0e2

…gMatcher-Matches Improve `zeroOrOneCharacterStringMatcher` by using `utf8.DecodeRuneInString`

Merge pull request #14100 from bboreham/windows-flake

d1eff95

[TEST] Rules: Sleep 15ms to fit Windows behaviour better

Merge pull request #13970 from jiekun/doc/ovh-dedicated-server-label

edf5ebd

docs: [ovh sd] Added missing label for OVH dedicated server in SD

added line When set, query.max-concurrency may need to be adjusted ac…

0fea106

…cordingly. Signed-off-by: kushagra Shukla <kushalshukla110@gmail.com> Signed-off-by: kushagra Shukla <kushalshukla110@gmail.com>

Merge pull request #13638 from NeerajGartia21/promql-test

114dc5c

Converts existing native histogram unit tests to the PromQL testing framework

Merge pull request #13218 from machine424/ro-promtool

3119b8a

Make DBReadOnly more RO

Use the goleak version that excludes the dependecies that produce f…

7a6e325

…alse positives. Signed-off-by: gotjosh <josue.abreu@gmail.com>

gotjosh commented Jun 3, 2024

View reviewed changes

aknuds1 reviewed Jun 3, 2024

View reviewed changes

tsdb/index/index_test.go Show resolved Hide resolved

tsdb/index/index_test.go Show resolved Hide resolved

tsdb/index/index_test.go Outdated Show resolved Hide resolved

tsdb/index/index_test.go Show resolved Hide resolved

gotjosh added 2 commits June 3, 2024 19:35

Address review comments

86722a6

- Don't change t to innerT - Bring back `t.Cleanup` with `reader.Close()` - Duplicate the createFileReader functions Signed-off-by: gotjosh <josue.abreu@gmail.com>

Remove closure of reader on TestIndexRW_Postings

9433b57

Signed-off-by: gotjosh <josue.abreu@gmail.com>

aknuds1 reviewed Jun 4, 2024

View reviewed changes

tsdb/index/index_test.go Show resolved Hide resolved

gotjosh marked this pull request as ready for review June 4, 2024 08:28

Avoid duplication in createFileReaderWithOptions

06f1651

Signed-off-by: gotjosh <josue.abreu@gmail.com>

aknuds1 approved these changes Jun 4, 2024

View reviewed changes

dimitarvdimitrov reviewed Jun 4, 2024

View reviewed changes

pracucci reviewed Jun 5, 2024

View reviewed changes

gotjosh added 4 commits June 5, 2024 10:32

Revert "Fix more linters"

3836920

This reverts commit c9bb46a.

Revert "fix perfsprint linter"

2e6cc9b

This reverts commit 8d8d930.

update golangcilint to match Prometheus

9cce125

Signed-off-by: gotjosh <josue.abreu@gmail.com>

Fix lint

199a070

Signed-off-by: gotjosh <josue.abreu@gmail.com>

krajorama reviewed Jun 6, 2024

View reviewed changes

Preserve both evaluation_delay and query_offset

a8dec7c

Signed-off-by: gotjosh <josue.abreu@gmail.com>

krajorama approved these changes Jun 6, 2024

View reviewed changes

pracucci added 2 commits June 6, 2024 14:09

Fixed typo

969d5ee

Signed-off-by: Marco Pracucci <marco@pracucci.com>

Do not handle the migration at YAML serialization, but giving prefere…

c210ff7

…nce when reading the setting Signed-off-by: Marco Pracucci <marco@pracucci.com>

pracucci mentioned this pull request Jun 6, 2024

Update vendored prometheus grafana/mimir#8295

Merged

4 tasks

pracucci added 2 commits June 6, 2024 16:30

Fix linter issue

29c0c58

Signed-off-by: Marco Pracucci <marco@pracucci.com>

Trigger CI

03092b6

pracucci merged commit ef8f745 into main Jun 6, 2024
7 of 8 checks passed

pracucci deleted the gotjosh/update-mimir-prometheus-37b408c branch June 6, 2024 16:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge upstream prometheus/prometheus at `37b408c` #639

Merge upstream prometheus/prometheus at `37b408c` #639

gotjosh commented May 30, 2024 •

edited

Loading

bboreham commented Jun 3, 2024

gotjosh Jun 3, 2024

colega Jun 4, 2024

dimitarvdimitrov Jun 4, 2024

pracucci Jun 5, 2024

gotjosh Jun 6, 2024

krajorama Jun 6, 2024

dimitarvdimitrov Jun 6, 2024

dimitarvdimitrov Jun 6, 2024 •

edited

Loading

pracucci Jun 6, 2024

aknuds1 left a comment

aknuds1 left a comment

bboreham commented Jun 4, 2024

aknuds1 commented Jun 4, 2024

bboreham commented Jun 4, 2024

dimitarvdimitrov Jun 4, 2024

dimitarvdimitrov Jun 4, 2024

pracucci Jun 5, 2024

gotjosh Jun 6, 2024

pracucci Jun 5, 2024

pracucci Jun 5, 2024

krajorama Jun 6, 2024

krajorama Jun 6, 2024

pracucci Jun 6, 2024

krajorama Jun 6, 2024

gotjosh commented Jun 6, 2024

Merge upstream prometheus/prometheus at 37b408c #639

Merge upstream prometheus/prometheus at 37b408c #639

Conversation

gotjosh commented May 30, 2024 • edited Loading

Relevant

Not Relevant

bboreham commented Jun 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimitarvdimitrov Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aknuds1 left a comment

Choose a reason for hiding this comment

aknuds1 left a comment

Choose a reason for hiding this comment

bboreham commented Jun 4, 2024

aknuds1 commented Jun 4, 2024

bboreham commented Jun 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gotjosh commented Jun 6, 2024

Merge upstream prometheus/prometheus at `37b408c` #639

Merge upstream prometheus/prometheus at `37b408c` #639

gotjosh commented May 30, 2024 •

edited

Loading

dimitarvdimitrov Jun 6, 2024 •

edited

Loading