webhooks: allow to update webhook status & response #1185

gioelecerati · 2022-08-05T16:35:07Z

What does this pull request do? Explain your changes. (required)

Not in the weekly planning, but needed while debugging token gating:
All the webhooks success & failures are currently updated by Studio, apart of the webhook playback.user.new (token gating) which is handled on mapic. This new api allows admins (& mapic) to update the status of a webhook and store the responses, allowing us to:

Better debug problems with token gating
Update the dashboard correctly
Inform the user on the status of the webhook

This needs some changes on mapic to store this information.

Specific updates (required)

Added /webhook/:id/status POST endpoint (similar to the task status update endpoint used by task runner)
Created new payload check in the schema for the response & status of a webhook

How did you test each of these updates (required)

yarn test

Does this pull request close any open issues?
FIxes #1171

Screenshots (optional):

Checklist:

I have read the CONTRIBUTING document.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.

vercel · 2022-08-05T16:35:09Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
livepeer-studio	✅ Ready (Inspect)	Visit Preview	Aug 10, 2022 at 4:22PM (UTC)

codecov · 2022-08-05T16:47:28Z

Codecov Report

Merging #1185 (1cd465c) into master (0f808b4) will decrease coverage by 0.00332%.
The diff coverage is 47.61905%.

@@                 Coverage Diff                 @@
##              master       #1185         +/-   ##
===================================================
- Coverage   50.70655%   50.70323%   -0.00333%     
===================================================
  Files             66          66                 
  Lines           4246        4266         +20     
  Branches         748         755          +7     
===================================================
+ Hits            2153        2163         +10     
- Misses          1836        1840          +4     
- Partials         257         263          +6

