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 VaultAutopilotHealthy alert name and time window in alert descriptions #1157

Closed
wants to merge 4 commits into from
Closed

Fix VaultAutopilotHealthy alert name and time window in alert descriptions #1157

wants to merge 4 commits into from

Conversation

in0rdr
Copy link
Contributor

@in0rdr in0rdr commented Dec 4, 2023

I would like to improve the alert name for the autopilot.

Also, I noticed a link that no longer points to a meaningful location. We can remove it.

@in0rdr in0rdr requested review from pree, eyenx and a team as code owners December 4, 2023 15:49
@in0rdr in0rdr requested a review from hairmare December 4, 2023 15:49
@github-actions github-actions bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Dec 4, 2023
pree
pree requested changes Dec 4, 2023
View reviewed changes
Copy link
Member

@pree pree left a comment

Choose a reason for hiding this comment

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

Looks good to me, please bump the chart and run hack/helm-docs.sh

pree
pree approved these changes Dec 4, 2023
View reviewed changes
Copy link
Member

@pree pree 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.

Thanks, please also update security-apps (in a separate PR) to include the new version.

@in0rdr in0rdr force-pushed the fix/vault-monitoring-descriptions branch from 0c69cbd to ab67db6 Compare December 4, 2023 16:15
@pree
Copy link
Member

pree commented Dec 4, 2023

@adfinis/helm-charts what do you think about bumping this chart to 1.0.0 ?

@github-actions github-actions bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Dec 4, 2023
@in0rdr in0rdr force-pushed the fix/vault-monitoring-descriptions branch from af851c8 to 73f97ea Compare December 4, 2023 16:28
@in0rdr in0rdr changed the title fix: VaultAutopilotHealthy alert name Fix VaultAutopilotHealthy alert name and time window in alert descriptions Dec 4, 2023
@eyenx
Copy link
Member

eyenx commented Dec 4, 2023

@adfinis/helm-charts what do you think about bumping this chart to 1.0.0 ?

Hmm read fix and this big bump and was like why? Are you sure this stuff is 1.0 ready?

@in0rdr
Copy link
Contributor Author

in0rdr commented Dec 5, 2023

Looking at https://semver.org/#spec-item-5, we only need to care about the "breaking" change if we have 1.*? Because probably nobody takes your word for it before 1.* anyways (i.e., it's not yet a public API so you can do whatever). Let me revert to 0.3.0, I was not aware of that rule nr. 5

@in0rdr in0rdr force-pushed the fix/vault-monitoring-descriptions branch from 73f97ea to 2adb3a0 Compare December 5, 2023 09:30
pree
pree requested changes Dec 5, 2023
View reviewed changes
charts/vault-monitoring/values.yaml
- alert: VaultResponseFailures
expr: increase(vault_audit_log_response_failure[5m]) > 0
for: 15m
labels:
severity: critical
annotations:
summary: High frequency of failed Vault responses
description: There has been an increased number of failed Vault responses in the last 15 minutes
description: There has been an increased number of failed Vault responses in the last 20 minutes
Copy link
Member

Choose a reason for hiding this comment

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

How has this changed? It's still for: 15m

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 because 15m on the check + 5m in the query add up to 20m?

@eyenx
Copy link
Member

eyenx commented Dec 5, 2023

Looking at https://semver.org/#spec-item-5, we only need to care about the "breaking" change if we have 1.*? Because probably nobody takes your word for it before 1.* anyways (i.e., it's not yet a public API so you can do whatever). Let me revert to 0.3.0, I was not aware of that rule nr. 5

Exactly. That is what is keeping @hairmare from going 1.x on ìnfra-apps` I think

@tongpu
Copy link
Member

tongpu commented Dec 5, 2023

Looking at https://semver.org/#spec-item-5, we only need to care about the "breaking" change if we have 1.*? Because probably nobody takes your word for it before 1.* anyways (i.e., it's not yet a public API so you can do whatever). Let me revert to 0.3.0, I was not aware of that rule nr. 5

I honestly believe that we depend on too many downstream charts in our *-apps charts that it's going to be close to impossible to ever provide a stable public API.

@tongpu tongpu mentioned this pull request Dec 5, 2023
Closed
@hairmare
Copy link
Contributor

hairmare commented Dec 5, 2023

I honestly believe that we depend on too many downstream charts in our *-apps charts that it's going to be close to impossible to ever provide a stable public API.

i believe that as well, but this is vault-monitoring hence the reasoning doesn't apply

@in0rdr in0rdr force-pushed the fix/vault-monitoring-descriptions branch from a7240f7 to eed5b77 Compare June 11, 2024 09:38
@in0rdr in0rdr mentioned this pull request Jun 11, 2024
Closed
@in0rdr in0rdr closed this Jun 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pree pree pree requested changes

@eyenx eyenx Awaiting requested review from eyenx eyenx is a code owner

@hairmare hairmare Awaiting requested review from hairmare hairmare is a code owner automatically assigned from adfinis/helm-charts

Assignees
No one assigned
Labels
size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@in0rdr @pree @eyenx @tongpu @hairmare