Skip to content
This repository has been archived by the owner on Jun 9, 2023. It is now read-only.

Feature/kebechet analysis specific day #411

Merged

Conversation

xtuchyna
Copy link
Member

Related Issues and Dependencies

Related to thoth-station/thoth-application#1500 (comment)
and thoth-station/mi-scheduler#131
and thoth-station/kebechet#546

This introduces a breaking change

  • Yes
  • No

This Pull Request implements

Instead of using today's date as default, user needs to specify now from which date the kebechet statistics will be collected.
This allows us an inspection of yesterday's data for kebechet stats, so that we can merge all the collected data for a SLI/SLO metrics on how the kebechet did the day before.

@sesheta sesheta added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 31, 2021
@xtuchyna xtuchyna requested a review from pacospace May 31, 2021 14:40
Copy link
Contributor

@pacospace pacospace left a comment

Choose a reason for hiding this comment

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

Few coments, otherwise lgtm!

@@ -160,9 +161,12 @@ def cli(
for project in repos:
os.environ["PROJECT"] = project

today = date.today()
yesterday = today.replace(day=today.day - 1)
Copy link
Contributor

@pacospace pacospace Jun 1, 2021

Choose a reason for hiding this comment

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

This is going to give error, if you don't change also month and eventually year.
For example today is 2021-06-01, if you run yesterday = today.replace(day=today.day - 1) you will get error: ValueError: day is out of range for month, unless you change also month and you will get problem with year on 2022-01-01.

A more robust/easier solution is to use:

from datetime import date, timedelta
today = date.today()
yesterday = today - timedelta(days=1)

which returns correctly 2021-05-31

file_name += ".json"

KnowledgeStorage(is_local=self.is_local).save_data(file_path=path.joinpath(file_name), data=stats)

@staticmethod
def merge_kebechet_metrics_today(is_local: bool = False):
def merge_kebechet_metrics_for_day(day: date, is_local: bool = False):
Copy link
Contributor

Choose a reason for hiding this comment

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

might be per_day instead of for_day in this case.

@xtuchyna xtuchyna requested a review from pacospace June 1, 2021 09:42
@xtuchyna
Copy link
Member Author

xtuchyna commented Jun 1, 2021

/approve

@sesheta
Copy link
Member

sesheta commented Jun 1, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: xtuchyna

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@sesheta sesheta added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 1, 2021
@sesheta sesheta merged commit abece86 into thoth-station:master Jun 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants