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

http2: perf_hooks integration #17906

Closed
wants to merge 2 commits into from
Closed

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Dec 28, 2017

Collect and report basic timing information about Http2Session and Http2Stream instances.

This is just the initial impl. Additional stats to be added.

const { PerformanceObserver } = require('perf_hooks');

const obs = new PerformanceObserver((items) => {
  const entry = items.getEntries()[0];
  console.log(entry);
});
obs.observe({ entryTypes: ['http2'] });

ping @nodejs/http2

Refs: #17746

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

http2

Collect and report basic timing information about `Http2Session`
and `Http2Stream` instances.
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Dec 28, 2017
@@ -2800,13 +2800,63 @@ given newly created [`Http2Stream`] on `Http2ServerRespose`.
The callback will be called with an error with code `ERR_HTTP2_STREAM_CLOSED`
if the stream is closed.

## Collecting HTTP/2 Performance Metrics

The [Performance Observer][] API can be used to collect basic performance
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be [Performance Hooks][] or should the bottom reference be changed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reference updated

doc/api/http2.md Outdated
If `name` is equal to `Http2Session`, the `PerformanceEntry` will contain the
following additional properties:

* `pingRTT` {number} The number of millseconds elapsed since the transmission
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

millseconds -> milliseconds

doc/api/http2.md Outdated
following additional properties:

* `pingRTT` {number} The number of millseconds elapsed since the transmission
of a `PING` frame and the reception of it's acknowledgement. Only present if
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's -> its

@jasnell jasnell mentioned this pull request Dec 28, 2017
11 tasks
@jasnell
Copy link
Member Author

jasnell commented Dec 28, 2017

@jasnell
Copy link
Member Author

jasnell commented Dec 29, 2017

CI is good. One build bot failure on windows is unrelated.

@jasnell
Copy link
Member Author

jasnell commented Dec 30, 2017

Ping @nodejs/http2

@jasnell jasnell added the http2 Issues or PRs related to the http2 subsystem. label Jan 2, 2018
* `timeToFirstByte` {number} The number of milliseconds elapsed between the
`PerformanceEntry` `startTime` and the reception of the first `DATA` frame.
* `timeToFirstHeader` {number} The number of milliseconds elapsed between the
`PerformanceEntry` `startTime` and the reception of the first header.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is only for data coming in, should we include also some metrics for data going out?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, this is only the initial implementation. Once it lands, I plan to extend the available metrics to include additional pieces.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

jasnell added a commit that referenced this pull request Jan 2, 2018
Collect and report basic timing information about `Http2Session`
and `Http2Stream` instances.

PR-URL: #17906
Refs: #17746
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@jasnell
Copy link
Member Author

jasnell commented Jan 2, 2018

Landed in 231b116

@jasnell jasnell closed this Jan 2, 2018
MylesBorins pushed a commit to jasnell/node that referenced this pull request Jan 9, 2018
Collect and report basic timing information about `Http2Session`
and `Http2Stream` instances.

PR-URL: nodejs#17906
Refs: nodejs#17746
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
Collect and report basic timing information about `Http2Session`
and `Http2Stream` instances.

Backport-PR-URL: #18050
PR-URL: #17906
Refs: #17746
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jan 10, 2018
MylesBorins added a commit that referenced this pull request Jan 10, 2018
Notable change:

* async_hooks:
  - deprecate AsyncHooks Sensitive API and runInAsyncIdScope. Neither
    API were documented. (Andreas Madsen)
    #16972
* deps:
  - update nghttp2 to 1.29.0 (James M Snell)
    #17908
  - upgrade npm to 5.6.0 (Kat Marchán)
    #17535
  - cherry-pick 50f7455 from upstream V8 (Michaël Zasso)
    #16591
* events:
  - remove reaches into _events internals (Anatoli Papirovski)
    #17440
* http:
  - add rawPacket in err of `clientError` event (XadillaX)
    #17672
* http2:
  - implement maxSessionMemory (James M Snell)
    #17967
  - add initial support for originSet (James M Snell)
    #17935
  - add altsvc support (James M Snell)
    #17917
  - perf_hooks integration (James M Snell)
    #17906
* net:
  - remove Socket.prototype.write (Anna Henningsen)
    #17644
  - remove Socket.prototype.listen (Ruben Bridgewater)
    #13735
* repl:
  - show lexically scoped vars in tab completion (Michaël Zasso)
    #16591
* stream:
  - rm {writeable/readable}State.length (Calvin Metcalf)
    #12857
  - add flow and buffer properties to streams (Calvin Metcalf)
    #12855
* util:
  - allow wildcards in NODE_DEBUG variable (Tyler)
    #17609
* zlib:
  - add ArrayBuffer support (Jem Bezooyen)
    #16042
