Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature Request: Performance hooks entry for http #28445

Closed
vmarchaud opened this issue Jun 27, 2019 · 6 comments
Closed

Feature Request: Performance hooks entry for http #28445

vmarchaud opened this issue Jun 27, 2019 · 6 comments

Comments

@vmarchaud
Copy link
Contributor

Is your feature request related to a problem? Please describe.
Retrieve metrics from a HTTP server.

Describe the solution you'd like
Pretty much the same as the current implementation between http2 and perf_hooks : https://github.com/nodejs/node/blob/master/test/parallel/test-http2-perf_hooks.js

Since the http server is only js-land, i believe we could just implement by hooking to the perf_hooks js api.
I've looked a little bit at the code, we could add a stats object in the request state and emit them when the request is ended (of course only if the entry has an observer).
I'm really interested on implementing it so i'm opening it an issue to have some feedback before coding anything.

Describe alternatives you've considered

Currently i believe all APM vendors are forced to monkey-patch the event-emitter on http.Server.prototype and then listen on request event, add event listener to know when the response is sent and compute some metrics about both the request and the response.

@vmarchaud
Copy link
Contributor Author

@jasnell i believe you worked a lot on the perf_hooks, do you have any opinion on this ?

@Flarna
Copy link
Member

Flarna commented Jun 27, 2019

Currently i believe all APM vendors are forced to monkey-patch ...

Yes, APMs do monkey-patching of http but we have more needs then the data provided by perf_hooks therefore this would be no replacement but a good extension/partial replacement.

@vmarchaud
Copy link
Contributor Author

@Flarna I totally agree, it's a large task to offer all the functionality of an APM in the core.
I believe starting to add more metrics natively will be useful for both APM vendors that will offload complexity of their agent and common users that will be able to get basic monitoring easily.

@jasnell
Copy link
Member

jasnell commented Jun 27, 2019

It is certainly possible. One of the challenges with the http implementation currently is that it is fairly brittle performance wise... we would need to be careful about where we introduce data collection and how. The actual emitting of the perf entries is done at the C++ layer because there is integration into the trace events mechanism but that's a fairly minor detail. Bottom line, this is certainly doable and I'm happy to help mentor someone through what is needed. A good first step, however, would be to detail which metrics are most helpful.

@vmarchaud
Copy link
Contributor Author

One of the challenges with the http implementation currently is that it is fairly brittle performance wise

Agreed that we need to take a look at the benchmark before/after.

The actual emitting of the perf entries is done at the C++ layer because there is integration into the trace events mechanism but that's a fairly minor detail

I suppose you were referring to the http2 implementation but i didn't find any place on the code where the pref entries are linked to the trace events mechanism (i found your old PR that was never merged there though: #18809). I would be interested to understand the connection between both

A good first step, however, would be to detail which metrics are most helpful.

Based from the specification of OpenCensus (which i believe people studied which metrics were useful for end users when making a choice) and the already-implemented http2 metrics, i think we could target:

  • Time to first byte received
  • Received bytes
  • Time to first byte sent
  • Total duration (from first byte read to last byte written)
  • Sent bytes

What do you think ?

vmarchaud added a commit to vmarchaud/node that referenced this issue Jul 8, 2019
```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();
});
```
dnalborczyk pushed a commit to dnalborczyk/node that referenced this issue Jul 11, 2019
```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>
@vmarchaud
Copy link
Contributor Author

Closing it landed on master

targos pushed a commit that referenced this issue Jul 20, 2019
```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>
This was referenced Jul 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants