-
Notifications
You must be signed in to change notification settings - Fork 5
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
Conversation
...ice-api/src/main/proto/org/hypertrace/notification/config/service/v1/notification_rule.proto
Outdated
Show resolved
Hide resolved
@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 |
string rate_limit_interval_duration = 6; | ||
|
||
oneof notification_target { |
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.
@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..
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.
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?
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.
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.
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.
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).
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.
@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..
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.
^^^ works for me.. missed the part that we can add optional to string without breaking API.. @DibyojyotiS let's go with this..
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.
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.
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.
I made the change - replaced the enum with a string.
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.
The second part was unwinding the one of and leaving the channel field as is, just adding the optional qualifier - see above
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.
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()) { |
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.
If you don't want channel ID set with this, can also validate it's unset
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.
done
@@ -18,4 +18,11 @@ message NotificationRuleMutableData { | |||
|
|||
string channel_id = 5; |
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.
string channel_id = 5; | |
optional string channel_id = 5; |
Just want to confirm we're all on the same page here, will override merge once @singhalprerana approves |
} | ||
|
||
message NotificationIntegrationTarget { | ||
string type = 1; |
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.
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.
@aaron-steinfeld approved - works for the current use-case. Plz go ahead and merge. |
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:
Documentation
Make sure that you have documented corresponding changes in this repository or hypertrace docs repo if required.