Impacted Files	Coverage Δ
packages/api/src/app-router.ts	`51.28205% <0.00000%> (-0.66601%)`	⬇️
packages/api/src/store/webhook-table.ts	`73.33333% <ø> (ø)`
packages/api/src/controllers/webhook.ts	`56.55172% <52.63158%> (-0.59114%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bbf21b3...1cd465c. Read the comment docs.

Impacted Files	Coverage Δ
packages/api/src/app-router.ts	`51.28205% <0.00000%> (-0.66601%)`	⬇️
packages/api/src/store/webhook-table.ts	`73.33333% <ø> (ø)`
packages/api/src/controllers/webhook.ts	`56.55172% <52.63158%> (-0.59114%)`	⬇️

victorges

Neat!!

victorges · 2022-08-05T20:27:40Z

packages/api/src/controllers/webhook.ts

+  async (req, res) => {
+    const { id } = req.params;
+    const webhook = await db.webhook.get(id);
+    if (!webhook) {


Gotta check if the webhook is deleted as well

Suggested change

if (!webhook) {

if (!webhook || webhook.deleted) {

Actually, maybe this is one of the few APIs we don't want to check that?

victorges · 2022-08-05T20:31:46Z

packages/api/src/controllers/webhook.ts

+    const doc = req.body.status;
+    const webhookResponse = req.body.response;


nit: You can grab all the fields with the proper typings! like this:

Suggested change

const doc = req.body.status;

const webhookResponse = req.body.response;

const { status, response} = req.body as WebhookStatusPayload;

You could still rename the destructured variables with sth like this const { status: doc, response: webhookResponse}, but I think the original names of the fields would be the clearest here anyway. doc kinda hides away what it is, would only use that if there's only one "payload" being handled in the function and doc is that.

Hmmmmm, actually, thinking further about this, I think the clients shouldn't specify the status of the webhook. That because the status field is actually computed from the response:
https://github.com/livepeer/livepeer-com/blob/0f808b4b87eb72942eb098e8c3e4f13ea7699a90/packages/api/src/webhooks/cannon.ts#L456

So we would end up either missing the lastFailure field which is the most important for debuggability (and that's where the dashboards shows errors from), or the client would have to specify some redundant stuff in their request, with the response (which might be big 📈) going twice. So I am thinking we could do:

Receive only the response object in the request payload

Derive the status from it instead, likely reusing some of the same object in cannon.ts (or if you copy:

const triggerTime = response.createdAt ?? Date.now() let status: DBWebhook["status"] = { lastTriggeredAt: triggerTime }; if (response.statusCode >= 300 || !response.statusCode) { status = { ...status, lastFailure: { timestamp: triggerTime, statusCode: response.statusCode, response: response.response.body, }, }; } await this.db.webhook.updateStatus(response.webhookId, status);

)

I guess this makes sense, but what about the cases where we don't get any response from the webhook? For example, when there is a connection reset or a failed DNS resolve, we actually don't have a status code and a response. Unless if the status code is missing then we consider it a failure and updated LastFailure accordingly?

Ok I missed that it's actually handled! Sounds good!

I think we also have an errorMessage field in the webhook status to account for those non-HTTP failures. We could probably receive it from the caller as well, as a separate field apart from the response (just like on the status as well afaik). WDYT?

Cool, makes sense! I am keeping also the errorMessage as a separate field. response.statusCode is a required field tho in the payload, so I either set up the statusCode as 0 when the response fails or I make it optional, wdyt?

You could have a request like this

{ errorMessage: foo, response: { status: 123, ... } }

In which status(code) is a required field of response, but response is not a required field of that payload. So you can omit the whole response if you got some different error, but if you do send a response it must have a status code. WDYT?

FTR: We decided to simplify the first version and just comment out the webhook_response table update. Will do a follow-up to start saving the responses soon.

victorges · 2022-08-05T20:34:28Z

packages/api/src/controllers/webhook.ts

+      webhookResponse.id = uuid();
+      let w: WithID<WebhookResponse> = webhookResponse;
+      await db.webhookResponse.create(w);


typescript rescue

Suggested change

webhookResponse.id = uuid();

let w: WithID<WebhookResponse> = webhookResponse;

await db.webhookResponse.create(w);

await db.webhookResponse.create({

...webhookResponse,

id: uuid()

});

and if it still complains on the typing, can change last line to

} as WithID<WebhookResponse>);

victorges · 2022-08-05T20:38:51Z

packages/api/src/controllers/webhook.test.js

+      const payload = {
+        status: {
+          lastTriggeredAt: Date.now(),
+          lastFailure: { timestamp: Date.now() },
+        },
+      };


You should try sending a response here as well to catch 1 issue that you likely haven't noticed 👀

The webhook-response object has a required eventId because that's how it works for all webhook calls right now. I think we should make that field optional instead, just so the client doesn't have to generate some random ID just to fill that field.

victorges · 2022-08-05T20:48:23Z

packages/api/src/controllers/webhook.ts

+      return res.status(422).json({ errors: ["missing status in payload"] });
+    }
+
+    await db.webhook.update(req.params.id, {


There is a dedicated updateStatus function which should be used instead. Try taking a look on the cannon.ts code that does these updates to make sure you are doing the same thing here (or if there's anything you could reuse, like exposing some of the logic there as helpers):
https://github.com/livepeer/livepeer-com/blob/0f808b4b87eb72942eb098e8c3e4f13ea7699a90/packages/api/src/webhooks/cannon.ts#L446-L503

victorges

LGTM again, but I think a few bugs would be fixed and it'd be a lot cleaner if the response field was not required in the payload of the new API. It is not always present and we shouldn't force it to be. Would naturally fix the bug of webhook responses by written even when there is no response data.

victorges · 2022-08-10T15:18:46Z

packages/api/src/controllers/webhook.ts

-    if (!webhook) {
-      return res.status(404).json({ errors: ["not found"] });
+    if (!webhook || webhook.deleted) {
+      return res.status(404).json({ errors: ["webhook not found or deleted"] });


For the user, it's just a not found!

Suggested change

return res.status(404).json({ errors: ["webhook not found or deleted"] });

return res.status(404).json({ errors: ["webhook not found"] });

I added deleted because it's never a user call but an admin one! But yeah, could remove it!

victorges · 2022-08-10T15:21:00Z

packages/api/src/controllers/webhook.ts

+    if (!response || !response.response) {
+      return res.status(422).json({ errors: ["missing response in payload"] });
+    }
+    if (!response.response.body && response.response.body !== "") {
+      return res
+        .status(400)
+        .json({ errors: ["missing body in payload response"] });
    }


The response should be an optional field since the webhook call could have gotten an error (errorMessage) but not an actual HTTP response.

Apart from that, even if we wanted to have a required field here, this logic should be handled on the schema level, by making the field required with required: [response]. There are some more "semantic" checks that can't be declared too easily in a JSON schema, but otherwise we should not be doing "json schema" checks in our code as much as possible.

victorges · 2022-08-10T15:22:25Z

packages/api/src/controllers/webhook.ts

+      if (
+        response.statusCode >= 300 ||
+        !response.statusCode ||
+        response.statusCode === 0
+      ) {


Response field should be optional since not always there will be a response, and statusCode should be obligatory (checked by JSON schema) and so we don't need to check if it's present here:

Suggested change

if (

response.statusCode >= 300 ||

!response.statusCode ||

response.statusCode === 0

) {

if (

errorMessage ||

response?.statusCode >= 300

) {

victorges · 2022-08-10T15:22:45Z

packages/api/src/controllers/webhook.ts

+            statusCode: response.statusCode,
+            response: response.response.body,


Suggested change

statusCode: response.statusCode,

response: response.response.body,

statusCode: response?.statusCode,

response: response?.response.body,

victorges · 2022-08-10T15:25:14Z

packages/api/src/controllers/webhook.ts

+    await db.webhookResponse.create({
+      id: uuid(),
+      webhookId: webhook.id,
+      createdAt: Date.now(),
+      statusCode: response.statusCode,
+      response: {
+        body: response.response.body,
+        status: response.statusCode,
+      },
+    });


Need to surround this with if (response). It will not always be present and we shouldn't be writing empty/zeroed entries on the webhook_response if we're not saving the error message in there as well (we could save the error message 👀)

victorges · 2022-08-10T15:28:04Z

packages/api/src/schema/schema.yaml

      additionalProperties: false
      properties:


Right! On this object you would declare

Suggested change

additionalProperties: false

properties:

additionalProperties: false

required: [response]

properties:

To make the response field obligatory and remove the need to check manually on your API handler code. BUT I don't think it should be required anyway, just explaining here what you could do on another case like this (or if you prefer to keep it as required)

victorges · 2022-08-10T15:28:24Z

packages/api/src/schema/schema.yaml

@@ -273,7 +273,6 @@ components:
      table: webhook_response
      required:
        - webhookId
-        - eventId
        - statusCode


Turns out the statusCode field is already required. No need to have those checks in the code :)

victorges · 2022-08-10T15:31:14Z

packages/api/src/controllers/webhook.ts

+        body: response.response.body,
+        status: response.statusCode,


There is also a lot more stuff in the response object. Please save at least the response.headers here as well but do go through the schema to figure out anything else we want to record. Ideally we should at least keep it consistent with the webhook cannon code, which right now it still isnt'.

victorges · 2022-08-10T15:32:44Z

packages/api/src/schema/schema.yaml

-          $ref: "#/components/schemas/webhook/properties/status"
+        errorMessage:
+          type: string
+          description: Error message if the webhook failed to process the event
        response:
          $ref: "#/components/schemas/webhook-response"


Hmm... Maybe here we actually want to send a "#/components/schemas/webhook-response/properties/response" instead? seems like this objet has a lot of other fields that we don't need while we only really use the response.response field anyway (there is a status code in there too). Would simplify the client and the server-side code. This is the most optional change.

gioelecerati · 2022-08-10T16:23:08Z

As per async discussion, for faster debugging of token gating the webhookResponse is not saved on update status for now, going to implement it with a compatible object in another pr.

victorges · 2022-08-10T16:46:52Z

packages/api/src/store/webhook-table.ts

@@ -29,7 +29,7 @@ export default class WebhookTable extends Table<DBWebhook> {
    const query = [sql`data->>'userId' = ${userId}`];
    if (streamId) {
      query.push(
-        sql`data->>'streamId' = ${streamId} OR data->>'streamId' IS NULL`
+        sql`(data->>'streamId' = ${streamId} OR data->>'streamId' IS NULL)`


Nice! At some point we should just do that inside that find() function that concats everything with an AND. Shouldn't be the consumer's responsibility to care about this.

webhooks: allow to update webhook status & response

8e5bb1d

gioelecerati requested a review from a team as a code owner August 5, 2022 16:35

cleanup

280beb8

vercel bot deployed to Preview August 5, 2022 16:42 View deployment

webhook: added test for status update

2892af7

vercel bot deployed to Preview August 5, 2022 17:10 View deployment

victorges approved these changes Aug 5, 2022

View reviewed changes

some updates

9801150

vercel bot deployed to Preview August 9, 2022 12:53 View deployment

change response

a1f18c5

vercel bot deployed to Preview August 10, 2022 13:18 View deployment

fix test

e33569b

vercel bot deployed to Preview August 10, 2022 15:12 View deployment

victorges approved these changes Aug 10, 2022

View reviewed changes

webhooks: fix issue with subscribed query

4740f43

vercel bot deployed to Preview August 10, 2022 16:16 View deployment

remove webhook response save : todo

1cd465c

vercel bot deployed to Preview August 10, 2022 16:22 View deployment

gioelecerati merged commit 8f732b2 into master Aug 10, 2022

gioelecerati deleted the gio/webhooks/update-status branch August 10, 2022 16:45

victorges reviewed Aug 10, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

webhooks: allow to update webhook status & response #1185

webhooks: allow to update webhook status & response #1185

gioelecerati commented Aug 5, 2022 •

edited by victorges

vercel bot commented Aug 5, 2022 •

edited

codecov bot commented Aug 5, 2022 •

edited

victorges left a comment

victorges Aug 5, 2022

victorges Aug 5, 2022

victorges Aug 5, 2022

victorges Aug 5, 2022

gioelecerati Aug 9, 2022

gioelecerati Aug 9, 2022

victorges Aug 9, 2022

gioelecerati Aug 10, 2022

victorges Aug 10, 2022

victorges Aug 17, 2022

victorges Aug 5, 2022

victorges Aug 5, 2022

victorges Aug 5, 2022

victorges left a comment

victorges Aug 10, 2022

gioelecerati Aug 10, 2022

victorges Aug 10, 2022

victorges Aug 10, 2022

victorges Aug 10, 2022

victorges Aug 10, 2022

victorges Aug 10, 2022

victorges Aug 10, 2022

victorges Aug 10, 2022

victorges Aug 10, 2022

gioelecerati commented Aug 10, 2022

victorges Aug 10, 2022

		const doc = req.body.status;
		const webhookResponse = req.body.response;

	const doc = req.body.status;
	const webhookResponse = req.body.response;
	const { status, response} = req.body as WebhookStatusPayload;

	return res.status(404).json({ errors: ["webhook not found or deleted"] });
	return res.status(404).json({ errors: ["webhook not found"] });

		statusCode: response.statusCode,
		response: response.response.body,

webhooks: allow to update webhook status & response #1185

webhooks: allow to update webhook status & response #1185

Conversation

gioelecerati commented Aug 5, 2022 • edited by victorges

vercel bot commented Aug 5, 2022 • edited

codecov bot commented Aug 5, 2022 • edited

Codecov Report

victorges 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

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

victorges 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

Choose a reason for hiding this comment

gioelecerati commented Aug 10, 2022

Choose a reason for hiding this comment

gioelecerati commented Aug 5, 2022 •

edited by victorges

vercel bot commented Aug 5, 2022 •

edited

codecov bot commented Aug 5, 2022 •

edited