feat: Implement custom trigger for webhooks #9879

gt2345 · 2024-08-29T15:38:30Z

Ticket

Description

Part of workload alerting project.
ERD link: https://hpe-aiatscale.atlassian.net/wiki/spaces/ENGINEERIN/pages/1666809868/Workload+Alerting+ERD

Implement trigger type CUSTOM for webhook.

Test Plan

Create a webhook with custom trigger, note that for custom trigger, the mode of webhook can only be Specific.
Within the same workspace as the webhook, create an experiment with config including

integrations:
  webhooks:
    webhook_name:
      - the_name_of_custom_webhook

In the experiment code, trigger the webhook as

with det.core.init() as core_context:
    core_context.alert(title="some content", level="debug")

Verify that the webhook is triggered.

Checklist

Changes have been manually QA'd
New features have been approved by the corresponding PM
User-facing API changes have the "User-facing API Change" label
Release notes have been added as a separate file under docs/release-notes/
See Release Note for details.
Licenses have been included for new code which was copied and/or modified from any external code

codecov · 2024-08-29T15:38:47Z

Codecov Report

Attention: Patch coverage is 3.34928% with 202 lines in your changes missing coverage. Please review.

Project coverage is 54.61%. Comparing base (036477b) to head (f72241e).
Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
master/internal/webhooks/postgres_webhook.go	4.23%	113 Missing ⚠️
webui/react/src/components/WebhookCreateModal.tsx	0.00%	43 Missing ⚠️
master/internal/webhooks/api_webhook.go	0.00%	21 Missing ⚠️
harness/determined/core/_context.py	12.50%	14 Missing ⚠️
webui/react/src/pages/WebhookList.tsx	0.00%	7 Missing ⚠️
master/internal/webhooks/webhook.go	0.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9879      +/-   ##
==========================================
- Coverage   54.67%   54.61%   -0.07%     
==========================================
  Files        1263     1263              
  Lines      156862   157047     +185     
  Branches     3597     3598       +1     
==========================================
+ Hits        85768    85769       +1     
- Misses      70963    71147     +184     
  Partials      131      131

Flag	Coverage Δ
backend	`45.08% <3.49%> (-0.12%)`	⬇️
harness	`72.58% <12.50%> (-0.04%)`	⬇️
web	`54.40% <0.00%> (-0.03%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
master/internal/webhooks/webhook.go	`12.00% <0.00%> (-0.50%)`	⬇️
webui/react/src/pages/WebhookList.tsx	`0.00% <0.00%> (ø)`
harness/determined/core/_context.py	`53.04% <12.50%> (-4.67%)`	⬇️
master/internal/webhooks/api_webhook.go	`0.00% <0.00%> (ø)`
webui/react/src/components/WebhookCreateModal.tsx	`0.00% <0.00%> (ø)`
master/internal/webhooks/postgres_webhook.go	`48.70% <4.23%> (-11.94%)`	⬇️

... and 4 files with indirect coverage changes

netlify · 2024-08-29T15:38:48Z

✅ Deploy Preview for determined-ui canceled.

Name	Link
🔨 Latest commit	`f72241e`
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/66db3c5625106600081cb6a8

johnkim-det

some suggestions but otherwise web LGTM. would be nice to have unit testing but won't block on it.

johnkim-det · 2024-09-04T17:04:48Z

webui/react/src/components/WebhookCreateModal.tsx

+  useEffect(() => {
+    if ((triggers || []).includes(V1TriggerType.CUSTOM)) {
+      form.setFieldValue('mode', V1WebhookMode.SPECIFIC);
+    }
+  }, [triggers, form]);
+


nit: consolidate with existing useEffect and/or move to onChange?

johnkim-det · 2024-09-04T17:05:28Z

webui/react/src/components/WebhookCreateModal.tsx

+            name="mode"
+            rules={[{ message: 'Webhook mode is required', required: true }]}>
+            <Select
+              disabled={(triggers || []).includes(V1TriggerType.CUSTOM)}


nit: don't think || [] throughout the file is necessary?

johnkim-det · 2024-09-04T17:10:41Z

webui/react/src/pages/WebhookList.tsx

+        if (t.triggerType === V1TriggerType.CUSTOM) {
+          return (
+            <li className={css.listBadge} key={t.id}>
+              <Badge>CUSTOM</Badge>


nit: could use labels from triggerOptions in WebhookCreateModal?

amandavialva01 · 2024-09-04T18:29:41Z

proto/src/determined/webhook/v1/webhook.proto

+
+// Event data for custom trigger.
+message CustomWebhookEventData {
+  // The level for the event data.


Suggested change

// The level for the event data.

// The level at which the event data is logged.

amandavialva01 · 2024-09-04T18:29:55Z

master/internal/webhooks/api_webhook.go

+		if t.TriggerType == webhookv1.TriggerType_TRIGGER_TYPE_CUSTOM {
+			if req.Webhook.Mode != webhookv1.WebhookMode_WEBHOOK_MODE_SPECIFIC {
+				return nil, status.Errorf(codes.InvalidArgument,
+					"custom trigger only works on webhook with mode 'SPECIFIC' got %v",


Suggested change

"custom trigger only works on webhook with mode 'SPECIFIC' got %v",

"custom trigger only works on webhook with mode 'SPECIFIC'. Got %v",

amandavialva01 · 2024-09-04T18:38:11Z

master/internal/webhooks/postgres_webhook.go

+type CustomTriggerData struct {
+	Title       string `json:"title"`
+	Description string `json:"description"`
+	Level       string `json:"level"`


nit: Should we keep the CustomWebhookEventData fields in the same order as this CustomTriggerData?
Just so it's clear what the most important field(s) are (which is presumably the Title field)

amandavialva01 · 2024-09-04T18:39:23Z

proto/src/determined/webhook/v1/webhook.proto

+// Event data for custom trigger.
+message CustomWebhookEventData {
+  // The level for the event data.
+  determined.log.v1.LogLevel level = 1;


Should we make this log_level?

Suggested change

determined.log.v1.LogLevel level = 1;

determined.log.v1.LogLevel log_level = 1;

master/internal/webhooks/postgres_webhook.go

amandavialva01 · 2024-09-04T18:51:34Z

master/internal/webhooks/postgres_webhook.go

+
+	activeConfig, err := expconf.ParseAnyExperimentConfigYAML(m.ConfigBytes)
+	if err != nil {
+		return fmt.Errorf("error parsing experiment config : %w", err)


Suggested change

return fmt.Errorf("error parsing experiment config : %w", err)

return fmt.Errorf("error parsing experiment config: %w", err)

master/static/migrations/20240828101021_custom-webhook-trigger.tx.up.sql

master/internal/webhooks/postgres_webhook.go

amandavialva01 · 2024-09-04T20:29:34Z

master/internal/webhooks/api_webhook.go

@@ -223,3 +230,29 @@ func (a *WebhooksAPIServer) TestWebhook(
 	}
 	return &apiv1.TestWebhookResponse{}, nil
 }
+
+// PostWebhookEventData handles data for custom trigger.
+func (a *WebhooksAPIServer) PostWebhookEventData(


Can we maybe add a couple of integration tests for this function?

e2e tests for this function were accidentally dropped during a merge, now they should be restored

master/internal/webhooks/postgres_webhook.go

jgongd · 2024-09-05T20:41:01Z

harness/determined/core/_context.py

@@ -359,6 +390,7 @@ def init(
        _metrics=metrics,
        _tensorboard_manager=tensorboard_manager,
        _session=session,
+        info=info,


Just for my understanding, why do we need info now?

Because we want to access the experiment id and trial id in alert function

amandavialva01 · 2024-09-05T21:33:08Z

master/internal/webhooks/postgres_webhook.go

+			err = generateEventForCustomTrigger(
+				ctx, &es, webhook.Triggers, webhook.WebhookType, webhook.URL, m.Experiment, activeConfig, data, trialID)
+			if err != nil {
+				return fmt.Errorf("error genrating event %d %+v: %w", webhookID, webhook, err)


Suggested change

return fmt.Errorf("error genrating event %d %+v: %w", webhookID, webhook, err)

return fmt.Errorf("error genrating event for webhook with ID %d %+v: %w", webhookID, webhook, err)

jgongd · 2024-09-05T21:58:03Z

master/internal/webhooks/postgres_webhook.go

+	}
+
+	var es []Event
+	if webhookConfig.WebhookID != nil {


(not a blocker)From the user perspective, since they can provide a list of webhook ids, where can a user find out the id of a webhook?

Since webhook is not part of the CLI, user can only access the webhook ids from REST api. This is alright to me because we want to encourage user to use webhook name for readability, and using id is more like an add-on.

jgongd · 2024-09-05T22:19:57Z

master/internal/webhooks/postgres_webhook.go

+	}
+
+	if _, err := db.Bun().NewInsert().Model(&es).Exec(ctx); err != nil {
+		return fmt.Errorf("report experiment state changed inserting event trigger: %w", err)


would the error message be something like handle custom trigger data inserting events?

jgongd · 2024-09-06T01:42:00Z

master/internal/webhooks/postgres_webhook.go

+		Where("name = ?", webhookName).
+		Where("workspace_id = ?", workspaceID).


Nice, this makes sure only one webhook is selected if there are multiple webhooks with the same names but different url.

jgongd · 2024-09-06T02:03:44Z

master/internal/webhooks/postgres_webhook.go

+		mStatus = string(e.State)
+	}
+
+	endTime := time.Now()


Why we need to set endTime to time.Now()?

I could be wrong but it seems like endTime is only used here to set the experiment duration in expBlockFields, so if it hasn't completed yet (endTime == nil), then we can get the duration from start time up until the present!

Ah thanks! That calculates the experiment duration for a Slack message.

Exactly, thanks @amandavialva01

jgongd

LGTM

amandavialva01

LGTM! Awesome work with this 😆

gt2345 added 3 commits August 28, 2024 17:32

add custom trigger type

3656619

e2e test

5ec4d27

Merge branch 'main' into gt/480-custom-trigger

286bf9b

cla-bot bot added the cla-signed label Aug 29, 2024

gt2345 added 9 commits August 29, 2024 11:01

fixes

3794fcc

lint

43e5b10

remove workspace after test

42df954

remove workspace

cc17f20

remove webhook after test

224ccea

fix

f4399b4

fix

e4a581b

lint

fef27b6

lint

807536b

gt2345 marked this pull request as ready for review September 3, 2024 15:53

gt2345 requested review from a team as code owners September 3, 2024 15:53

gt2345 requested review from johnkim-det, amandavialva01 and jgongd September 3, 2024 15:53

johnkim-det approved these changes Sep 4, 2024

View reviewed changes

gt2345 added 4 commits September 4, 2024 13:09

fixes

bd52183

Merge branch 'main' into gt/480-custom-trigger

5717149

restore test

0b2020c

fmt

b2e8f0b

amandavialva01 reviewed Sep 4, 2024

View reviewed changes

master/internal/webhooks/postgres_webhook.go Show resolved Hide resolved

amandavialva01 reviewed Sep 4, 2024

View reviewed changes

master/static/migrations/20240828101021_custom-webhook-trigger.tx.up.sql Show resolved Hide resolved

amandavialva01 reviewed Sep 4, 2024

View reviewed changes

master/internal/webhooks/postgres_webhook.go Show resolved Hide resolved

gt2345 added 2 commits September 4, 2024 15:06

fixes

1f973bc

Merge branch 'main' into gt/480-custom-trigger

bf74df2

gt2345 requested a review from amandavialva01 September 4, 2024 20:28

amandavialva01 reviewed Sep 4, 2024

View reviewed changes

master/internal/webhooks/postgres_webhook.go Show resolved Hide resolved

gt2345 requested a review from amandavialva01 September 5, 2024 01:15

jgongd reviewed Sep 5, 2024

View reviewed changes

amandavialva01 reviewed Sep 5, 2024

View reviewed changes

jgongd reviewed Sep 5, 2024

View reviewed changes

fixes

e6b3a95

jgongd reviewed Sep 5, 2024

View reviewed changes

jgongd reviewed Sep 6, 2024

View reviewed changes

fix

f334a55

jgongd reviewed Sep 6, 2024

View reviewed changes

jgongd approved these changes Sep 6, 2024

View reviewed changes

amandavialva01 approved these changes Sep 6, 2024

View reviewed changes

skip wrong webhook id

f72241e

gt2345 merged commit bfeb418 into main Sep 9, 2024
83 of 97 checks passed

gt2345 deleted the gt/480-custom-trigger branch September 9, 2024 01:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Implement custom trigger for webhooks #9879

feat: Implement custom trigger for webhooks #9879

gt2345 commented Aug 29, 2024 •

edited by jira bot

Loading

codecov bot commented Aug 29, 2024 •

edited

Loading

netlify bot commented Aug 29, 2024 •

edited

Loading

johnkim-det left a comment

johnkim-det Sep 4, 2024

johnkim-det Sep 4, 2024

johnkim-det Sep 4, 2024

amandavialva01 Sep 4, 2024

amandavialva01 Sep 4, 2024

amandavialva01 Sep 4, 2024

amandavialva01 Sep 4, 2024

amandavialva01 Sep 4, 2024

amandavialva01 Sep 4, 2024

gt2345 Sep 4, 2024 •

edited

Loading

jgongd Sep 5, 2024 •

edited

Loading

gt2345 Sep 5, 2024

amandavialva01 Sep 5, 2024

jgongd Sep 5, 2024 •

edited

Loading

gt2345 Sep 5, 2024

jgongd Sep 5, 2024

jgongd Sep 6, 2024

jgongd Sep 6, 2024

amandavialva01 Sep 6, 2024

jgongd Sep 6, 2024

gt2345 Sep 6, 2024

jgongd left a comment

amandavialva01 left a comment

	// The level for the event data.
	// The level at which the event data is logged.

	"custom trigger only works on webhook with mode 'SPECIFIC' got %v",
	"custom trigger only works on webhook with mode 'SPECIFIC'. Got %v",

	determined.log.v1.LogLevel level = 1;
	determined.log.v1.LogLevel log_level = 1;

	return fmt.Errorf("error parsing experiment config : %w", err)
	return fmt.Errorf("error parsing experiment config: %w", err)

	return fmt.Errorf("error genrating event %d %+v: %w", webhookID, webhook, err)
	return fmt.Errorf("error genrating event for webhook with ID %d %+v: %w", webhookID, webhook, err)

		Where("name = ?", webhookName).
		Where("workspace_id = ?", workspaceID).

feat: Implement custom trigger for webhooks #9879

feat: Implement custom trigger for webhooks #9879

Conversation

gt2345 commented Aug 29, 2024 • edited by jira bot Loading

Ticket

Description

Test Plan

Checklist

codecov bot commented Aug 29, 2024 • edited Loading

Codecov Report

netlify bot commented Aug 29, 2024 • edited Loading

✅ Deploy Preview for determined-ui canceled.

johnkim-det left a 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

Choose a reason for hiding this comment

gt2345 Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

jgongd Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgongd Sep 5, 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

jgongd left a comment

Choose a reason for hiding this comment

amandavialva01 left a comment

Choose a reason for hiding this comment

gt2345 commented Aug 29, 2024 •

edited by jira bot

Loading

codecov bot commented Aug 29, 2024 •

edited

Loading

netlify bot commented Aug 29, 2024 •

edited

Loading

gt2345 Sep 4, 2024 •

edited

Loading

jgongd Sep 5, 2024 •

edited

Loading

jgongd Sep 5, 2024 •

edited

Loading