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.

Sign up for GitHub

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

Jump to bottom

fix(filter): fix support custom retention for tagged metrics #950

Merged
merged 6 commits into from
Aug 12, 2024
Merged

fix(filter): fix support custom retention for tagged metrics #950

merged 6 commits into from
Aug 12, 2024

Conversation

kissken
Copy link
Member

@kissken kissken commented Oct 27, 2023

No description provided.

@kissken kissken requested a review from a team as a code owner October 27, 2023 11:16
@codecov-commenter
Copy link

Codecov Report

Merging #950 (4204ba3) into master (18568f0) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 4204ba3 differs from pull request most recent head 90cc505. Consider uploading reports for the commit 90cc505 to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@           Coverage Diff           @@
##           master     #950   +/-   ##
=======================================
  Coverage   69.25%   69.26%           
=======================================
  Files         196      196           
  Lines       11040    11041    +1     
=======================================
+ Hits         7646     7647    +1     
  Misses       2943     2943           
  Partials      451      451
Files Coverage Δ
filter/cache_storage.go 81.08% <100.00%> (+0.25%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Tetrergeru
Tetrergeru previously approved these changes Oct 30, 2023
View reviewed changes
almostinf
almostinf previously approved these changes Oct 30, 2023
View reviewed changes
@kissken
Copy link
Member Author

kissken commented Oct 31, 2023

/build

@almostinf almostinf dismissed stale reviews from Tetrergeru and themself via ec217d7 August 7, 2024 09:05
@almostinf
Copy link
Member

/build

almostinf
almostinf previously approved these changes Aug 7, 2024
View reviewed changes
Tetrergeru
Tetrergeru reviewed Aug 7, 2024
View reviewed changes
filter/cache_storage.go Outdated Show resolved Hide resolved
almostinf
almostinf previously approved these changes Aug 7, 2024
View reviewed changes
Tetrergeru
Tetrergeru previously approved these changes Aug 7, 2024
View reviewed changes
AleksandrMatsko
AleksandrMatsko requested changes Aug 9, 2024
View reviewed changes
Copy link
Member

@AleksandrMatsko AleksandrMatsko left a comment

Choose a reason for hiding this comment

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

Надушнил немного...

filter/cache_storage.go Outdated
@@ -11,7 +13,12 @@ import (
"github.com/moira-alert/moira/metrics"
)

var defaultRetention = 60
var (
defaultRetention = 60
Copy link
Member

Choose a reason for hiding this comment

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

Мб константой эту штуку сделать? Вроде нигде ей ничего не присваивается.

Copy link
Member

Choose a reason for hiding this comment

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

Сделал

filter/cache_storage.go Outdated
var (
defaultRetention = 60

InvalidRetentionsFormatErr = errors.New("Invalid retentions format, it is correct to write in the format 'retentions = timePerPoint:timeToStore, timePerPoint:timeToStore, ...'")
Copy link
Member

Choose a reason for hiding this comment

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

Ошибки не должны быть с маленькой буквы? (у меня Goland подчеркнул)

Copy link
Member

Choose a reason for hiding this comment

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

Резонно, ошибка на текущий момент по сути наша внутренняя и снаружи не нужна

Copy link
Member

Choose a reason for hiding this comment

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

Исправил

@almostinf almostinf dismissed stale reviews from Tetrergeru and themself via de92374 August 9, 2024 09:10
@almostinf
Copy link
Member

/build

@almostinf almostinf merged commit a5ba594 into master Aug 12, 2024
7 checks passed
@almostinf almostinf deleted the fix/custom_retentions_for_tagged_metrics branch August 12, 2024 07:22
@github-actions GitHub Actions
Copy link

Build and push Docker images with tag: 2024-08-12.a5ba594

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AleksandrMatsko AleksandrMatsko AleksandrMatsko approved these changes

@almostinf almostinf almostinf approved these changes

@Tetrergeru Tetrergeru Tetrergeru approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kissken @codecov-commenter @almostinf @Tetrergeru @AleksandrMatsko