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

Changes in Notification rule to allow types #209

Merged
merged 6 commits into from
Apr 26, 2024
Merged

Conversation

DibyojyotiS
Copy link
Contributor

Description

For non-streaming notification. Particularly for integrations supporting a batched type of notification.

Testing

Please describe the tests that you ran to verify your changes. Please summarize what did you test and what needs to be tested e.g. deployed and tested helm chart locally.

Checklist:

  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • Any dependent changes have been merged and published in downstream modules

Documentation

Make sure that you have documented corresponding changes in this repository or hypertrace docs repo if required.

@DibyojyotiS DibyojyotiS requested a review from a team as a code owner April 23, 2024 07:24
Copy link

github-actions bot commented Apr 23, 2024

Test Results

120 tests  ±0   120 ✅ ±0   1m 0s ⏱️ +4s
 29 suites ±0     0 💤 ±0 
 29 files   ±0     0 ❌ ±0 

Results for commit 5264402. ± Comparison against base commit 4a1dd43.

♻️ This comment has been updated with latest results.

@DibyojyotiS DibyojyotiS changed the title ENG-43011: Changes in Notification rule to allow types Changes in Notification rule to allow types Apr 23, 2024
@DibyojyotiS DibyojyotiS requested a review from japoorv April 24, 2024 06:56
japoorv
japoorv previously approved these changes Apr 24, 2024
@singhalprerana
Copy link
Contributor

@aaron-steinfeld the policy-engine changes are still some time away and this is a priority requirement, hence we need to do the changes now.. cc: @avinashkolluru

singhalprerana
singhalprerana previously approved these changes Apr 26, 2024
string rate_limit_interval_duration = 6;

oneof notification_target {
Copy link
Contributor

Choose a reason for hiding this comment

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

@aaron-steinfeld Based on the slack discussion (pasted the snippet below), hope we can go forward with this?

now one particular notification condition requires a special notification config

that’s how the requirement goes..

Are they actually agnostic though? every notification type can be sent to every channel already?

yes AFAIK and from the user UI perspective too

Agree an enum is better here, but since both would be placeholders until we migrate to policy engine, it doesn’t seem like it’s a significant amount of tech debt to take on. The whole idea of a channel is the real tech debt.

if that’s anyways true, there shouldn’t be any harm to have an enum.. that would be clearer of the 2 options..

Copy link
Contributor Author

@DibyojyotiS DibyojyotiS Apr 26, 2024

Choose a reason for hiding this comment

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

if that’s anyways true, there shouldn’t be any harm to have an enum.. that would be clearer of the 2 options..

Which Enum are we referring to?

Copy link
Contributor

Choose a reason for hiding this comment

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

that’s how the requirement goes..

My point here was that you have the same issue whether you make the channel deal with it or the notification rule (since was one of the points made against putting it in the channel rule).

hope we can go forward with this?

I don't like this approach, but I also won't block it. The one thing I'd still like to re-emphasize is to leave channel id as it is. We don't want to migrate all of the rules and now channel can basically just show up in two places making these much harder to work with. You can add the integration target as a sibling and un-deprecate the channel id to dramatically reduce the blast radius of this change.

Copy link
Contributor

@aaron-steinfeld aaron-steinfeld Apr 26, 2024

Choose a reason for hiding this comment

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

Which Enum are we referring to?

The integration type - my preference was to use a string there so this notification is not tightly coupled with integration support as it's just one more place to change when adding a new type (since the only code that actually needs to know about the type is the code that's evaluating the rule - here it's just a passthrough).

Copy link
Contributor

@singhalprerana singhalprerana Apr 26, 2024

Choose a reason for hiding this comment

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

@aaron-steinfeld currently channel allows co-existence of multiple components - say email+webhook+splunk.. But in this case, Wiz cannot coexist with these other components because of its different behaviour. I was preferring keeping this restriction as part of the API rather than the channel implementation..
Also the end-user (meaning UI) needs to know the distinction b/w the channel and this integration to drive the rest of the notification schema..

Copy link
Contributor

Choose a reason for hiding this comment

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

^^^ works for me.. missed the part that we can add optional to string without breaking API.. @DibyojyotiS let's go with this..

Copy link
Contributor

Choose a reason for hiding this comment

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

it's technically a breaking change, but I think the functional impact is less than if we left it deprecated and had a second way of referencing a channel.

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 made the change - replaced the enum with a string.

Copy link
Contributor

Choose a reason for hiding this comment

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

The second part was unwinding the one of and leaving the channel field as is, just adding the optional qualifier - see above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's technically a breaking change, but I think the functional impact is less than if we left it deprecated and had a second way of referencing a channel.

Oh, my bad. I had incorrectly read this comment - somehow I missed the word "than" and it changed the meaning. Removed the oneof too.

@@ -28,7 +29,14 @@ public void validateUpdateNotificationRuleRequest(

private void validateNotificationRuleMutableData(NotificationRuleMutableData data) {
validateNonDefaultPresenceOrThrow(data, NotificationRuleMutableData.RULE_NAME_FIELD_NUMBER);
validateNonDefaultPresenceOrThrow(data, NotificationRuleMutableData.CHANNEL_ID_FIELD_NUMBER);
if (data.hasIntegrationTarget()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If you don't want channel ID set with this, can also validate it's unset

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -18,4 +18,11 @@ message NotificationRuleMutableData {

string channel_id = 5;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
string channel_id = 5;
optional string channel_id = 5;

@aaron-steinfeld aaron-steinfeld dismissed their stale review April 26, 2024 15:55

accidental approve

@aaron-steinfeld
Copy link
Contributor

Just want to confirm we're all on the same page here, will override merge once @singhalprerana approves

}

message NotificationIntegrationTarget {
string type = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not fully onboard with this - still feel enum would be better for a single source of truth. But given it's a small nuance in the scheme of things and it works for our use-case, so works for me.

@singhalprerana
Copy link
Contributor

@aaron-steinfeld approved - works for the current use-case. Plz go ahead and merge.

@aaron-steinfeld aaron-steinfeld merged commit ef9a8f2 into main Apr 26, 2024
8 of 9 checks passed
@aaron-steinfeld aaron-steinfeld deleted the ENG-43011 branch April 26, 2024 18:24
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