-
Notifications
You must be signed in to change notification settings - Fork 130
Add pyrra_url annotations #1522
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
base: main
Are you sure you want to change the base?
Conversation
Add proper pyrra_url annotations with grouping parameters to all grouping test cases in slo/rules_test.go. The annotations now include URL-encoded grouping parameters that match the labels used in the sum by expressions: This ensures consistency with the already-fixed http-ratio-grouping test case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds a new pyrra_url
annotation to all generated alert rules, allowing users to link directly from an alert to its SLO dashboard in Pyrra.
- Changed
Burnrates
(and related) signatures to accept an external URL. - Updated
commonRuleAnnotations
to construct and includepyrra_url
. - Propagated the external URL through CLI commands, filesystem, Kubernetes controller, and generator.
- Augmented existing tests and added
TestObjective_PyrraURLAnnotation
.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
slo/rules.go | Extended Burnrates and commonRuleAnnotations to add pyrra_url . |
slo/rules_test.go | Updated expected annotations and added a test for Pyrra URL. |
main.go | Passed CLI ExternalURL into rule‐generation functions. |
kubernetes/controllers/servicelevelobjective.go | Added PyrraExternalURL field and threaded it into rule creation. |
kubernetes.go | Wired PyrraExternalURL through cmdKubernetes . |
generate.go | Expanded cmdGenerate to accept and pass ExternalURL. |
filesystem.go | Expanded cmdFilesystem and writeRuleFile to propagate ExternalURL. |
Comments suppressed due to low confidence (2)
slo/rules_test.go:2094
- Tests cover the base URL case but not scenarios where
Grouping()
returns labels. Add a test case to verify that grouping parameters appear correctly in the pyrra_url.
func TestObjective_PyrraURLAnnotation(t *testing.T) {
slo/rules.go:609
- [nitpick] The method accepts
externalURL
as a string and reparses it. Consider accepting a*url.URL
to avoid repeated parsing and improve type safety.
func (o Objective) commonRuleAnnotations(externalURL string) map[string]string {
|
||
externalURLParsed, err := url.Parse(externalURL) | ||
if err != nil { | ||
return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning nil on URL parse error will drop any existing annotations. Instead, log the error and continue without adding pyrra_url (or return the existing annotations map).
return nil | |
fmt.Printf("Error parsing external URL: %v\n", err) | |
// Continue without adding pyrra_url | |
return annotations |
Copilot uses AI. Check for mistakes.
groups = append(groups, l+"=%22{{$labels."+l+"}}%22") | ||
} | ||
annotations["pyrra_url"] += "&grouping=%7B" + strings.Join(groups, ",") + "%7D" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Manually appending URL-encoded grouping parameters is error-prone. Consider adding grouping via url.Values
and let Encode()
handle proper escaping.
groups = append(groups, l+"=%22{{$labels."+l+"}}%22") | |
} | |
annotations["pyrra_url"] += "&grouping=%7B" + strings.Join(groups, ",") + "%7D" | |
groups = append(groups, l+"={{$labels."+l+"}}") | |
} | |
groupingParams := url.Values{} | |
groupingParams.Add("grouping", "{"+strings.Join(groups, ",")+"}") | |
annotations["pyrra_url"] += "&" + groupingParams.Encode() |
Copilot uses AI. Check for mistakes.
This PR adds a new
pyrra_url
annotation to all generated alert rules. This feature allows users to navigate directly from alerts to the corresponding SLO dashboard in Pyrra, improving the incident response workflow.Changes
pyrra_url
annotation to all ErrorBudgetBurn alertsCloses #1091