perf_hooks: add HttpRequest statistics monitoring #28445 #28486
Conversation
nodejs-github-bot
added
c++
|
Can't we add some conditionals to avoid these extra bits of code at runtime? |
|
@mscdex I believe we could check when we call |
addaleax
added
http
vmarchaud
force-pushed
the
http-perf-hooks
branch
from
7fef792
to
20c8529
Compare
Not necessarily. Modules could register hooks that get called from C++ whenever a particular "observer" is enabled/disabled. These hooks could then simply set a local (to the module) boolean variable that is checked in the actual code. EDIT: After briefly reviewing perf_hooks.js and related C++ code, it seems if JavaScript is the only place where performance hooks can be enabled/disabled, then we shouldn't even need to go through C++ at all, we could either just call these per-module hooks directly, have a lookup table of enabled/disabled status for each module, or have a way for each module to (if I'm understanding perf_hooks.js correctly) check |
|
@mscdex I'm not really aware of how the frontier works but the observers table is on the c++ side here, the only thing exposed to JS land is the |
|
@vmarchaud Accessing typed arrays created in C++, like |
vmarchaud
force-pushed
the
http-perf-hooks
branch
from
20c8529
to
d003117
Compare
|
Done. I hardcoded the index of the |
vmarchaud
force-pushed
the
http-perf-hooks
branch
from
d003117
to
29f6586
Compare
vmarchaud
force-pushed
the
http-perf-hooks
branch
2 times, most recently
from
b0df4ba
to
d8a7384
Compare
vmarchaud
force-pushed
the
http-perf-hooks
branch
from
d8a7384
to
4dd0750
Compare
vmarchaud
force-pushed
the
http-perf-hooks
branch
from
4dd0750
to
8de724e
Compare
addaleax
added
the
author ready
```js
const { PerformanceObserver, performance } = require('perf_hooks');
const http = require('http');
const obs = new PerformanceObserver((items) => {
const entry = items.getEntries()[0];
console.log(entry.name, entry.duration);
});
obs.observe({ entryTypes: ['http'] });
const server = http.Server(function(req, res) {
server.close();
res.writeHead(200);
res.end('hello world\n');
});
server.listen(0, function() {
const req = http.request({
port: this.address().port,
path: '/',
method: 'POST'
}).end();
});
```
vmarchaud
force-pushed
the
http-perf-hooks
branch
from
8de724e
to
15bb9be
Compare
|
I believe the PR is good to be landed ? |
|
@vmarchaud Yes, thanks for the ping! I’ll start a new CI because that hasn’t happened for the latest (force-)push yet, but otherwise this should be ready. |
|
Landed in 0ebf01d 🎉 |
```js
const { PerformanceObserver, performance } = require('perf_hooks');
const http = require('http');
const obs = new PerformanceObserver((items) => {
const entry = items.getEntries()[0];
console.log(entry.name, entry.duration);
});
obs.observe({ entryTypes: ['http'] });
const server = http.Server(function(req, res) {
server.close();
res.writeHead(200);
res.end('hello world\n');
});
server.listen(0, function() {
const req = http.request({
port: this.address().port,
path: '/',
method: 'POST'
}).end();
});
```
PR-URL: nodejs#28486
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
```js
const { PerformanceObserver, performance } = require('perf_hooks');
const http = require('http');
const obs = new PerformanceObserver((items) => {
const entry = items.getEntries()[0];
console.log(entry.name, entry.duration);
});
obs.observe({ entryTypes: ['http'] });
const server = http.Server(function(req, res) {
server.close();
res.writeHead(200);
res.end('hello world\n');
});
server.listen(0, function() {
const req = http.request({
port: this.address().port,
path: '/',
method: 'POST'
}).end();
});
```
PR-URL: #28486
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Add a way to track http request timing directly via
perf_hooks:I initially started this by adding inside the socket state but it could lead to race condition where multiple request on the same socket would mess with each other timings.
The downside of this approach is that we can't track how much it take to parse the header or how much byte we read/write for this connection, it would require to add more hook to the
ServerResponsethat could impact performance.Since generally we want to monitor how much times it take by userland to process the request, since approach should be enough.
I've also added a generic
Notifyon the performance c++ to allow any core js-land module to broadcastPerformanceEntry, it should allow to instrument other core js module. There still a problem through, this method will not broadcasttrace_eventsfor each perf entry, i don't know if it really necessary ?cc @jasnell
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes