Skip to content

Commit fe97a3e

Browse files
lukaszczerpak-cloudinarylukaszczerpak-cloudinary
authored
Delo 2346 fix webspeedtest debug information sent to rollbar (#46)
* fixes * add api tests * typo * improved e2e tests and error handling fixes * cleanup * updated deps * updated deps * updated deps
1 parent 58f5e96 commit fe97a3e

File tree

10 files changed

+820
-978
lines changed

10 files changed

+820
-978
lines changed

‎app.js‎

Lines changed: 1 addition & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
const config = require('config');
22
const express = require('express');
3-
const http = require('http');
43
const bodyParser = require('body-parser');
54
const logger = require('./logger').logger;
65
const app = express();
@@ -51,16 +50,5 @@ app.use(function (err, req, res, next) {
5150
// render the error page
5251
res.status(err.status || 500);
5352
});
54-
const listenPort = process.env.PORT || 5000;
55-
56-
const server = http.createServer(app);
57-
server.setTimeout(3 * 60 * 1000)
58-
server.on('error', (e) => {
59-
console.log(e);
60-
})
61-
62-
server.listen(listenPort, () => {
63-
logger.info('Server started listing on port ' + listenPort);
64-
console.log('Listening on port ' + listenPort);
65-
});
6653

54+
module.exports = app;

‎cloudinary/apiCaller.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ const cloudinary = require('cloudinary');
1313
const async = require('async');
1414
const request = require('got');
1515

16-
const sentToAnalyze = (imagesArray, dpr, metaData, quality, cb, rollBarMsg) => {
16+
const sendToAnalyze = (imagesArray, dpr, metaData, quality, cb, rollBarMsg) => {
1717
let batchSize = config.get('cloudinary.batchSize');
1818
addServerInfo(imagesArray, batchSize, dpr, metaData, quality, cb, rollBarMsg);
1919
};
@@ -111,6 +111,6 @@ const sendToCloudinary = (imagesArray, batchSize, dpr, metaData, quality, cb, ro
111111
});
112112

113113
};
114-
module.exports = sentToAnalyze;
114+
module.exports = sendToAnalyze;
115115

116116

‎cloudinary/cloudinaryResultParser.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ const logger = require('../logger').logger;
66
const _ = require('lodash');
77
"use strict";
88

9-
const parseCloudinaryResults = (results, rollBarMag) => {
9+
const parseCloudinaryResults = (results, rollBarMsg) => {
1010

1111
try {
1212
let imagesTestResults = [];
@@ -49,7 +49,7 @@ const parseCloudinaryResults = (results, rollBarMag) => {
4949
}
5050
};
5151
} catch (e) {
52-
logger.error('Error parsing cloudinary result', e, rollBarMag);
52+
logger.error('Error parsing cloudinary result', e, rollBarMsg);
5353
return {status: 'error', message: 'Error parsing cloudinary result'}
5454
}
5555
};

‎package.json‎

