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

feat(filter,cli): add remove future metrics in cli and discarding metrics written into the future #1012

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

almostinf
Copy link
Member

@almostinf almostinf commented Apr 16, 2024

Add remove future metrics in cli and discarding metrics written into the future

Added ability to delete metrics via --cleanup-future-metrics command in cli with specifying cleanup_future_metrics_duration parameter in config, starting from which metrics written to the future will be deleted

Also added discarding metrics whose timestamp is greater than now() + dropMetricsTTL to avoid writing garbage to the database

And accelerated tests by 8 seconds by replacing time.Sleep with metricsCache.Flush.

@almostinf almostinf requested a review from a team as a code owner April 16, 2024 12:45
log.Info().Msg("Cleanup of outdated metrics finished")
}

if *cleanupFutureMetrics {
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 Author

@almostinf almostinf Apr 17, 2024

Choose a reason for hiding this comment

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

Мне кажется, это не нужно, тк операция удаления метрик в будущее довольно тяжелая, тк требует прохода по всем метрикам, в идеале 1 раз удалить метрики, уже записанные, а все новые, пишущиеcя в будущее, и так будут откидываться

cmd/cli/main.go Outdated Show resolved Hide resolved
c := *connector.client
result, err := c.ZRemRangeByScore(connector.context, metricDataKey(metric), "-inf", strconv.FormatInt(toTime, 10)).Result()
result, err := c.ZRemRangeByScore(connector.context, metricDataKey(metric), from, to).Result()
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 Author

@almostinf almostinf Apr 17, 2024

Choose a reason for hiding this comment

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

так я передаю "-inf", только делаю это на уровне выше, идея была в переиспользовании этой функции

@almostinf almostinf requested a review from kissken April 17, 2024 10:05
@almostinf
Copy link
Member Author

/build

Copy link

Build and push Docker images with tag: fix-remove-future-metrics.2024-04-18.25406f9

1 similar comment
Copy link

Build and push Docker images with tag: fix-remove-future-metrics.2024-04-18.25406f9

kissken
kissken previously approved these changes Apr 25, 2024
@codecov-commenter
Copy link

codecov-commenter commented Apr 27, 2024

Codecov Report

Attention: Patch coverage is 74.41860% with 11 lines in your changes are missing coverage. Please review.

Project coverage is 67.82%. Comparing base (1f2b4ac) to head (3d10991).
Report is 6 commits behind head on master.

❗ Current head 3d10991 differs from pull request most recent head 959e096. Consider uploading reports for the commit 959e096 to get more accurate results

Files Patch % Lines
cmd/cli/main.go 0.00% 9 Missing ⚠️
database/redis/metric.go 90.00% 1 Missing and 1 partial ⚠️

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

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1012      +/-   ##
==========================================
- Coverage   67.92%   67.82%   -0.11%     
==========================================
  Files         212      212              
  Lines       12024    12098      +74     
==========================================
+ Hits         8167     8205      +38     
- Misses       3359     3388      +29     
- Partials      498      505       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

3 participants