Skip to content

feat(alerts): adds support for signal_seasonality for NRQL baseline conditions #2844

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

Merged

Conversation

akane0915
Copy link
Contributor

@akane0915 akane0915 commented Mar 31, 2025

Description

Related to Go Client PR

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

Checklist:

Please delete options that are not relevant.

  • My commit message follows conventional commits
  • My code is formatted to Go standards
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes. Go here for instructions on running tests locally.

How to test this change?

Please describe how to test your changes. Include any relevant steps in the UI, HCL file(s), commands, etc

  • Try creating a newrelic_nrql_alert_condition of type baseline.
  • Ensure you can set signal_seasonality successfully.
  • Try creating a newrelic_nrql_alert_condition of type static without signal_seasonality.

E.g.

resource "newrelic_nrql_alert_condition" "baseline_condition" {
  account_id                   = 400304
  policy_id                    = 1514915
  type                         = "baseline"
  name                         = "Baseline Condition from Terraform"
  enabled                      = false
  violation_time_limit_seconds = 3600
  baseline_direction           = "upper_only"
  nrql {
    query = "SELECT average(duration) FROM Transaction"
  }
  critical {
    operator              = "above"
    threshold             = 1
    threshold_duration    = 300
    threshold_occurrences = "all"
  }
  aggregation_window = 60
  aggregation_method = "event_flow"
  aggregation_delay  = 120
  fill_option        = "static"
  fill_value         = 1
  signal_seasonality = "daily"
}

@akane0915 akane0915 marked this pull request as ready for review April 9, 2025 20:40
@codecov-commenter
Copy link

codecov-commenter commented Apr 10, 2025

Codecov Report

Attention: Patch coverage is 17.54386% with 47 lines in your changes missing coverage. Please review.

Project coverage is 30.55%. Comparing base (6cbc205) to head (9f3517e).
Report is 26 commits behind head on main.

Files with missing lines Patch % Lines
...wrelic/structures_newrelic_nrql_alert_condition.go 17.30% 41 Missing and 2 partials ⚠️
newrelic/resource_newrelic_nrql_alert_condition.go 20.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2844      +/-   ##
==========================================
- Coverage   30.58%   30.55%   -0.03%     
==========================================
  Files         104      104              
  Lines       28631    28687      +56     
==========================================
+ Hits         8756     8765       +9     
- Misses      19701    19746      +45     
- Partials      174      176       +2     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@akane0915 akane0915 force-pushed the NR-368788/NRQL_condition_seasonality branch from df717f0 to 06c81df Compare April 14, 2025 20:45
@@ -396,7 +396,8 @@ func resourceNewRelicNrqlAlertCondition() *schema.Resource {
Description: "Seasonality under which a condition's signal(s) are evaluated. Valid values are: 'NEW_RELIC_CALCULATION', 'HOURLY', 'DAILY', 'WEEKLY', or 'NONE'. To have New Relic calculate seasonality automatically, set to 'NEW_RELIC_CALCULATION' (default). To turn off seasonality completely, set to 'NONE'.",
ValidateFunc: validation.StringInSlice([]string{"NEW_RELIC_CALCULATION", "HOURLY", "DAILY", "WEEKLY", "NONE"}, true),
DiffSuppressFunc: func(k, old, new string, d *schema.ResourceData) bool {
return strings.EqualFold(old, new) // Case fold this attribute when diffing
// If a value is not provided and the condition uses the default value, don't show a diff. Also case insensitive.
return (strings.EqualFold(old, "NEW_RELIC_CALCULATION") && new == "") || strings.EqualFold(old, new)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Use case for this is:

  • User creates a baseline condition with no signal seasonality specified.
  • We default to "NEW_RELIC_CALCULATION" in the backend
  • User updates a different field, say enabled
  • Without this diff suppress function, there would be a diff of "NEW_RELIC_CALCULATION" ~> null which wouldn't actually doing anything to the condition but it might confuser the user.

Copy link
Member

Choose a reason for hiding this comment

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

👍

@pranav-new-relic
Copy link
Member

pranav-new-relic commented Apr 16, 2025

All is well with the integration tests and lint checks :) (as of the latest commit, 723e61b which references newrelic/newrelic-client-go@8671fd0 in the New Relic Go Client)

foreman run go test -v ./newrelic -tags integration -run '^TestAccNewRelicNrqlAlertCondition'
integration_test_run_2844_723e61bd4c883184816011344036499a54840155.mov

@pranav-new-relic
Copy link
Member

🚧

Comment on lines 397 to 401
ValidateFunc: validation.StringInSlice([]string{"NEW_RELIC_CALCULATION", "HOURLY", "DAILY", "WEEKLY", "NONE"}, true),
DiffSuppressFunc: func(k, old, new string, d *schema.ResourceData) bool {
// If a value is not provided and the condition uses the default value, don't show a diff. Also case insensitive.
return (strings.EqualFold(old, "NEW_RELIC_CALCULATION") && new == "") || strings.EqualFold(old, new)
},
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ValidateFunc: validation.StringInSlice([]string{"NEW_RELIC_CALCULATION", "HOURLY", "DAILY", "WEEKLY", "NONE"}, true),
DiffSuppressFunc: func(k, old, new string, d *schema.ResourceData) bool {
// If a value is not provided and the condition uses the default value, don't show a diff. Also case insensitive.
return (strings.EqualFold(old, "NEW_RELIC_CALCULATION") && new == "") || strings.EqualFold(old, new)
},
ValidateFunc: validation.StringInSlice(
[]string{
string(alerts.NrqlSignalSeasonalities.NewRelicCalculation),
string(alerts.NrqlSignalSeasonalities.Hourly),
string(alerts.NrqlSignalSeasonalities.Daily),
string(alerts.NrqlSignalSeasonalities.Weekly),
string(alerts.NrqlSignalSeasonalities.None),
},
true,
),
DiffSuppressFunc: func(k, old, new string, d *schema.ResourceData) bool {
// If a value is not provided and the condition uses the default value, don't show a diff. Also case insensitive.
return (strings.EqualFold(old, string(alerts.NrqlSignalSeasonalities.NewRelicCalculation)) && new == "") || strings.EqualFold(old, new)
},

Copy link
Member

Choose a reason for hiding this comment

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

just wanted to replace hard coded references to each of these strings with references in the client as a good practice so as to facilitate easier modifications to the values corresponding to each of these types (if any) in the future :)

Comment on lines 77 to 83
if conditionType == "baseline" {
if attr, ok := d.GetOk("signal_seasonality"); ok {
seasonality := alerts.NrqlSignalSeasonality(strings.ToUpper(attr.(string)))
input.SignalSeasonality = &seasonality
}
}

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if conditionType == "baseline" {
if attr, ok := d.GetOk("signal_seasonality"); ok {
seasonality := alerts.NrqlSignalSeasonality(strings.ToUpper(attr.(string)))
input.SignalSeasonality = &seasonality
}
}
if conditionType == "baseline" {
if attr, ok := d.GetOk("signal_seasonality"); ok {
seasonality := alerts.NrqlSignalSeasonality(strings.ToUpper(attr.(string)))
input.SignalSeasonality = &seasonality
} else {
seasonality := alerts.NrqlSignalSeasonalities.NewRelicCalculation
input.SignalSeasonality = &seasonality
}
}

I'll explain why I suggest this modification in the following comment (just under this).

Comment on lines 144 to 150
if conditionType == "baseline" {
if attr, ok := d.GetOk("signal_seasonality"); ok {
seasonality := alerts.NrqlSignalSeasonality(strings.ToUpper(attr.(string)))
input.SignalSeasonality = &seasonality
}
}

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if conditionType == "baseline" {
if attr, ok := d.GetOk("signal_seasonality"); ok {
seasonality := alerts.NrqlSignalSeasonality(strings.ToUpper(attr.(string)))
input.SignalSeasonality = &seasonality
}
}
if conditionType == "baseline" {
if attr, ok := d.GetOk("signal_seasonality"); ok {
seasonality := alerts.NrqlSignalSeasonality(strings.ToUpper(attr.(string)))
input.SignalSeasonality = &seasonality
} else {
seasonality := alerts.NrqlSignalSeasonalities.NewRelicCalculation
input.SignalSeasonality = &seasonality
}
}

I'd like to explain why I suggest this change here, and also a similar change in the function handling create (the above comment). While we have handled all discrepancies we've noticed by

  • restoring omitempty with the discussed datatypes in the client,
  • introducing a new NEW_RELIC_CALCULATION type instead of null (which led to the problem with an omitempty not existing), and
  • handling diff suppression in the provider here to suppress diffs if the API returns NEW_RELIC_CALCULATION against signal_seasonality but the value in the configuration is null,

one of the downsides I'm seeing with the current code in the provider and the client is the fact that because omitempty is back, it becomes impossible for customers who could have already set a valid signal_seasonality (e.g. DAILY) to remove it entirely (in API terms, move it back to NEW_RELIC_CALCULATION); while the API does this under the hoodbased on what you shared earlier when there is no signalSeasonality in the request, it doesn't seem so since a diff is shown after trying to remove an already configured signal_seasonality = DAILY and performing an apply, which is leaving a diff upon the next plan suggesting that DAILY was never gone.

We can visit more details on why this happens later, but I believe a simpler solution to this problem (as you've implemented everything else like the diff suppression along the lines of this problem :)) is to add an else condition to wherever we're adding signalSeasonality in the create/update handlers, to say that if there is a non-null signal_seasonality in the state, that is picked and sent to the API, else, NEW_RELIC_CALCULATION is sent to the API; by which means "all" baseline conditions created/updated will have a signalSeasonality for sure, which will help keep the experience consistent and prevent such drift - this is basically a way of making NEW_RELIC_CALCULATION the default signal_seasonality on baseline NRQL conditions, unless a different value is specified.

To better understand what I mean, you can try compiling these changes (without my suggestions), try applying an NRQL alert condition with signal_seasonality = DAILY, then removing it and trying to apply again; the expected behaviour of the subsequent plan after this would be to see no drift, but we'd see a drift as I've explained above. You can then add these suggestions, and the drift would be seen.

I'd like you to review if these changes sound apt to you and add them accordingly :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes sense. I actually did notice this in my testing but I figured that instead of removing the signalSeasonality entirely on update, a user could just set it to NEW_RELIC_CALCULATION which is effectively the same. I didn't think to do this, and I think this is a good idea!

@@ -180,6 +181,7 @@ resource "newrelic_nrql_alert_condition" "foo" {

# baseline type only
baseline_direction = "upper_only"
signal_seasonality = "none"
Copy link
Member

Choose a reason for hiding this comment

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

how about adding a valid signal_seasonality here like HOURLY or DAILY? @akane0915

Copy link
Contributor Author

Choose a reason for hiding this comment

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

NONE is a valid signal_seasonality but I can update it anyways.

Comment on lines +153 to +154
// Null is equivalent to the default value of `NEW_RELIC_CALCULATION` in the API
// so this effectively allows the user to null out the signal seasonality on update
Copy link
Member

Choose a reason for hiding this comment

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

@akane0915 nice, I like the comment here :)

@pranav-new-relic
Copy link
Member

Integration tests work fine with the latest changes too. We should be good to proceed as all requested changes have been handled.

@pranav-new-relic pranav-new-relic merged commit d54c1f9 into newrelic:main Apr 17, 2025
11 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.

5 participants