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

Add optional ring support to overrides-exporter #3908

Merged
merged 34 commits into from
Jan 12, 2023
Merged

Conversation

flxbk
Copy link
Contributor

@flxbk flxbk commented Jan 10, 2023

What this PR does

In read-write deployment mode, every backend replica includes an overrides-exporter that exports per-tenant limit metrics, leading to duplicate metrics being exposed if the number of replicas is greater than one. This PR adds experimental support for a ring to overrides-exporter, which is used to uniquely shard tenant metrics to a given instance and avoid the export of duplicate metrics.

While the solution proposed in this PR works for periods of stable service operations, it is still prone to duplicate exports during rollouts because of the effects of tenant resharding. This problem will be addressed in a follow-up PR.

Which issue(s) this PR fixes or relates to

Fixes #3806

Checklist

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

@flxbk flxbk self-assigned this Jan 10, 2023
@flxbk flxbk force-pushed the felix/exporter-ring branch 2 times, most recently from b6ec9de to 65ee964 Compare January 10, 2023 17:22
@pracucci pracucci self-requested a review January 11, 2023 09:44
Copy link
Collaborator

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Good job!

pkg/util/validation/exporter/ring.go Outdated Show resolved Hide resolved
pkg/util/validation/exporter/ring.go Outdated Show resolved Hide resolved
pkg/util/validation/exporter/ring.go Outdated Show resolved Hide resolved
pkg/util/validation/exporter/ring.go Outdated Show resolved Hide resolved
pkg/util/validation/exporter/ring.go Outdated Show resolved Hide resolved
pkg/mimir/modules.go Outdated Show resolved Hide resolved
pkg/util/validation/exporter/exporter.go Outdated Show resolved Hide resolved
pkg/util/validation/exporter/exporter.go Outdated Show resolved Hide resolved
pkg/util/validation/exporter/exporter.go Outdated Show resolved Hide resolved
pkg/util/validation/exporter/exporter.go Show resolved Hide resolved
@flxbk flxbk force-pushed the felix/exporter-ring branch 2 times, most recently from 9383723 to c03b2b6 Compare January 11, 2023 17:53
@flxbk flxbk marked this pull request as ready for review January 12, 2023 07:19
@flxbk flxbk requested review from a team as code owners January 12, 2023 07:19
@flxbk flxbk changed the title [WIP] Add optional ring support to overrides-exporter Add optional ring support to overrides-exporter Jan 12, 2023
@pracucci pracucci self-requested a review January 12, 2023 09:03
Copy link
Collaborator

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Good job! Few last comments. I just realised we may need the wait ring stability. Let's talk on Slack.

CHANGELOG.md Outdated Show resolved Hide resolved
pkg/util/validation/exporter/exporter.go Outdated Show resolved Hide resolved
pkg/util/validation/exporter/exporter_test.go Outdated Show resolved Hide resolved
pkg/util/validation/exporter/exporter_test.go Outdated Show resolved Hide resolved
pkg/util/validation/exporter/exporter_test.go Outdated Show resolved Hide resolved
pkg/util/validation/exporter/exporter_test.go Show resolved Hide resolved
integration/overrides_exporter_test.go Outdated Show resolved Hide resolved
integration/overrides_exporter_test.go Outdated Show resolved Hide resolved
integration/overrides_exporter_test.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Thanks to address my feedback, LGTM! We discussed offline about improvements to reduce the chances tenants will be resharded among replicas during a rollout, and we'll address them in a follow up PR.

@pracucci pracucci enabled auto-merge (squash) January 12, 2023 11:03
@pracucci pracucci merged commit 62279ef into main Jan 12, 2023
@pracucci pracucci deleted the felix/exporter-ring branch January 12, 2023 11:15
zenador pushed a commit that referenced this pull request Jan 13, 2023
* Use assert statements instead of handmade checks (#3936)

jsonnet already has an 'assert <assertion>' statement, there's no need to craft
handmade 'if <assertion> then null else error'.

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

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

* Fixes templating parsing for manually created vars (#3921)

When you create a dashboard variable from within the Grafana UI, it
creates the query field as an object. The object contains a refId as
well as the original query.

The existing code expects the query to be a string, not an object
and will therefor miss any metrics used if the variable was created
in the UI.

* Add optional ring support to overrides-exporter (#3908)

* Add optional ring support to overrides-exporter

* add docs

* add config for read-write mode

* fix import

* fix naming, swallow error on ring failure

* make format

* make doc

* add license

* inline ownership check

* add memberlist dependency for overrides exporter

* basic integration test

* [wip] troubleshoot ring membership

* start ring client

* fix instance address

* Move code

* wait on active instance state at startup

* fix copy-pasta

* improve integration tests

* refactor

* formatting

* inline

* move code around

* Apply suggestions from code review

Co-authored-by: Marco Pracucci <marco@pracucci.com>

* incorporate review feedback

* basic unit test

* mock ring in unit test

* format

* clean up

* cosmetics/changelog

* cosmetic

* cosmetic

* changelog/log

* improve waiting in tests

* use GetAddresses()

Co-authored-by: Marco Pracucci <marco@pracucci.com>

* Update test

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
Co-authored-by: Oleg Zaytsev <mail@olegzaytsev.com>
Co-authored-by: Jack <57678801+mothershipper@users.noreply.github.com>
Co-authored-by: Felix Beuke <felix.j.beuke@gmail.com>
Co-authored-by: Marco Pracucci <marco@pracucci.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: add (optional) ring support to overrides-exporter to only export metrics from 1 replica
3 participants