Skip to content

Commit e23dd47

Browse files
mikesurowiecmikesurowiec
authored
Merge pull request github#18602 from github/webhook-response-no-hydro
don't await hydro for /events POST response
2 parents 59ab72d + f79d23c commit e23dd47

File tree

3 files changed

+51
-51
lines changed

3 files changed

+51
-51
lines changed

‎lib/hydro.js

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,11 @@
11
const crypto = require('crypto')
22
const fetch = require('node-fetch')
3+
const statsd = require('../lib/statsd')
4+
const FailBot = require('../lib/failbot')
5+
6+
const hydroStats = statsd.childClient({
7+
prefix: 'hydro.'
8+
})
39

410
const SCHEMAS = {
511
page: 'docs.v0.PageEvent',
@@ -62,7 +68,7 @@ module.exports = class Hydro {
6268
})
6369
const token = this.generatePayloadHmac(body)
6470

65-
return fetch(this.endpoint, {
71+
const doFetch = () => fetch(this.endpoint, {
6672
method: 'POST',
6773
body,
6874
headers: {
@@ -71,5 +77,25 @@ module.exports = class Hydro {
7177
'X-Hydro-App': 'docs-production'
7278
}
7379
})
80+
81+
const res = await hydroStats.asyncTimer(doFetch, 'response_time')()
82+
83+
hydroStats.increment(`response_code.${res.statusCode}`, 1)
84+
hydroStats.increment('response_code.all', 1)
85+
86+
// Track hydro exceptions in Sentry, but don't track 503s because we can't do anything about service availability
87+
if (!res.ok && res.status !== 503) {
88+
const err = new Error(`Hydro request failed: ${res.statusText}`)
89+
err.status = res.status
90+
91+
FailBot.report(err, {
92+
hydroStatus: res.status,
93+
hydroText: res.statusText
94+
})
95+
96+
throw err
97+
}
98+
99+
return res
74100
}
75101
}

‎middleware/events.js

Lines changed: 9 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@ const express = require('express')
22
const { omit } = require('lodash')
33
const Ajv = require('ajv')
44
const schema = require('../lib/schema-event')
5-
const FailBot = require('../lib/failbot')
65

76
const OMIT_FIELDS = ['type', 'token']
87

@@ -19,43 +18,18 @@ router.post('/', async function postEvents (req, res, next) {
1918
return res.status(400).json({})
2019
}
2120

22-
// Don't depend on Hydro on local development
23-
if (isDev && !req.hydro.maySend()) {
24-
return res.status(200).json({})
25-
}
26-
27-
try {
28-
const hydroRes = await req.hydro.publish(
21+
if (req.hydro.maySend()) {
22+
// intentionally don't await this async request
23+
// so that the http response afterwards is sent immediately
24+
req.hydro.publish(
2925
req.hydro.schemas[fields.type],
3026
omit(fields, OMIT_FIELDS)
31-
)
32-
33-
if (!hydroRes.ok) {
34-
const err = new Error('Hydro request failed')
35-
err.status = hydroRes.status
36-
err.path = fields.path
37-
38-
await FailBot.report(err, {
39-
path: fields.path,
40-
hydroStatus: hydroRes.status,
41-
hydroText: await hydroRes.text()
42-
})
43-
44-
throw err
45-
}
46-
47-
if (res.headersSent) {
48-
throw new Error('Cannot send http response: Hydro publish succeeded, but took too long.')
49-
}
50-
51-
return res.status(201).json(fields)
52-
} catch (err) {
53-
if (isDev) console.error(err)
54-
55-
if (!res.headersSent) {
56-
return res.status(502).json({})
57-
}
27+
).catch((e) => {
28+
if (isDev) console.error(e)
29+
})
5830
}
31+
32+
return res.status(200).json({})
5933
})
6034

6135
module.exports = router

‎tests/rendering/events.js

Lines changed: 15 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ describe('POST /events', () => {
7474
const pageExample = { ...baseExample, type: 'page' }
7575

7676
it('should record a page event', () =>
77-
checkEvent(pageExample, 201)
77+
checkEvent(pageExample, 200)
7878
)
7979

8080
it('should require a type', () =>
@@ -128,7 +128,7 @@ describe('POST /events', () => {
128128
...pageExample.context,
129129
page_event_id: baseExample.context.event_id
130130
}
131-
}, 201)
131+
}, 200)
132132
)
133133

134134
it('should not allow a honeypot token', () =>
@@ -201,7 +201,7 @@ describe('POST /events', () => {
201201
}, 400)
202202
)
203203

204-
it('should a valid os option', () =>
204+
it('should os a valid os option', () =>
205205
checkEvent({
206206
...pageExample,
207207
context: {
@@ -295,7 +295,7 @@ describe('POST /events', () => {
295295
}
296296

297297
it('should record an exit event', () =>
298-
checkEvent(exitExample, 201)
298+
checkEvent(exitExample, 200)
299299
)
300300

301301
it('should exit_render_duration is a positive number', () =>
@@ -330,7 +330,7 @@ describe('POST /events', () => {
330330
}
331331

332332
it('should send a link event', () =>
333-
checkEvent(linkExample, 201)
333+
checkEvent(linkExample, 200)
334334
)
335335

336336
it('link_url is a required uri formatted string', () =>
@@ -347,15 +347,15 @@ describe('POST /events', () => {
347347
}
348348

349349
it('should record a search event', () =>
350-
checkEvent(searchExample, 201)
350+
checkEvent(searchExample, 200)
351351
)
352352

353353
it('search_query is required string', () =>
354354
checkEvent({ ...searchExample, search_query: undefined }, 400)
355355
)
356356

357357
it('search_context is optional string', () =>
358-
checkEvent({ ...searchExample, search_context: undefined }, 201)
358+
checkEvent({ ...searchExample, search_context: undefined }, 200)
359359
)
360360
})
361361

@@ -367,11 +367,11 @@ describe('POST /events', () => {
367367
}
368368

369369
it('should record a navigate event', () =>
370-
checkEvent(navigateExample, 201)
370+
checkEvent(navigateExample, 200)
371371
)
372372

373373
it('navigate_label is optional string', () =>
374-
checkEvent({ ...navigateExample, navigate_label: undefined }, 201)
374+
checkEvent({ ...navigateExample, navigate_label: undefined }, 200)
375375
)
376376
})
377377

@@ -385,7 +385,7 @@ describe('POST /events', () => {
385385
}
386386

387387
it('should record a survey event', () =>
388-
checkEvent(surveyExample, 201)
388+
checkEvent(surveyExample, 200)
389389
)
390390

391391
it('survey_vote is boolean', () =>
@@ -411,7 +411,7 @@ describe('POST /events', () => {
411411
}
412412

413413
it('should record an experiment event', () =>
414-
checkEvent(experimentExample, 201)
414+
checkEvent(experimentExample, 200)
415415
)
416416

417417
it('experiment_name is required string', () =>
@@ -423,7 +423,7 @@ describe('POST /events', () => {
423423
)
424424

425425
it('experiment_success is optional boolean', () =>
426-
checkEvent({ ...experimentExample, experiment_success: undefined }, 201)
426+
checkEvent({ ...experimentExample, experiment_success: undefined }, 200)
427427
)
428428
})
429429

@@ -436,7 +436,7 @@ describe('POST /events', () => {
436436
}
437437

438438
it('should record an redirect event', () =>
439-
checkEvent(redirectExample, 201)
439+
checkEvent(redirectExample, 200)
440440
)
441441

442442
it('redirect_from is required url', () =>
@@ -456,7 +456,7 @@ describe('POST /events', () => {
456456
}
457457

458458
it('should record an clipboard event', () =>
459-
checkEvent(clipboardExample, 201)
459+
checkEvent(clipboardExample, 200)
460460
)
461461

462462
it('clipboard_operation is required copy, paste, cut', () =>
@@ -471,7 +471,7 @@ describe('POST /events', () => {
471471
}
472472

473473
it('should record a print event', () =>
474-
checkEvent(printExample, 201)
474+
checkEvent(printExample, 200)
475475
)
476476
})
477477
})

0 commit comments

Comments
 (0)