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

alerts: Fix MimirIngesterHasNotShippedBlocks for other deployment modes #3627

Conversation

jhesketh
Copy link
Contributor

@jhesketh jhesketh commented Dec 2, 2022

What this PR does

This fixes the job regex for monolithic and read-write deployment modes for MimirIngesterHasNotShippedBlocks and MimirIngesterHasNotShippedBlocksSinceStart

Which issue(s) this PR fixes or relates to

Partial fixes #3366

Checklist

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

@jhesketh jhesketh requested a review from a team as a code owner December 2, 2022 06:05
CHANGELOG.md Outdated Show resolved Hide resolved
@jhesketh jhesketh force-pushed the read-write-deployment/alerts/MimirIngesterHasNotShippedBlocks branch 2 times, most recently from 70adca1 to 845db53 Compare December 5, 2022 11:32
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 @jhesketh for addressing my feedback. I left few comments, but overall the approach LGTM.

pkg/ingester/shipper.go Outdated Show resolved Hide resolved
pkg/ingester/shipper.go Outdated Show resolved Hide resolved
pkg/ingester/shipper.go Outdated Show resolved Hide resolved
pkg/ingester/shipper.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
operations/mimir-mixin/alerts/blocks.libsonnet Outdated Show resolved Hide resolved
pkg/ingester/shipper.go Outdated Show resolved Hide resolved
Copy link
Member

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

Thanks, this looks like better approach. My comments are not blocking.

pkg/ingester/shipper.go Outdated Show resolved Hide resolved
pkg/ingester/shipper.go Outdated Show resolved Hide resolved
jhesketh and others added 9 commits December 7, 2022 16:16
This fixes the job regex for monolithic and read-write deployment modes
for MimirIngesterHasNotShippedBlocks and MimirIngesterHasNotShippedBlocksSinceStart
Use the new mimir_ingester_shipper_bucket_last_successful_upload_time
metric which only counts for ingester shipper rather than any uploaded
blocks.
Co-authored-by: Marco Pracucci <marco@pracucci.com>
@jhesketh jhesketh force-pushed the read-write-deployment/alerts/MimirIngesterHasNotShippedBlocks branch from ed98e87 to aa350a6 Compare December 7, 2022 05:43
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.

Logic changes LGTM!

Some suggestions to unit test it:

  • You could add the assertions to the test TestShipper().
  • Since we can't assert on the exact timestamp (it would be unreliable) we can't assert on the actual /metrics output. However, you can get the metric value using testutil.ToFloat64(s.metrics. lastSuccessfulUploadTime) and assert on a value with a delta toleration (e.g. now +/- 2 seconds). You can assert value is 0 before the 1st upload, then updated after Sync() is called for the first time.

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.

LGTM (modulo a couple of nit I'm going to self-merge if you don't mind). Thanks!

pkg/ingester/shipper_test.go Outdated Show resolved Hide resolved
pkg/ingester/shipper_test.go Outdated Show resolved Hide resolved
@pracucci pracucci enabled auto-merge (squash) December 13, 2022 08:26
@pracucci pracucci merged commit c4df655 into grafana:main Dec 13, 2022
masonmei pushed a commit to udmire/mimir that referenced this pull request Dec 16, 2022
…es (grafana#3627)

* alerts: Fix MimirIngesterHasNotShippedBlocks for other deployment modes

This fixes the job regex for monolithic and read-write deployment modes
for MimirIngesterHasNotShippedBlocks and MimirIngesterHasNotShippedBlocksSinceStart

* Update CHANGELOG

* Add new metric for tracking last shipped block time

* Update IngesterHasNotShippedBlocks to use new metric

Use the new mimir_ingester_shipper_bucket_last_successful_upload_time
metric which only counts for ingester shipper rather than any uploaded
blocks.

* Apply suggestions from code review

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

* Update CHANGELOG

* Update alert to use new metric

* Fix gauge type

* Update helm test

* Group code a bit cleaner

* Add test for new metric

* Apply suggestions from code review

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ensure all alerts correctly work when Mimir is deployed using the read-write deployment mode
3 participants