Bump prometheus and thanos to master #3000

yeya24 · 2020-08-08T20:21:50Z

Signed-off-by: Ben Ye yb532204897@gmail.com

What this PR does:

This pr updates Prometheus and Thanos version to the current master, required by this pr

Update Prometheus to prometheus/prometheus@2899773.

Update Thanos to thanos-io/thanos@9b578af.

Checklist

Tests updated
Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

pstibrany · 2020-08-10T12:53:01Z

Cortex changes look good to me.

yeya24 · 2020-08-10T13:03:15Z

Cortex changes look good to me.

Thanks for the review! Do you think a changelog entry is needed?

pstibrany · 2020-08-10T13:06:45Z

Cortex changes look good to me.

Thanks for the review! Do you think a changelog entry is needed?

I don't think so in this case. There are some new Thanos metrics (thanos_memcached_client_info, thanos_memcached_operation_data_size_bytes), that will get exposed by Cortex, but I don't think we need to document them.

annanay25

Thanks for this PR!

annanay25 · 2020-08-10T13:55:57Z

pkg/ruler/compat.go

@@ -63,7 +63,7 @@ type PusherAppendable struct {
 }

 // Appender returns a storage.Appender
-func (t *PusherAppendable) Appender() storage.Appender {
+func (t *PusherAppendable) Appender(_ context.Context) storage.Appender {


I think we should propagate context from here to Pusher.Push(). It will also help to remove userID parameter from pusherAppender struct. We can encode the userID into context, in ruler.go here

cortex/pkg/ruler/ruler.go

Line 543 in a01c101

return r.managerFactory(ctx, userID, notifier, logger, reg), nil

.. and extract userID from context in this file as part of DefaultTenantManagerFactory

It will also help to remove userID parameter from pusherAppender struct. We can encode the userID into context, in ruler.go here

In other areas of the code, we prefer to pass userID explicitly, and not hide it into Context. But Cortex codebase is definitely not consistent about it :(

Oh, thanks @pstibrany! Would you prefer we separate it out? Its definitely a by-product in this PR, its more important we pass context along :)

Would you prefer we separate it out?

Yes, I think it would be bit cleaner approach. Thanks! (And sorry for not reacting earlier)

No worries @pstibrany :)

Yes, let's be explicit here and pass userID as parameter.

annanay25 · 2020-08-10T14:49:14Z

pkg/ruler/ruler.go

+	ctx = user.InjectUserID(ctx, userID)
+	manager := r.managerFactory(ctx, notifier, logger, reg)
+	if manager == nil {
+		return nil, errors.New("unable to create managerFactory")


nit: s/managerFactory/manager :)

pracucci

Thanks for working on this PR! I'm personally a bit worried about the big refactoring in TSDB, but we internally agreed to move on with this PR and eventually take precautions when deploying it to production.

Could you add a CHANGELOG entry like this, please?

- [CHANGE] Experimental blocks storage: the `operation` label value `getrange` has changed into `get_range` for the metrics `thanos_store_bucket_cache_operation_requests_total` and `thanos_store_bucket_cache_operation_hits_total`. #3000

FYI, other then Prometheus and Thanos, I also checked these direct dependencies upgrade:

github.com/aws/aws-sdk-go v1.33.5 to v1.33.12: checked the CHANGELOG, no change affecting Cortex

pracucci · 2020-08-11T08:28:20Z

Please be aware of this issue causing segfault. Not sure Prometheus we could be affected as well, but worth to keep an eye on it: prometheus/prometheus#7776

yeya24 · 2020-08-12T02:26:00Z

Sorry for the delay, I just updated the pr and update Prometheus version to current master. Please help review and let me know if I need to miss something.

Signed-off-by: Ben Ye <yb532204897@gmail.com>

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

pracucci · 2020-08-12T14:02:54Z

@yeya24 FYI I rebased master and fixed the CHANGELOG entry ordering in this commit.

pracucci

I investigated the issue prometheus/prometheus#7776 and I believe it's not a problem for us because, looking at the code, the issue should just affect the usage of db.ChunkQuerier() when querying the TSDB head, but we don't use it yet in any of our code paths (Cortex or Thanos code we do rely on).

yeya24 · 2020-08-12T14:36:31Z

@yeya24 FYI I rebased master and fixed the CHANGELOG entry ordering in this commit.

Awesome! Thanks for the help.

pstibrany

Overall change looks good to me.

I would still prefer to revert change that pushes injecting of userID into context to upper layer, and instead keep dependency on userID as explicit as possible.

pracucci · 2020-08-12T15:00:29Z

I run a local stress test for about 30 minutes. No issue found.

pracucci · 2020-08-12T15:01:25Z

I would still prefer to revert change that pushes injecting of userID into context to upper layer, and instead keep dependency on userID as explicit as possible.

I take care of that so we can proceed merging it and unblocking several people (@annanay25 included).

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

pull-request-size bot added the size/XXL label Aug 8, 2020

yeya24 force-pushed the bump-prometheus-thanos branch from dcbf356 to e79e9ac Compare August 8, 2020 20:22

yeya24 mentioned this pull request Aug 9, 2020

Add thanos query frontend sub command thanos-io/thanos#2973

Merged

5 tasks

annanay25 reviewed Aug 10, 2020

View reviewed changes

pracucci reviewed Aug 10, 2020

View reviewed changes

yeya24 force-pushed the bump-prometheus-thanos branch 2 times, most recently from c9b7bbb to 74f17f9 Compare August 12, 2020 02:25

yeya24 force-pushed the bump-prometheus-thanos branch 2 times, most recently from 4775778 to 24ccacd Compare August 12, 2020 03:03

annanay25 mentioned this pull request Aug 12, 2020

Refactor Ruler, introduce MultiTenantManager interface #3019

Merged

3 tasks

yeya24 and others added 3 commits August 12, 2020 16:01

Bump prometheus and thanos to master

0742bcf

Signed-off-by: Ben Ye <yb532204897@gmail.com>

add changelog entry

11747f2

Signed-off-by: Ben Ye <yb532204897@gmail.com>

Rebased and fixed CHANGELOG entry ordering

b328d5b

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

pracucci force-pushed the bump-prometheus-thanos branch from 24ccacd to b328d5b Compare August 12, 2020 14:02

pracucci approved these changes Aug 12, 2020

View reviewed changes

pstibrany approved these changes Aug 12, 2020

View reviewed changes

Reverted logic to pass userID to pusherAppender just for clarity

764bf00

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

pracucci merged commit a4aad5d into cortexproject:master Aug 12, 2020

yeya24 mentioned this pull request Aug 13, 2020

Update Cortex to master thanos-io/thanos#3027

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump prometheus and thanos to master #3000

Bump prometheus and thanos to master #3000

yeya24 commented Aug 8, 2020

pstibrany commented Aug 10, 2020

yeya24 commented Aug 10, 2020

pstibrany commented Aug 10, 2020

annanay25 left a comment

annanay25 Aug 10, 2020

pstibrany Aug 10, 2020

annanay25 Aug 10, 2020

pstibrany Aug 10, 2020

annanay25 Aug 10, 2020

pracucci Aug 11, 2020

annanay25 Aug 10, 2020

pracucci left a comment

pracucci commented Aug 11, 2020

yeya24 commented Aug 12, 2020 •

edited

Loading

pracucci commented Aug 12, 2020

pracucci left a comment

yeya24 commented Aug 12, 2020

pstibrany left a comment

pracucci commented Aug 12, 2020

pracucci commented Aug 12, 2020

Bump prometheus and thanos to master #3000

Bump prometheus and thanos to master #3000

Conversation

yeya24 commented Aug 8, 2020

pstibrany commented Aug 10, 2020

yeya24 commented Aug 10, 2020

pstibrany commented Aug 10, 2020

annanay25 left a comment

Choose a reason for hiding this comment

annanay25 Aug 10, 2020

Choose a reason for hiding this comment

pstibrany Aug 10, 2020

Choose a reason for hiding this comment

annanay25 Aug 10, 2020

Choose a reason for hiding this comment

pstibrany Aug 10, 2020

Choose a reason for hiding this comment

annanay25 Aug 10, 2020

Choose a reason for hiding this comment

pracucci Aug 11, 2020

Choose a reason for hiding this comment

annanay25 Aug 10, 2020

Choose a reason for hiding this comment

pracucci left a comment

Choose a reason for hiding this comment

pracucci commented Aug 11, 2020

yeya24 commented Aug 12, 2020 • edited Loading

pracucci commented Aug 12, 2020

pracucci left a comment

Choose a reason for hiding this comment

yeya24 commented Aug 12, 2020

pstibrany left a comment

Choose a reason for hiding this comment

pracucci commented Aug 12, 2020

pracucci commented Aug 12, 2020

yeya24 commented Aug 12, 2020 •

edited

Loading