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.

Sign up for GitHub

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

Changes in Notification rule to allow types #209

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
be34947
ENG-43011: Changes in Notification rule to allow types.
DibyojyotiS Apr 23, 2024
5b0867b
change protos
DibyojyotiS Apr 26, 2024
75873e4
spotlessApply
DibyojyotiS Apr 26, 2024
c7fd3c8
replace the integration type enum with a string
DibyojyotiS Apr 26, 2024
ab9049d
remove oneof
DibyojyotiS Apr 26, 2024
5264402
make channel id optional
DibyojyotiS Apr 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion ...-api/src/main/proto/org/hypertrace/notification/config/service/v1/notification_rule.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,25 @@ message NotificationRuleMutableData {
string event_condition_id = 3;
string event_condition_type = 4;

string channel_id = 5;
string channel_id = 5 [deprecated=true];
singhalprerana marked this conversation as resolved.
Show resolved Hide resolved
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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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.

NotificationChannelTarget channel_target = 7;
NotificationIntegrationTarget integration_target = 8;
}
}

message NotificationChannelTarget {
string channel_id = 1;
}

message NotificationIntegrationTarget {
IntegrationType type = 1;
string integration_id = 2;
}

enum IntegrationType {
INTEGRATION_TYPE_UNSPECIFIED = 0;
INTEGRATION_TYPE_WIZ = 1;
}
50 changes: 39 additions & 11 deletions ...in/java/org/hypertrace/notification/config/service/NotificationRuleConfigServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import org.hypertrace.notification.config.service.v1.GetAllNotificationRulesResponse;
import org.hypertrace.notification.config.service.v1.GetNotificationRuleRequest;
import org.hypertrace.notification.config.service.v1.GetNotificationRuleResponse;
import org.hypertrace.notification.config.service.v1.NotificationChannelTarget;
import org.hypertrace.notification.config.service.v1.NotificationRule;
import org.hypertrace.notification.config.service.v1.NotificationRuleConfigServiceGrpc;
import org.hypertrace.notification.config.service.v1.UpdateNotificationRuleRequest;
Expand All @@ -42,15 +43,18 @@ public void createNotificationRule(
try {
RequestContext requestContext = RequestContext.CURRENT.get();
validator.validateCreateNotificationRuleRequest(requestContext, request);
NotificationRule.Builder builder =
NotificationRule notificationRule =
NotificationRule.newBuilder()
.setId(UUID.randomUUID().toString())
.setNotificationRuleMutableData(request.getNotificationRuleMutableData());
.setNotificationRuleMutableData(request.getNotificationRuleMutableData())
.build();

responseObserver.onNext(
CreateNotificationRuleResponse.newBuilder()
.setNotificationRule(
notificationRuleStore.upsertObject(requestContext, builder.build()).getData())
notificationRuleStore
.upsertObject(requestContext, makeBackwardCompatible(notificationRule))
.getData())
.build());
responseObserver.onCompleted();
} catch (Exception e) {
Expand All @@ -66,17 +70,16 @@ public void updateNotificationRule(
try {
RequestContext requestContext = RequestContext.CURRENT.get();
validator.validateUpdateNotificationRuleRequest(requestContext, request);
NotificationRule notificationRule =
NotificationRule.newBuilder()
.setId(request.getId())
.setNotificationRuleMutableData(request.getNotificationRuleMutableData())
.build();
responseObserver.onNext(
UpdateNotificationRuleResponse.newBuilder()
.setNotificationRule(
notificationRuleStore
.upsertObject(
requestContext,
NotificationRule.newBuilder()
.setId(request.getId())
.setNotificationRuleMutableData(
request.getNotificationRuleMutableData())
.build())
.upsertObject(requestContext, makeBackwardCompatible(notificationRule))
.getData())
.build());
responseObserver.onCompleted();
Expand All @@ -98,6 +101,7 @@ public void getAllNotificationRules(
.addAllNotificationRules(
notificationRuleStore.getAllObjects(requestContext).stream()
.map(ConfigObject::getData)
.map(this::makeBackwardCompatible)
.collect(Collectors.toUnmodifiableList()))
.build());
responseObserver.onCompleted();
Expand Down Expand Up @@ -137,11 +141,35 @@ public void getNotificationRule(
.getData(requestContext, request.getNotificationRuleId())
.orElseThrow(Status.NOT_FOUND::asRuntimeException);
responseObserver.onNext(
GetNotificationRuleResponse.newBuilder().setNotificationRule(notificationRule).build());
GetNotificationRuleResponse.newBuilder()
.setNotificationRule(makeBackwardCompatible(notificationRule))
.build());
responseObserver.onCompleted();
} catch (Exception e) {
log.error("Get Notification Rule by id RPC failed for request:{}", request, e);
responseObserver.onError(e);
}
}

private NotificationRule makeBackwardCompatible(NotificationRule rule) {
if (rule.getNotificationRuleMutableData().hasChannelTarget()) {
// makes the rules backward compatible
return rule.toBuilder()
.setNotificationRuleMutableData(
rule.getNotificationRuleMutableData().toBuilder()
.setChannelId(
rule.getNotificationRuleMutableData().getChannelTarget().getChannelId()))
.build();
} else if (!rule.getNotificationRuleMutableData().getChannelId().isEmpty()) {
// makes old notification rules compatible with new logic
return rule.toBuilder()
.setNotificationRuleMutableData(
rule.getNotificationRuleMutableData().toBuilder()
.setChannelTarget(
NotificationChannelTarget.newBuilder()
.setChannelId(rule.getNotificationRuleMutableData().getChannelId())))
.build();
}
return rule;
}
}
20 changes: 19 additions & 1 deletion ...hypertrace/notification/config/service/NotificationRuleConfigServiceRequestValidator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
import org.hypertrace.notification.config.service.v1.DeleteNotificationRuleRequest;
import org.hypertrace.notification.config.service.v1.GetAllNotificationRulesRequest;
import org.hypertrace.notification.config.service.v1.GetNotificationRuleRequest;
import org.hypertrace.notification.config.service.v1.NotificationChannelTarget;
import org.hypertrace.notification.config.service.v1.NotificationIntegrationTarget;
import org.hypertrace.notification.config.service.v1.NotificationRuleMutableData;
import org.hypertrace.notification.config.service.v1.UpdateNotificationRuleRequest;