* Addedew collaborator**
  - [starkwang](https://github.com/starkwang) Weijia Wang
* Addedew TSC member**
  - [danbev](https://github.com/danbev) Daniel Bevenius

PR-URL: #18069
MylesBorins pushed a commit that referenced this pull request Jan 10, 2018
Collect and report basic timing information about `Http2Session`
and `Http2Stream` instances.

Backport-PR-URL: #18050
PR-URL: #17906
Refs: #17746
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins added a commit that referenced this pull request Jan 10, 2018
Notable change:

* async_hooks:
  - deprecate AsyncHooks Sensitive API and runInAsyncIdScope. Neither
    API were documented. (Andreas Madsen)
    #16972
* deps:
  - update nghttp2 to 1.29.0 (James M Snell)
    #17908
  - upgrade npm to 5.6.0 (Kat Marchán)
    #17535
  - cherry-pick 50f7455 from upstream V8 (Michaël Zasso)
    #16591
* events:
  - remove reaches into _events internals (Anatoli Papirovski)
    #17440
* http:
  - add rawPacket in err of `clientError` event (XadillaX)
    #17672
* http2:
  - implement maxSessionMemory (James M Snell)
    #17967
  - add initial support for originSet (James M Snell)
    #17935
  - add altsvc support (James M Snell)
    #17917
  - perf_hooks integration (James M Snell)
    #17906
  - Refactoring and cleanup of Http2Session and Http2Stream destroy
    (James M Snell) #17406
* net:
  - remove Socket.prototype.write (Anna Henningsen)
    #17644
  - remove Socket.prototype.listen (Ruben Bridgewater)
    #13735
* repl:
  - show lexically scoped vars in tab completion (Michaël Zasso)
    #16591
* stream:
  - rm {writeable/readable}State.length (Calvin Metcalf)
    #12857
  - add flow and buffer properties to streams (Calvin Metcalf)
    #12855
* util:
  - allow wildcards in NODE_DEBUG variable (Tyler)
    #17609
* zlib:
  - add ArrayBuffer support (Jem Bezooyen)
    #16042
* Addedew collaborator**
  - [starkwang](https://github.com/starkwang) Weijia Wang
* Addedew TSC member**
  - [danbev](https://github.com/danbev) Daniel Bevenius

PR-URL: #18069
MylesBorins added a commit that referenced this pull request Jan 10, 2018
Notable change:

* async_hooks:
  - deprecate AsyncHooks Sensitive API and runInAsyncIdScope. Neither
    API were documented. (Andreas Madsen)
    #16972
* deps:
  - update nghttp2 to 1.29.0 (James M Snell)
    #17908
  - upgrade npm to 5.6.0 (Kat Marchán)
    #17535
  - cherry-pick 50f7455 from upstream V8 (Michaël Zasso)
    #16591
* events:
  - remove reaches into _events internals (Anatoli Papirovski)
    #17440
* http:
  - add rawPacket in err of `clientError` event (XadillaX)
    #17672
* http2:
  - implement maxSessionMemory (James M Snell)
    #17967
  - add initial support for originSet (James M Snell)
    #17935
  - add altsvc support (James M Snell)
    #17917
  - perf_hooks integration (James M Snell)
    #17906
  - Refactoring and cleanup of Http2Session and Http2Stream destroy
    (James M Snell) #17406
* net:
  - remove Socket.prototype.write (Anna Henningsen)
    #17644
  - remove Socket.prototype.listen (Ruben Bridgewater)
    #13735
* repl:
  - show lexically scoped vars in tab completion (Michaël Zasso)
    #16591
* stream:
  - rm {writeable/readable}State.length (Calvin Metcalf)
    #12857
  - add flow and buffer properties to streams (Calvin Metcalf)
    #12855
* util:
  - allow wildcards in NODE_DEBUG variable (Tyler)
    #17609
* zlib:
  - add ArrayBuffer support (Jem Bezooyen)
    #16042
* Addedew collaborator**
  - [starkwang](https://github.com/starkwang) Weijia Wang
* Addedew TSC member**
  - [danbev](https://github.com/danbev) Daniel Bevenius

PR-URL: #18069
kjin pushed a commit to kjin/node that referenced this pull request May 1, 2018
Collect and report basic timing information about `Http2Session`
and `Http2Stream` instances.

Backport-PR-URL: nodejs#18050
PR-URL: nodejs#17906
Refs: nodejs#17746
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 2, 2018
Collect and report basic timing information about `Http2Session`
and `Http2Stream` instances.

Backport-PR-URL: #18050
Backport-PR-URL: #20456
PR-URL: #17906
Refs: #17746
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@MylesBorins MylesBorins mentioned this pull request May 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. http2 Issues or PRs related to the http2 subsystem. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants