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

*: Upgrade Prometheus to v2.39.0 #5767

Merged
merged 13 commits into from Oct 7, 2022

Conversation

douglascamata
Copy link
Contributor

@douglascamata douglascamata commented Oct 6, 2022

Signed-off-by: Douglas Camata 159076+douglascamata@users.noreply.github.com

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

  • Upgrade Prometheus to v2.39.0
  • Upgrade promql-engine

Almost the entirety of this work was started by @GiedriusS at #5602. This PR was opened with his consent, as he's on vacations.

Verification

Repro for thanos-io#5600.

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
Update prometheus dependency.

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
@douglascamata douglascamata changed the title Upgrade Prometheus to v2.39.0 *: Upgrade Prometheus to v2.39.0 Oct 6, 2022
Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>
Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>
@douglascamata douglascamata marked this pull request as ready for review October 6, 2022 18:44
Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>
Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>
yeya24
yeya24 previously approved these changes Oct 6, 2022
Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

LGTM on green.
A little bit concern on the new TSDB change on the compactor side, but as long as the tests are passing then we can give it a try

Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>
@douglascamata
Copy link
Contributor Author

@yeya24 sure. I think the commit I just pushed should fix the e2e tests failure. Let's wait and see. What is the change in TSDB that worries you on the compactor side? Maybe we can try to write an e2e test for it (in this PR or a follow up).

Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>
@yeya24
Copy link
Contributor

yeya24 commented Oct 6, 2022

@yeya24 sure. I think the commit I just pushed should fix the e2e tests failure. Let's wait and see. What is the change in TSDB that worries you on the compactor side? Maybe we can try to write an e2e test for it (in this PR or a follow up).

The OOO feature introduces a new chunk encoding. For blocks generated by Thanos I think that's fine since we don't enable this feature. But for blocks generated by Prometheus I am not sure if it will contain OOO chunks.

I think I need to read the OOO source code to see how it works. If OOO chunks only exist in memory (head block) then seems we should be safe.

@douglascamata
Copy link
Contributor Author

douglascamata commented Oct 6, 2022

Some TestCompactor_WriteSeries_e2e tests are failing. I will investigate tomorrow, but I believe I might already stumble on something related to what you mentioned, @yeya24.

Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>
@fpetkovski
Copy link
Contributor

Hm, shouldn't OOO chunks be explicitly enabled? Or does Prometheus always create them?

@douglascamata
Copy link
Contributor Author

Only one unit test left to fix 💪

Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>
@douglascamata
Copy link
Contributor Author

Hm, shouldn't OOO chunks be explicitly enabled? Or does Prometheus always create them?

After all the hacking I did today, I learned that OOO has to be explicitly enabled otherwise the OOO data is refused. I could be wrong though, please peer review my statement. 🤔 😂

@douglascamata
Copy link
Contributor Author

@yeya24 @fpetkovski we are ✅ on all checks. PTAL whenever you have some time. 🙇

Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@@ -82,7 +82,6 @@ func registerReceive(app *extkingpin.App) {
RetentionDuration: int64(time.Duration(*conf.retention) / time.Millisecond),
NoLockfile: conf.noLockFile,
WALCompression: conf.walCompression,
AllowOverlappingBlocks: conf.tsdbAllowOverlappingBlocks,
Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering if we make overlapping blocks a default at the TSDB level, on the compactor side should we also do the same? Right now it is not enabled by default, we can follow up it on a separate pr though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it makes sense to be consistent and also enable it by default in the compactor.

Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Amazing, thanks!

@@ -37,6 +37,7 @@ We use *breaking :warning:* to mark changes that are not backward compatible (re
- [#5641](https://github.com/thanos-io/thanos/pull/5641) Query: Inject unshardable le label in query analyzer.
- [#5685](https://github.com/thanos-io/thanos/pull/5685) Receive: Make active/head series limiting configuration per tenant by adding it to new limiting config.
- [#5411](https://github.com/thanos-io/thanos/pull/5411) Tracing: Change Jaeger exporter from OpenTracing to OpenTelemetry. *Options `RPC Metrics`, `Gen128Bit` and `Disabled` are now deprecated and won't have any effect when set :warning:.*
- [#5767](https://github.com/thanos-io/thanos/pull/5767) *: Upgrade Prometheus to v2.39.0.
Copy link
Member

Choose a reason for hiding this comment

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

Anything notable to mention? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Besides the perf improvements notables, I think the overlapping blocks being on by default is also notable.

@bwplotka
Copy link
Member

bwplotka commented Oct 7, 2022

After all the hacking I did today, I learned that OOO has to be explicitly enabled otherwise the OOO data is refused.

That's correct.

@bwplotka bwplotka merged commit 561a113 into thanos-io:main Oct 7, 2022
utukJ pushed a commit to utukJ/thanos that referenced this pull request Oct 13, 2022
* e2e: add tests for pushed down subqueries

Repro for thanos-io#5600.

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

* *: update dependency

Update prometheus dependency.

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

* test: fix test

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

* Upgrade Prometheus to v2.39.0

Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>

* Fix UnRegisterer to work well when it's wrapped

Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>

* Update CHANGELOG

Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>

* Fix UnRegisterer creation

Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>

* Fix e2e env name to comply with new rules

Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>

* Remove query pushdown test case

Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>

* Fix compactor relabeling after Prometheus upgrade

Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>

* Fix tsdb.NewHead call after wbl addition

Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>
Co-authored-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
Signed-off-by: utukj <utukphd@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants