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

Convert MaxTime to seconds when recording compactor lag #3429

Merged
merged 3 commits into from Nov 10, 2022

Conversation

56quarters
Copy link
Contributor

@56quarters 56quarters commented Nov 10, 2022

What this PR does

Fixes an issue where a timestamp in milliseconds was subtracted from a timestamp in seconds, resulting in negative deltas being recorded instead of the actual delay between the block maxtime and "now".

Signed-off-by: Nick Pillitteri nick.pillitteri@grafana.com

Which issue(s) this PR fixes or relates to

N/A

Checklist

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

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.

Would it be easy to add a check for this metric into some test? Exact numbers may be tricky, but at least verifying that "sum" is within expected range would be nice.

pkg/compactor/bucket_compactor.go Outdated Show resolved Hide resolved
@56quarters 56quarters marked this pull request as ready for review November 10, 2022 14:56
@56quarters 56quarters requested a review from a team as a code owner November 10, 2022 14:56
@56quarters
Copy link
Contributor Author

Would it be easy to add a check for this metric into some test? Exact numbers may be tricky, but at least verifying that "sum" is within expected range would be nice.

I can split the logic into a method and test that separately instead of adding to an existing test. WDYT?

Fixes an issue where a timestamp in milliseconds was subtracted from
a timestamp in seconds, resulting in negative deltas being recorded
instead of the actual delay between the block maxtime and "now".

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
@56quarters 56quarters force-pushed the 56quarters/compactor-delay-time-unit branch from b580257 to 85a5d33 Compare November 10, 2022 15:42
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.

lgtm, thanks for fixing reported seconds instead of millis.

pkg/compactor/bucket_compactor_test.go Show resolved Hide resolved
pkg/compactor/bucket_compactor.go Outdated Show resolved Hide resolved
Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
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!

@56quarters 56quarters enabled auto-merge (squash) November 10, 2022 16:11
@56quarters 56quarters merged commit ee050ff into main Nov 10, 2022
@56quarters 56quarters deleted the 56quarters/compactor-delay-time-unit branch November 10, 2022 16:21
masonmei pushed a commit to udmire/mimir that referenced this pull request Dec 16, 2022
Fixes an issue where a timestamp in milliseconds was subtracted from
a timestamp in seconds, resulting in negative deltas being recorded
instead of the actual delay between the block maxtime and "now".

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.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.

None yet

2 participants