Lines changed: 20 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,32 +1,32 @@
11
{
22
"name": "pagespeed-server",
3-
"version": "1.1.0",
3+
"version": "1.1.2",
44
"private": true,
55
"scripts": {
6-
"start": "node app.js"
6+
"start": "node start.js"
77
},
88
"dependencies": {
9-
"async": "^3.2.0",
10-
"body-parser": "~1.19.0",
11-
"bytes": "^3.1.0",
9+
"async": "^3.2.4",
10+
"body-parser": "~1.20.2",
11+
"bytes": "^3.1.2",
1212
"cloudinary": "https://github.com/cloudinary/cloudinary_npm.git#analyze_api_legacy",
13-
"config": "^3.3.6",
14-
"cookie-parser": "~1.4.5",
15-
"debug": "~4.3.1",
16-
"dotenv": "^10.0.0",
17-
"express": "~4.17.1",
18-
"express-winston": "^4.2.0",
19-
"got": "^11.8.2",
13+
"config": "^3.3.9",
14+
"cookie-parser": "~1.4.6",
15+
"debug": "~4.3.4",
16+
"dotenv": "^16.1.4",
17+
"express": "~4.18.2",
18+
"got": "^11.8.6",
2019
"lodash": "^4.17.21",
21-
"request": "^2.88.2",
22-
"rollbar": "^2.21.1",
23-
"valid-url": "^1.0.9",
24-
"winston": "^3.3.3",
25-
"winston-airbrake": "^1.1.0"
20+
"rollbar": "^2.26.1",
21+
"valid-url": "^1.0.9"
2622
},
2723
"devDependencies": {
28-
"chai": "^4.3.4",
29-
"husky": "^6.0.0",
30-
"mocha": "^8.4.0"
24+
"chai": "^4.3.7",
25+
"chai-http": "^4.4.0",
26+
"husky": "^8.0.3",
27+
"mocha": "^10.2.0",
28+
"nock": "^13.3.1",
29+
"sinon": "^15.1.2",
30+
"sinon-chai": "^3.7.0"
3131
}
3232
}

‎routes/wpt.js‎

Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3,16 +3,31 @@ const express = require('express');
33
const validUrl = require('valid-url');
44
const apiCaller = require('../wtp/apiCaller');
55
const logger = require('../logger').logger;
6+
const {LOG_LEVEL_INFO, LOG_LEVEL_WARNING, LOG_LEVEL_ERROR, LOG_LEVEL_CRITICAL, LOG_LEVEL_DEBUG} = require('../logger');
67
const path = require('path');
78

