Feat: Add a generic webhook for sending event notifications #879

brian-lou · 2024-10-16T22:46:00Z

This PR adds a generic webhook: /event-notifier/v1 which can be used to send events and messages to Slack and DD (with support for Jira coming soon).

codecov · 2024-10-16T22:52:17Z

Codecov Report

Attention: Patch coverage is 91.80328% with 5 lines in your changes missing coverage. Please review.

Project coverage is 89.75%. Comparing base (9aaa714) to head (bc4ed32).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
src/webhooks/generic-notifier/generic-notifier.ts	90.19%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #879      +/-   ##
==========================================
+ Coverage   89.43%   89.75%   +0.32%     
==========================================
  Files         121      124       +3     
  Lines        3701     3769      +68     
  Branches      741      753      +12     
==========================================
+ Hits         3310     3383      +73     
+ Misses        379      374       -5     
  Partials       12       12

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

src/buildServer.ts

fpacifici

Some comments in line

fpacifici · 2024-11-05T23:35:09Z

src/types/index.ts

+export type GenericEvent = {
+  source: string;
+  timestamp: number;
+  service_name?: string; // Official service registry name if applicable


What are you going to use this for ? If I remember correctly the service registry was supposed to be used to route messages to the right channel, but the channel is also passed as a parameter.

Yes, the idea was to use the service-registry if the service_name is supplied, but I'm not sure if everything that will use this in the future will be associated with a service. If we can make that guarantee, I can remove the channel parameter

If we can make that guarantee, I can remove the channel parameter
we cannot make this guarantee.

So the problem is the same as below. Please let's not have an api method with a lot of optional fields at the same level and rely on the user to know which combination are valid and which ones are not.

Instead, I would suggest one of these two so that making mistakes is harder:

Have more granular methods: one for slack with smart routing that goes to service and one where we specify the channel. Though we need to think through which methods we are going to build upfront.

Have one method but provide different structured types for the different types of notification: slack with direct routing, slack with service routing (service is relevant only for slack), jira, datadog, etc.

fpacifici · 2024-11-05T23:44:26Z

src/types/index.ts

+    channels: {
+      slack?: string[]; // list of Slack Channels
+      datadog?: string[]; // list of DD Monitors
+      jira?: string[]; // list of Jira Projects
+      bigquery?: string;
+    };
+    tags?: string[]; // Not used for Slack
+    misc: {
+      alertType?: EventAlertType; // Datadog alert type
+      blocks?: (KnownBlock | Block)[]; // Optional Slack blocks


IT has been quite some time since when we discussed this design.
I cannot remember anymore whether the generic event was supposed to know where to route the message or whether it was supposed to be the simple api the user would use to route a message to a specific channel.

If it is the first (the user sends the message and there is logic in eng-pipes that figures out where to route the message) then it should not be a generic event and it should not contain channel specific parameters (like tags and the list of channels)

If it is the simple one (the user picks the channel), then I don't think putting all the channel fields in the same message and api is a good design, you should have dedicated apis per channel. It would be quite hard to understand which fields are supposed to be populated in which scenario.

If there are good reaons to have one api (still in the simple option where eng-pipes does not provide any routing loigic) and if you wanted to support multiple channels in one api call then a better api design would be to require a list of objects, each object containing all the details needed for a specific channel:

channel[] where channel is the union of Slack: { channel: string blocks } Datadog { monitor: string tags: string[] } ...

If there are common fields they can be specified only once.

I think the idea was to have a combination of both: a generic, simple api which a user can use to route a message to a specific channel, and a more complex webhook which has logic to route more complicated events.

I agree that the typing for the generic event payload can be improved, will work on that

Defining the public api of a service is a task that should be thought through carefully as changing it afterwards when clients are using is hard.
So feel free to take the time to think the details of all the apis method you expect to create and send a PR with only those types and the empty business logic so we can review if the whole api is coherent and meaningful.

fpacifici · 2024-11-05T23:53:58Z

src/webhooks/generic-notifier/generic-notifier.ts

+    await messageSlack(body);
+    await sendEventToDatadog(body, moment().unix());


Where are the other channels ?

The only other one was Jira, but never really got around to getting that added since Alex was (i think?) working a little on the env setup, so I was originally waiting for him to get that done first.

fpacifici · 2024-11-06T00:00:32Z

src/webhooks/generic-notifier/generic-notifier.ts

+  message: GenericEvent,
+  timestamp: number
+) {
+  if (message.data.channels.datadog) {


I'd like a pattern where you decide which methods to call in the genericEventNotifier function. That is the orchestrating function, which parses the message and trigger the relevant channels. From their name sendEventToDatadog and messageSlack should always send the message, not decide when not to send.

fpacifici · 2024-11-13T18:43:13Z

Dockerfile

+RUN mkdir -p ~/.ssh && ssh-keyscan github.com >> ~/.ssh/known_hosts
+
+# Install production dependencies. & update service-registry to main
+RUN --mount=type=ssh yarn install --immutable && yarn up "service-registry@git+ssh://[email protected]:getsentry/service-registry#main"


I don't think you can do this.
service-registry is private, this is still a public repo.
I'd suggest you skip this part for now.

I think you will have to provide the service registry as a config file when you deploy the system, not as a package in the image.

package.json

fpacifici · 2024-11-13T19:00:01Z

src/types/index.ts

+export interface SlackMessage {
+  type: 'slack';
+  channels: string[];
+  text: string;
+  blocks?: KnownBlock[] | Block[];
+}
+
+export interface DatadogEvent {
+  type: 'datadog';
+  title: string;
+  text: string;
+  tags: string[];
+  alertType: EventAlertType;
+}
+
+export interface JiraEvent {
+  type: 'jira';
+  projectId: string;
+  title: string;
+}
+
+export type GenericEvent = {
+  source: string;
+  timestamp: number;
+  data: (DatadogEvent | JiraEvent | SlackMessage | ServiceSlackMessage)[];
+};


Ok, this is better.

fpacifici · 2024-11-13T19:03:59Z

src/types/index.ts

+// Currently only used for Slack notifications since
+// service registry only contains Slack channels (and not DD or Jira or others)
+export interface ServiceSlackMessage {
+  type: 'service_notification';
+  service_name: string; // Official service registry service id
+  text: string;
+  blocks?: KnownBlock[] | Block[];
+}


This seems the same as the slack message, except that you can provide the channel via a service name.
Doing it this way would mean we would duplicate all the methods that can take a destination from the service registry.

Why not abstracting the concept of SlackChannel, and pass it here https://github.com/getsentry/eng-pipes/pull/879/files#diff-a9ba9cbedd19c9f66d564d2e89912890209b98f0a7ef19187877d2587300e476R34 .

You can create two implementations: SlackChannel and ServiceSlackChannel. One contains the channel name and the other contains the service name.

src/webhooks/README.md

brian-lou added 9 commits October 16, 2024 10:14

Add txn logging

b15d743

Delete unused index.ts files

20b59c3

Fix tests

575eb6f

Add http status to txn

e38691d

Deprecate old Slack handler

3627224

Add generic-notifier

53802ac

Fix tests

6f14385

Add blocks to generic notifier

2d50f63

Update readme

67ff8c2

brian-lou requested a review from IanWoodard October 16, 2024 22:46

Merge branch 'main' into brian/add-generic-webhook

780a143

brian-lou added 2 commits October 16, 2024 15:57

Add 400 error for invalid source field

2628a1a

Update readme

ad0745b

brian-lou changed the title ~~Add a generic webhook for sending event notifications~~ Feat: Add a generic webhook for sending event notifications Oct 17, 2024

fpacifici reviewed Nov 5, 2024

View reviewed changes

src/buildServer.ts Show resolved Hide resolved

fpacifici reviewed Nov 6, 2024

View reviewed changes

fpacifici reviewed Nov 13, 2024

View reviewed changes

Change generic webhook API

c61f87c

brian-lou force-pushed the brian/add-generic-webhook branch from b4961f7 to c61f87c Compare November 13, 2024 19:46

brian-lou added 6 commits November 13, 2024 13:06

Add example service-registry config & change types

3915a01

Fix tests

30cb490

Del unused ssh-agent stuff

7a308c7

Revert ci.yml changes

85ac0f9

Revert migration.yml changes

7ff4b83

Add TODO

bc4ed32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: Add a generic webhook for sending event notifications #879

Feat: Add a generic webhook for sending event notifications #879

brian-lou commented Oct 16, 2024

codecov bot commented Oct 16, 2024 •

edited

Loading

fpacifici left a comment

fpacifici Nov 5, 2024

brian-lou Nov 6, 2024

fpacifici Nov 7, 2024 •

edited

Loading

fpacifici Nov 5, 2024

brian-lou Nov 6, 2024

fpacifici Nov 7, 2024

fpacifici Nov 5, 2024

brian-lou Nov 6, 2024

fpacifici Nov 6, 2024

fpacifici Nov 13, 2024

fpacifici Nov 13, 2024

fpacifici Nov 13, 2024

		await messageSlack(body);
		await sendEventToDatadog(body, moment().unix());

Feat: Add a generic webhook for sending event notifications #879

Are you sure you want to change the base?

Feat: Add a generic webhook for sending event notifications #879

Conversation

brian-lou commented Oct 16, 2024

codecov bot commented Oct 16, 2024 • edited Loading

Codecov Report

fpacifici left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fpacifici Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 16, 2024 •

edited

Loading

fpacifici Nov 7, 2024 •

edited

Loading