[#172163927] add keepalive and timeout to webhook calls #42
Conversation
Affected stories
New dependencies added: abort-controllerAuthor: Toru Nagashima Description: An implementation of WHATWG AbortController interface. Homepage: https://github.com/mysticatea/abort-controller#readme
Generated by 🚫 dangerJS |
| import { SuccessResponse } from "../generated/notifications/SuccessResponse"; | ||
|
|
||
| export type WebhookNotifyT = r.IPostApiRequestType< | ||
| { readonly notification?: Notification; readonly webhookEndpoint: HttpsUrl }, |
There was a problem hiding this comment.
why is notification optional? what's the use case for an empty notification?
There was a problem hiding this comment.
The generated requestType (from io-utils) is:
// Request type definition
export type NotifyT = r.IPostApiRequestType<
{ readonly notification?: Notification },
"Content-Type",
never,
| r.IResponseType<200, SuccessResponse>
| r.IResponseType<400, ProblemJson>
| r.IResponseType<401, undefined>
| r.IResponseType<500, ProblemJson>
>;probably because this parameter is not required
https://github.com/pagopa/io-backend/blob/master/api_notifications.yaml#L20
There was a problem hiding this comment.
it looks like it's actually required, we should fix the specs
| @@ -44,14 +53,26 @@ const notificationModel = new NotificationModel( | |||
| // Whether we're in a production environment | |||
| const isProduction = process.env.NODE_ENV === "production"; | |||
|
|
|||
| // Webhook must be an https endpoint | |||
| const abortableFetch = AbortableFetch(agent.getHttpsFetch(process.env)); | |||
There was a problem hiding this comment.
why getHttpsFetch needs to have access to all the environment? we should minimize the data required, can we just pass the relevant env vars?
There was a problem hiding this comment.
the whole point of this utility method is to abstract away the processing of options from the environment.
if we make it accepts only the required properties we end up reply (cut&paste) all this logic wherever you have to get a new agent: https://github.com/pagopa/io-ts-commons/blob/master/src/agent.ts#L10
which make the whole agent module useless
| // Webhook must be an https endpoint | ||
| const abortableFetch = AbortableFetch(agent.getHttpsFetch(process.env)); | ||
| // 10 seconds timeout by default | ||
| const fetchWithTimeout = setFetchTimeout( |
There was a problem hiding this comment.
this behavior differs from the one we implemented in app-backend where we rely on agentkeepalive's defaults, I would try to keep the behavior consistent across components
There was a problem hiding this comment.
what do you mean exactly ? the configured http agent does not terminate (abort) the connection, it only signal a timeout event and keeps the connection open. to close it we must use setFetchTimeout
There was a problem hiding this comment.
sorry I misunderstood the code
WebhookNotificationActivity/index.ts
Outdated
| const abortableFetch = AbortableFetch(agent.getHttpsFetch(process.env)); | ||
| // 10 seconds timeout by default | ||
| const fetchWithTimeout = setFetchTimeout( | ||
| (process.env.FETCH_KEEPALIVE_TIMEOUT |
There was a problem hiding this comment.
I'm not sure this makes sense, FETCH_KEEPALIVE_TIMEOUT sets the timeout on the socket while setFetchTimeout sets the timeout on the HTTP(s) request - I believe the former should be much higher than the latter (in fact it defaults to 30s)
There was a problem hiding this comment.
what do you suggest as request timeout value ?
There was a problem hiding this comment.
I would look at the 95th percentile response time of the webhook endpoint and add 20% to that
Codecov Report
@@ Coverage Diff @@
## master #42 +/- ##
==========================================
- Coverage 83.91% 82.29% -1.63%
==========================================
Files 7 8 +1
Lines 373 384 +11
Branches 47 49 +2
==========================================
+ Hits 313 316 +3
- Misses 59 67 +8
Partials 1 1
Continue to review full report at Codecov.
|
drop superagent and use node-fetch with timeout and keepalive to call the notify webhook