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

feat(alerts): add support for ignoreOnExpectedTermination in expiration to NRQL alert conditions #1180

Merged

Conversation

corinnekunze
Copy link
Contributor

@corinnekunze corinnekunze commented Jun 25, 2024

Summary

https://new-relic.atlassian.net/browse/NR-270141
This PR includes changes to support the usage of a new field with baseline/static NRQL alert conditions, ignoreOnExpectedTermination. This field is expected to be live on production soon, in queries and mutations which manage baseline/static NRQL alert conditions (the expiration field in these queries and mutations has been updated to contain ignoreOnExpectedTermination). Examples of some of these mutations and queries have been depicted below.

image

PR with Terraform Provider changes

newrelic/terraform-provider-newrelic#2700

@@ -712,3 +484,240 @@ func TestIntegrationNrqlConditions_StreamingMethods(t *testing.T) {
}
}()
}

func TestIntegrationNrqlConditions_IgnoreOnExpectedTermination(t *testing.T) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New test

return baseCondition
}

func nrqlUpdateFactory(args ConditionArgs) NrqlConditionUpdateBase {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just doing some go experimenting/clean up here to create factory functions

@corinnekunze corinnekunze force-pushed the ignore-on-expected-termination branch from 2fcb04c to 54631ff Compare June 27, 2024 16:32
Copy link
Contributor

@akane0915 akane0915 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, I just had a couple nits

@@ -731,6 +732,7 @@ const (
closeViolationsOnExpiration
expirationDuration
openViolationOnExpiration
ignoreOnExpectedTermination
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this indentation weird or is it just on my screen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just weird in the git diff I think
Screenshot 2024-06-27 at 12 51 54 PM

Copy link
Member

Choose a reason for hiding this comment

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

Hey, it seemed to me like this was actually unindented in the code, which is why I tried fixing it; and it turns out it actually is something weird with GitHub. Apologies, please ignore my latest commit 38a14d7

(here's what it looks like after the indent was fixed, but GitHub's diff still shows an extra indent).
image

policy, err := client.CreatePolicyMutation(testAccountID, testPolicy)
require.NoError(t, err)

// Test: Create (static condition with streaming methods fields)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Update comment to `Test: Create (static condition with expected termination field)

@corinnekunze corinnekunze force-pushed the ignore-on-expected-termination branch from 2ba6115 to 2c3b154 Compare August 7, 2024 17:45
@codecov-commenter
Copy link

codecov-commenter commented Aug 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 38.40%. Comparing base (3ff4ee2) to head (cdaf36b).
Report is 83 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1180      +/-   ##
==========================================
- Coverage   38.84%   38.40%   -0.44%     
==========================================
  Files          86       96      +10     
  Lines        5612     4822     -790     
==========================================
- Hits         2180     1852     -328     
+ Misses       3266     2795     -471     
- Partials      166      175       +9     
Flag Coverage Δ
unit 38.40% <ø> (-0.44%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pranav-new-relic pranav-new-relic self-requested a review August 12, 2024 04:26
@pranav-new-relic pranav-new-relic changed the title Ignore on expected termination feat(alerts): add support for ignoreOnExpectedTermination to queries and mutations dealing in baseline NRQL alert conditions Aug 12, 2024
chore(alerts): fix indentation of the newly added ignoreOnExpectedTermination field
@pranav-new-relic pranav-new-relic force-pushed the ignore-on-expected-termination branch from 38a14d7 to cdaf36b Compare August 12, 2024 05:05
@pranav-new-relic pranav-new-relic changed the title feat(alerts): add support for ignoreOnExpectedTermination to queries and mutations dealing in baseline NRQL alert conditions feat(alerts): add support for ignoreOnExpectedTermination to NRQL alert conditions Aug 12, 2024
@pranav-new-relic pranav-new-relic changed the title feat(alerts): add support for ignoreOnExpectedTermination to NRQL alert conditions feat(alerts): add support for ignoreOnExpectedTermination in expiration to NRQL alert conditions Aug 12, 2024
@pranav-new-relic pranav-new-relic merged commit 4acfff3 into newrelic:main Aug 12, 2024
13 checks passed
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.

4 participants