89
const routeCallback = (error, result, res, rollBarMsg) => {
910
if (error) {
1011
if (error.logLevel) {
11-
logger.configure({logLevel: error.logLevel});
12-
if (typeof error.error === 'object') {
13-
logger.log(error.message, error.error, rollBarMsg)
14-
} else {
15-
logger.log(error.message, rollBarMsg)
12+
let args = (typeof error.error === 'object')
13+
? [error.message, error.error, rollBarMsg]
14+
: [error.message, rollBarMsg];
15+
switch (error.logLevel) {
16+
case LOG_LEVEL_DEBUG:
17+
logger.debug(...args);
18+
break
19+
case LOG_LEVEL_INFO:
20+
logger.info(...args);
21+
break
22+
case LOG_LEVEL_WARNING:
23+
logger.warn(...args);
24+
break
25+
case LOG_LEVEL_ERROR:
26+
logger.error(...args);
27+
break
28+
case LOG_LEVEL_CRITICAL:
29+
logger.critical(...args);
30+
break
1631
}
1732
}
1833
if (error.statusCode) {
@@ -58,7 +73,7 @@ const wtp = (app) => {
5873
return;
5974
}*/
6075
logger.info('Started test called from webspeedtest', rollBarMsg, req);
61-
apiCaller.runWtpTest(testUrl, mobile, (error, result) => {
76+
apiCaller.runWtpTest(testUrl, mobile, (error, result, response, rollBarMsg) => {
6277
routeCallback(error, result, res, rollBarMsg)
6378
});
6479
});

‎start.js‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
const http = require('http');
2+
const logger = require('./logger').logger;
3+
const app = require('./app');
4+
5+
const listenPort = process.env.PORT || 5000;
6+
7+
const server = http.createServer(app);
8+
server.setTimeout(3 * 60 * 1000);
9+
server.on('error', (e) => {
10+
console.log(e);
11+
})
12+
13+
server.listen(listenPort, () => {
14+
logger.info('Server started listing on port ' + listenPort);
15+
console.log('Listening on port ' + listenPort);
16+
});

‎test/apiTests.js‎

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,95 @@
1+
process.env.NODE_ENV = 'test';
2+
process.env.LOG_LEVEL = 'warning';
3+
4+
let chai = require('chai');
5+
let chaiHttp = require('chai-http');
6+
let sinonChai = require('sinon-chai');
7+
let sinon = require('sinon');
8+
let server = require('../app');
9+
let should = chai.should();
10+
let expect = require('chai').expect
11+
let nock = require('nock');
12+
const {LOG_LEVEL_WARNING} = require("../logger");
13+
const logger = require('../logger');
14+
const log = logger.logger;
15+
16+
const RUN_TEST_HOST = 'http://www.webpagetest.org';
17+
const RUN_TEST_PATH = '/runtest.php';
18+
19+
chai.use(chaiHttp);
20+
chai.use(sinonChai);
21+
22+
describe('API', () => {
23+
beforeEach(function () {
24+
nock.disableNetConnect();
25+
nock.enableNetConnect('127.0.0.1');
26+
});
27+
28+
afterEach(function () {
29+
nock.cleanAll()
30+
nock.enableNetConnect()
31+
});
32+
33+
describe('GET /version', () => {
34+
it('it should contain version property', (done) => {
35+
chai.request(server)
36+
.get('/version')
37+
.end((err, res) => {
38+
res.should.have.status(200);
39+
res.body.should.have.property('version');
40+
done();
41+
});
42+
});
43+
});
44+
45+
describe('POST /test/run', () => {
46+
it('it should return 200 on general WPT API failure', async () => {
47+
let spy = sinon.spy(log, 'error');
48+
49+
const wpt = nock(RUN_TEST_HOST)
50+
.post(RUN_TEST_PATH)
51+
.query(true)
52+
.reply(500, 'Unknown WPT issue')
53+
54+
let res = await chai.request(server)
55+
.post('/test/run')
56+
.send({mobile: false, url: "http://www.example.com/"})
57+
58+
res.should.have.status(200);
59+
res.body.should.contain({status: 'error', message: 'WTP returned bad status with url http://www.example.com/'});
60+
expect(wpt.isDone()).to.be.true;
61+
62+
expect(log.error).to.be.called;
63+
expect(500).to.equal(spy.getCall(0).args[1].thirdPartyErrorCode);
64+
65+
log.error.restore();
66+
});
67+
68+
it('it should return 200 on soft error', async () => {
69+
sinon.spy(log, 'error');
70+
let spy = sinon.spy(log, 'warn');
71+
72+
const wpt = nock(RUN_TEST_HOST)
73+
.post(RUN_TEST_PATH)
74+
.query(true)
75+
.reply(200, {statusCode: 400, statusText: 'unknown issue'})
76+
77+
let res = await chai.request(server)
78+
.post('/test/run')
79+
.send({mobile: false, url: "http://www.example.com/"})
80+
81+
res.should.have.status(200);
82+
res.body.should.contain({status: 'error', message: 'wpt_failure'});
83+
expect(wpt.isDone()).to.be.true;
84+
85+
expect(log.warn).to.be.called;
86+
expect(log.error).to.not.be.called;
87+
expect(400).to.equal(spy.getCall(0).args[1].thirdPartyErrorCode);
88+
expect("unknown issue").to.equal(spy.getCall(0).args[1].responseBody);
89+
90+
log.error.restore();
91+
log.warn.restore();
92+
});
93+
});
94+
95+
});

‎wtp/apiCaller.js‎

Lines changed: 16 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ const getTestResults = async (testId, quality, cb) => {
5151
cloudinaryCaller(wtpRes.imageList, wtpRes.dpr, wtpRes.metaData, quality, cb, rollBarMsg);
5252
}
5353
} catch (e) {
54-
cb({status: 'error', message: 'Error calling WTP with testId ' + testId, error: error, logLevel: logger.LOG_LEVEL_ERROR}, null, response, rollBarMsg);
54+
cb({status: 'error', message: 'Error calling WTP with testId ' + testId, error: e, logLevel: logger.LOG_LEVEL_ERROR}, null, response, rollBarMsg);
5555
return;
5656
}
5757
};
@@ -75,27 +75,27 @@ const runWtpTest = async (url, mobile, cb) => {
7575
fvonly: 1, // first view only
7676
timeline: 1 // workaround for WPT sometimes hanging on getComputedStyle()
7777
},
78-
headers: { 'User-Agent': 'WebSpeedTest' }
78+
headers: { 'User-Agent': 'WebSpeedTest' },
79+
throwHttpErrors: false
7980
};
81+
let response;
82+
let rollBarMsg = {testId: "", analyzedUrl: url, thirdPartyErrorCode: "", file: path.basename((__filename))};
8083
try {
81-
const response = await got.post(options);
84+
response = await got.post(options);
8285
const {statusCode, body} = response;
83-
let bodyJson = JSON.parse(body);
84-
let tID = (typeof bodyJson.data !== 'undefined' && typeof bodyJson.data.testId !== 'undefined') ?
85-
(bodyJson.data.testId) :
86-
"N/A";
87-
let rollBarMsg = {testId: tID, analyzedUrl: url, thirdPartyErrorCode: "", file: path.basename((__filename))};
8886
if (statusCode !== 200) {
8987
rollBarMsg.thirdPartyErrorCode = response.statusCode;
90-
cb({status: 'error', message: 'WTP returned bad status with url ' + url}, null, response, rollBarMsg);
88+
cb({status: 'error', message: 'WTP returned bad status with url ' + url, error: response.statusMessage, logLevel: logger.LOG_LEVEL_ERROR}, null, response, rollBarMsg);
9189
return;
9290
}
9391
if (!body) {
94-
cb(
95-
{status: 'error', message: 'WTP returned empty body with url ' + url, error: 'empty body'}, null, response,
96-
rollBarMsg);
92+
cb({status: 'error', message: 'WTP returned empty body with url ' + url, error: 'empty body'}, null, response, rollBarMsg);
9793
return;
9894
}
95+
let bodyJson = JSON.parse(body);
96+
rollBarMsg.testId = (typeof bodyJson.data !== 'undefined' && typeof bodyJson.data.testId !== 'undefined') ?
97+
(bodyJson.data.testId) :
98+
"N/A";
9999
let testId = resultParser.parseTestResponse(bodyJson, rollBarMsg);
100100
if (typeof testId === 'object') {
101101
cb(null, testId);
@@ -116,11 +116,13 @@ const checkTestStatus = async (testId, quality, cb) => {
116116
searchParams: {test: testId, k: config.get('wtp.apiKey'), f: "json"},
117117
'headers': { 'User-Agent': 'WebSpeedTest' }
118118
};
119+
let response;
120+
let rollBarMsg = {};
119121
try {
120-
const response = await got.get(options);
122+
response = await got.get(options);
121123
const {statusCode, body} = response;
122124
let bodyJson = JSON.parse(body);
123-
let rollBarMsg = {testId: testId, thirdPartyErrorCode: "", file: path.basename((__filename))};
125+
rollBarMsg = {testId: testId, thirdPartyErrorCode: "", file: path.basename((__filename))};
124126
if (statusCode !== 200) {
125127
cb({status: 'error', message: 'WTP returned bad status testId ' + testId , error: response.statusCode}, null, response, rollBarMsg);
126128
return;

‎wtp/wtpResultsParser.js‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -95,15 +95,10 @@ const parseTestResults = (testJson) => {
9595
}
9696
};
9797

98-
/*
99-
const extractFileName = (uri) => {
100-
let parsedUrl = url.parse(uri);
101-
return path.basename(parsedUrl.pathname)
102-
};
103-
*/
104-
10598
const parseTestResponse = (body, rollBarMsg) => {
10699
if (body.statusText !== 'Ok') {
100+
rollBarMsg.thirdPartyErrorCode = body?.statusCode;
101+
rollBarMsg.responseBody = body.statusText;
107102
logger.warn('WPT returned an error', rollBarMsg);
108103
return {status: 'error', message: 'wpt_failure'}
109104
}

0 commit comments

Comments
 (0)