Expand All @@ -28,7 +30,23 @@ public void validateUpdateNotificationRuleRequest(

private void validateNotificationRuleMutableData(NotificationRuleMutableData data) {
validateNonDefaultPresenceOrThrow(data, NotificationRuleMutableData.RULE_NAME_FIELD_NUMBER);
validateNonDefaultPresenceOrThrow(data, NotificationRuleMutableData.CHANNEL_ID_FIELD_NUMBER);
switch (data.getNotificationTargetCase()) {
case CHANNEL_TARGET:
validateNonDefaultPresenceOrThrow(
data.getChannelTarget(), NotificationChannelTarget.CHANNEL_ID_FIELD_NUMBER);
break;
case INTEGRATION_TARGET:
validateNonDefaultPresenceOrThrow(
data.getIntegrationTarget(), NotificationIntegrationTarget.INTEGRATION_ID_FIELD_NUMBER);
validateNonDefaultPresenceOrThrow(
data.getIntegrationTarget(), NotificationIntegrationTarget.TYPE_FIELD_NUMBER);
break;
case NOTIFICATIONTARGET_NOT_SET:
default:
// backward compatibility
validateNonDefaultPresenceOrThrow(
data, NotificationRuleMutableData.CHANNEL_ID_FIELD_NUMBER);
}
validateNonDefaultPresenceOrThrow(
data, NotificationRuleMutableData.EVENT_CONDITION_ID_FIELD_NUMBER);
validateNonDefaultPresenceOrThrow(
Expand Down
8 changes: 7 additions & 1 deletion ...ava/org/hypertrace/notification/config/service/NotificationRuleConfigServiceImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import org.hypertrace.notification.config.service.v1.DeleteNotificationRuleRequest;
import org.hypertrace.notification.config.service.v1.GetAllNotificationRulesRequest;
import org.hypertrace.notification.config.service.v1.GetNotificationRuleRequest;
import org.hypertrace.notification.config.service.v1.NotificationChannelTarget;
import org.hypertrace.notification.config.service.v1.NotificationRule;
import org.hypertrace.notification.config.service.v1.NotificationRuleConfigServiceGrpc;
import org.hypertrace.notification.config.service.v1.NotificationRuleMutableData;
Expand Down Expand Up @@ -44,11 +45,15 @@ void createReadUpdateDeleteNotificationRules() {
getNotificationRuleMutableData("rule1", "channel1");
NotificationRuleMutableData notificationRuleMutableData2 =
getNotificationRuleMutableData("rule2", "channel1");

// should return a compatible rule with notification target populated
NotificationRuleMutableData oldNotificationRuleMutableData =
notificationRuleMutableData1.toBuilder().clearChannelTarget().build();
NotificationRule notificationRule1 =
notificationStub
.createNotificationRule(
CreateNotificationRuleRequest.newBuilder()
.setNotificationRuleMutableData(notificationRuleMutableData1)
.setNotificationRuleMutableData(oldNotificationRuleMutableData)
.build())
.getNotificationRule();
assertEquals(
Expand Down Expand Up @@ -127,6 +132,7 @@ private NotificationRuleMutableData getNotificationRuleMutableData(
.setChannelId(channelId)
.setEventConditionType("metricAnomalyEventCondition")
.setEventConditionId("rule-1")
.setChannelTarget(NotificationChannelTarget.newBuilder().setChannelId(channelId))
.build();
}

Expand Down
Loading