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

Add Minio Sloth rules #254

Merged
merged 2 commits into from
Nov 7, 2023
Merged

Add Minio Sloth rules #254

merged 2 commits into from
Nov 7, 2023

Conversation

Kidswiss
Copy link
Contributor

@Kidswiss Kidswiss commented Nov 1, 2023 •
edited
Loading

  • Added SLOTH rules for Minio
  • Added functions to generate the prometheus queries for SLOTH
    • Found and fixed some copy/paste errors between postgresql and redis

Checklist

  • The PR has a meaningful title. It will be used to auto generate the
    changelog.
    The PR has a meaningful description that sums up the change. It will be
    linked in the changelog.
  • PR contains a single logical change (to build a better changelog).
  • Categorize the PR by adding one of the labels:
    bug, enhancement, documentation, change, breaking, dependency
    as they show up in the changelog.
  • Link this PR to related issues or PRs.

@Kidswiss Kidswiss added the enhancement New feature or request label Nov 1, 2023
Kidswiss
Kidswiss commented Nov 1, 2023
View reviewed changes
component/class/defaults.yml Outdated
@@ -39,7 +39,7 @@ parameters:
appcat:
registry: ghcr.io
repository: vshn/appcat
tag: v4.37.0
tag: sli_minio
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will change this once the SLI exporter is released

Kidswiss
Kidswiss commented Nov 1, 2023
View reviewed changes
component/class/defaults.yml
@@ -168,6 +168,16 @@ parameters:
# If the alert is pending for more than 5m this indicates a real problem.
for: 6m
ticket_alert: {}
minio:
uptime:
objective: 99.9
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've used the same values as redis/postgres, any objections?

Copy link
Member

Choose a reason for hiding this comment

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

since we haven't got any special requirements, I'd keep it as default

Kidswiss
Kidswiss commented Nov 1, 2023
View reviewed changes
component/class/defaults.yml
@@ -420,6 +430,7 @@ parameters:
helmChartVersion: ${appcat:charts:minio:version}
grpcEndpoint: ${appcat:grpcEndpoint}
defaultPlan: standard-1
sla: 99.25
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've used the same values as redis/postgres, any objections?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep the same value across other services for now.

Copy link
Member

Choose a reason for hiding this comment

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

since we haven't got any special requirements, I'd keep it as default

@Kidswiss Kidswiss requested review from a team, TheBigLee, wejdross and zugao and removed request for a team November 1, 2023 10:37
wejdross
wejdross approved these changes Nov 2, 2023
View reviewed changes
Copy link
Member

@wejdross wejdross left a comment

Choose a reason for hiding this comment

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

aprooved

component/class/defaults.yml
@@ -168,6 +168,16 @@ parameters:
# If the alert is pending for more than 5m this indicates a real problem.
for: 6m
ticket_alert: {}
minio:
uptime:
objective: 99.9
Copy link
Member

Choose a reason for hiding this comment

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

since we haven't got any special requirements, I'd keep it as default

component/class/defaults.yml
@@ -420,6 +430,7 @@ parameters:
helmChartVersion: ${appcat:charts:minio:version}
grpcEndpoint: ${appcat:grpcEndpoint}
defaultPlan: standard-1
sla: 99.25
Copy link
Member

Choose a reason for hiding this comment

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

since we haven't got any special requirements, I'd keep it as default

component/component/provider.jsonnet Outdated Show resolved Hide resolved
component/component/slos.libsonnet
@@ -58,17 +58,25 @@ local prometheusRule(name) =
spec: patchedRules,
};

local getEvents(serviceName) = {
Copy link
Member

Choose a reason for hiding this comment

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

I love this refactor :D

component/tests/golden/vshn/appcat/appcat/sli_exporter/90_slo_vshn_redis_ha.yaml
@@ -71,7 +71,7 @@ spec:
sloth_window: 1d
record: slo:sli_error:ratio_rate1d
- expr: |
((sum(rate(appcat_probes_seconds_count{reason!="success", service="VSHNRedis", ha="true"}[3d]) or 0*rate(appcat_probes_seconds_count{service="VSHNRedis"}[3d])) by (service, namespace, name, organization, sla) or vector(0)) - scalar(appcat:cluster:maintenance) > 0 or sum(0*rate(appcat_probes_seconds_count{service="VSHNRedis"}[3d])) by (service, namespace, name, organization, sla))
Copy link
Member

Choose a reason for hiding this comment

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

good catch

zugao
zugao requested changes Nov 2, 2023
View reviewed changes
Copy link
Contributor

@zugao zugao left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Some things to look at

component/class/defaults.yml
@@ -420,6 +430,7 @@ parameters:
helmChartVersion: ${appcat:charts:minio:version}
grpcEndpoint: ${appcat:grpcEndpoint}
defaultPlan: standard-1
sla: 99.25
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep the same value across other services for now.

component/component/provider.jsonnet Outdated Show resolved Hide resolved
@Kidswiss Kidswiss force-pushed the add/minio_sla branch 2 times, most recently from 8f500a6 to 713106c Compare November 2, 2023 12:18
zugao
zugao approved these changes Nov 2, 2023
View reviewed changes
Copy link
Contributor

@zugao zugao left a comment

Choose a reason for hiding this comment

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

LGTM

@Kidswiss Kidswiss force-pushed the add/minio_sla branch 3 times, most recently from ec0d5d2 to 17e8ab0 Compare November 7, 2023 14:44
@Kidswiss Kidswiss merged commit 0090986 into master Nov 7, 2023
22 checks passed
@Kidswiss Kidswiss deleted the add/minio_sla branch November 7, 2023 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zugao zugao zugao approved these changes

@wejdross wejdross wejdross approved these changes

@TheBigLee TheBigLee Awaiting requested review from TheBigLee TheBigLee was automatically assigned from vshn/schedar

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Kidswiss @wejdross @zugao