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

Added tools/slo-rules-generator #77

Merged
merged 9 commits into from
Jan 18, 2022
Merged

Conversation

david-vavra
Copy link
Contributor

Adding tool to generate SLO recording rules with metadata for domains.

The original motivation was adding a new recording rule - slo:burn_rate_threshold in addition to already existing slo:stable_version and slo:violation_ratio_threshold. Introduction of SLO domain configuration lets us to automatically generate the metrics mentioned above and allows us further modify burn_rate alerts on per-domain basis.

*In future we will probably be generating custom burn-rate alerts for each domain,class,type instead of customizing the alerts via recording rules, but we are not there yet and this PR basically just sync our current internal state of tooling to github :) *

@david-vavra david-vavra requested a review from FUSAKLA January 3, 2022 19:51
Copy link
Contributor

@FUSAKLA FUSAKLA left a comment

Choose a reason for hiding this comment

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

Great, generally OK from me

There are a plenty of things we can iterate on, including generating of the alerts etc. leaving out all the joining but for now this is GTG

I added some minor nits for the future improvements, but I'm ok with merging it as is and iterating later.

👍

tools/slo-rules-generator/README.md Outdated Show resolved Hide resolved
}
}

func (d Domain) IsValid() []error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Future improvement:

Instead of adding everywhere the IsValid function which needs to be explicitly called, It's easier to override the UnmarshallYAML functions which would do the validation right a way

version: 6
alerting:
team: [email protected]
escalate: [email protected]
Copy link
Contributor

Choose a reason for hiding this comment

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

Future improvement:
Both these fields feel quite proprietary and bounded to our internal setup.

Maybe we might just add a something line additional_labels? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could be...

tools/slo-rules-generator/slo-domains.yaml.example Outdated Show resolved Hide resolved
tools/slo-rules-generator/slo-domains.yaml.example Outdated Show resolved Hide resolved
tools/slo-rules-generator/slo-domains.yaml.example Outdated Show resolved Hide resolved
tools/slo-rules-generator/slo-domains.yaml.example Outdated Show resolved Hide resolved
tools/slo-rules-generator/slo-domains.yaml.example Outdated Show resolved Hide resolved
type SloConfiguration map[string]Domain

func main() {
if len(os.Args) != 2 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Feature improvement:
Use Kingpin which simply solves all of this

@FUSAKLA
Copy link
Contributor

FUSAKLA commented Jan 5, 2022

Also, I'd consider mentioning it in the CHANGELOG marked as experimental just to let people know what direction we are heading

@david-vavra
Copy link
Contributor Author

Thanks for the review @FUSAKLA, I'm merging it in order to move this forward.

I agree that remaining comments should be addressed, however leaving them to a future refactor.

@david-vavra david-vavra merged commit e130164 into master Jan 18, 2022
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.

2 participants