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

added before-flushing-head event to _http_server.ServerResponse.writeHead #2538

Closed
wants to merge 10 commits into from

Conversation

yoavaa
Copy link

@yoavaa yoavaa commented Aug 25, 2015

The before-flushing-head event is needed for monitoring of HTTP requests and capturing the time to first byte as well as for enabling middleware to write headers to responses. TTFB (time to first byte) is important measurement as it measures the server performance regardless of sending data down the stream and it actually what most monitoring tools measure on the client side, including google with their page speed and SEO engines.

Today we have to resort to using a wrapper for this method, but I think it is a great functionality to have in node.js

@mscdex mscdex added the http Issues or PRs related to the http subsystem. label Aug 25, 2015
@thefourtheye
Copy link
Contributor

I believe this has to be well documented and should have proper tests.

@Fishrock123 Fishrock123 added semver-minor PRs that contain new features and should be released in the next minor version. feature request Issues that request new features to be added to Node.js. labels Aug 25, 2015
@Fishrock123
Copy link
Contributor

Should it emit any of the arguments writeHead was called with?

@yoavaa
Copy link
Author

yoavaa commented Aug 26, 2015

@thefourtheye - I have added docs and a test.

@Fishrock123 - I do not think it should get any parameters. When you set the event listener you have a reference to the res object. At that same point you can use the same res object to customize the response head before flushing it.

@yoavaa
Copy link
Author

yoavaa commented Aug 26, 2015

@Fishrock123 - thinking about this again - given how the code looks like now, you cannot modify the head status code or message. Do you think we should enable customizing those as well? (you can customize headers via the APIs exposed on the res object. However, because writeHead takes the statusCode and reason as parameters, the res API cannot really update them at this point.

@Fishrock123
Copy link
Contributor

@yoavaa a crazy idea that would allow arg modification:

function writeHead(statusCode, reason, obj) {
  var args = { statusCode, reason, obj }
  this.emit('before-flushing-head', args);
  this._writeHead(args);
}

@yoavaa
Copy link
Author

yoavaa commented Aug 26, 2015

The function itself allows a more complex handing of the args - it starts by checking for missing args and reordering them

  if (typeof reason === 'string') {
    // writeHead(statusCode, reasonPhrase[, headers])
    this.statusMessage = reason;
  } else {
    // writeHead(statusCode[, headers])
    this.statusMessage =
        this.statusMessage || STATUS_CODES[statusCode] || 'unknown';
    obj = reason;
  }
  this.statusCode = statusCode;

also, right after that the function copies headers from the _header member into obj, allowing the event handler to add additional headers using the response public APIs.
e.g.

// in writeHead
  if (this._headers) {
    // Slow-case: when progressive API and header fields are passed.
    if (obj) {
      var keys = Object.keys(obj);
      for (var i = 0; i < keys.length; i++) {
        var k = keys[i];
        if (k) this.setHeader(k, obj[k]);
      }
    }
    // only progressive api is used
    headers = this._renderHeaders();
  } else {
    // only writeHead() called
    headers = obj;
  }

because of this if we want the event to allow modification of the response code, response message and headers we are forced into emitting the event only after the second code block and effectively preventing users from modifying the headers using the setHeader method. Instead, people will have to use the header object passed into the event handler.

If we go with that, this is the new method (below). The event now gets the args object which includes

{ 
  statusCode: //the status code to be written
  reason: //the status message,
  headers: //headers object  
}

the method

ServerResponse.prototype.writeHead = function(statusCode, reason, obj) {
  var headers;

  // writeHead(statusCode, reasonPhrase[, headers])
  var args = {statusCode: statusCode, statusMessage: reason, headers: obj};

  if (!(typeof args.statusMessage === 'string')) {
    // writeHead(statusCode[, headers])
    args.statusMessage =
        this.statusMessage || STATUS_CODES[statusCode] || 'unknown';
    args.headers = reason;
  }

  if (this._headers) {
    // Slow-case: when progressive API and header fields are passed.
    if (obj) {
      var keys = Object.keys(obj);
      for (var i = 0; i < keys.length; i++) {
        var k = keys[i];
        if (k) this.setHeader(k, obj[k]);
      }
    }
    // only progressive api is used
    args.headers = this._renderHeaders();
  } else {
    // only writeHead() called
    args.headers = obj || {};
  }


  this.emit("before-flushing-head", args);

  this.statusMessage = args.statusMessage;
  this.statusCode = args.statusCode;
  headers = args.headers;

  var statusLine = 'HTTP/1.1 ' + statusCode.toString() + ' ' +
                   this.statusMessage + CRLF;

  if (statusCode === 204 || statusCode === 304 ||
      (100 <= statusCode && statusCode <= 199)) {
    // RFC 2616, 10.2.5:
    // The 204 response MUST NOT include a message-body, and thus is always
    // terminated by the first empty line after the header fields.
    // RFC 2616, 10.3.5:
    // The 304 response MUST NOT contain a message-body, and thus is always
    // terminated by the first empty line after the header fields.
    // RFC 2616, 10.1 Informational 1xx:
    // This class of status code indicates a provisional response,
    // consisting only of the Status-Line and optional headers, and is
    // terminated by an empty line.
    this._hasBody = false;
  }

  // don't keep alive connections where the client expects 100 Continue
  // but we sent a final status; they may put extra bytes on the wire.
  if (this._expect_continue && !this._sent100) {
    this.shouldKeepAlive = false;
  }

  this._storeHeader(statusLine, headers);
};

@jasnell
Copy link
Member

jasnell commented Nov 16, 2015

@Fishrock123 @yoavaa @thefourtheye ... any further thoughts on this?

@jasnell jasnell added the stalled Issues and PRs that are stalled. label Nov 16, 2015
@jasnell
Copy link
Member

jasnell commented Nov 16, 2015

@nodejs/http

@indutny
Copy link
Member

indutny commented Nov 16, 2015

I like the idea, and I have seen some use cases for this myself. The only minor question that I have is about event name. We are definitely using camel case for them in core, should we consider doing it the same way here as well?

@jasnell
Copy link
Member

jasnell commented Nov 16, 2015

+1 for consistency

@yoavaa
Copy link
Author

yoavaa commented Nov 16, 2015

@indutny so we should rename the event to 'beforeFlushingHead'?
do you prefer using a different event name?

@indutny
Copy link
Member

indutny commented Nov 16, 2015

I'm not a native speaker, but it looks good to me. cc @jasnell ?

@jasnell
Copy link
Member

jasnell commented Nov 16, 2015

Works for me.

@yoavaa
Copy link
Author

yoavaa commented Nov 16, 2015

renamed the event.

var common = require('../common');
var assert = require('assert');
var http = require('http');

Copy link
Member

Choose a reason for hiding this comment

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

Please use const for requires

@jasnell
Copy link
Member

jasnell commented Nov 16, 2015

Few additional nits.

@yoavaa
Copy link
Author

yoavaa commented Dec 8, 2015

ok, I have addressed the code formatting issues above and found and (and fixed) a few bugs, as well as updated the docs.

@viliusl
Copy link

viliusl commented Mar 4, 2016

+1

@yoavaa
Copy link
Author

yoavaa commented Mar 4, 2016

@jasnell, can we proceed with merging this pull request.

@jasnell
Copy link
Member

jasnell commented Mar 4, 2016

@yoavaa
Copy link
Author

yoavaa commented Mar 6, 2016

@jasnell Just did rebase, and tests are green

@jasnell
Copy link
Member

jasnell commented Mar 7, 2016

@nodejs/http @nodejs/ctc ... any additional comments on this one?

@trevnorris
Copy link
Contributor

@yoavaa Can you clarify your definition of "time to first byte"? I can think of several ways to define this that run in contradiction to this PR.

@yoavaa
Copy link
Author

yoavaa commented Mar 20, 2016

@trevnorris I agree that measuring time to first byte, when talking about it in most cases, refers to the client (browser or another http client) measuring the time from issuing the request until it gets back the first byte of response. This time normally includes DNS lookup, network latency, server request queuing and server request processing.

What I refer to, in this case, is the server view of time to first byte - that it, the time between getting the request (user code) and the time the first bytes are sent out of the server (again, user code). This metric is useful to separate the impact of a user code from the rest of the variables that are less under control (like geo latency, network latency, DNS, etc).

This PR gives a notification just before node writes the HTTP head, it is a good indication of the time to first byte sent by node (ignoring any network buffers on the node side).

Does that makes sense?

@jasnell
Copy link
Member

jasnell commented Apr 4, 2016

Thinking about this further in relation to #1873.
The one limitation of this is that is misses the on-write headers that are added in storeHeaders (things like Date, etc).

@yoavaa
Copy link
Author

yoavaa commented Apr 14, 2016

@jasnell, I have looked into changing the implementation to support the headers added by _storeHeaders as well. It can be done, but before we do so, lets make sure this is what we want.

  1. to support the headers added by _storeHeaders, we have to emit the event from inside this method - end of _storeHeaders, or in the beginning of _send.

The second option is actually the better because it ensures that the event is emitted exactly when needed - just before sending the body.

  1. The _storeHeaders function is called from both _http_client and _http_server. I guess it is a good thing that we get the event for both cases (client making HTTP request, server responding to an HTTP request).
  2. The _send method gets the headers as a string stored on this._header. In order to support the event with modifications of the headers, we will need to change the type of this._header into an object like
{
  statusCode: 200,
  statusLine: "---",
  headers: [
    [name, value]
  ]
}

so, what do you think? should we make this change?

@jasnell
Copy link
Member

jasnell commented Apr 15, 2016

Yeah, I was looking at similar kinds of changes and decided to hold off. We'd need to get a good understanding of what the performance impact of the changes would be as this particular bit of code is in the hot path for many users. Would love to get input from @indutny and @mscdex on this also.

@yoavaa
Copy link
Author

yoavaa commented Apr 17, 2016

Note sure there will be a major performance impact.
After all, in writeHead and _storeHeader we format the headers, status code and status message to string. What we propose is to delay the rendering to string to the _send function, until after the event.

But we do have a side effect of including outgoing messages from HTTP client as well - requiring there a similar change as well.

@jasnell
Copy link
Member

jasnell commented Feb 28, 2017

@yoavaa ... is this still something you'd like to pursue?

@jasnell
Copy link
Member

jasnell commented Mar 24, 2017

Closing due to lack of forward progress on this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. http Issues or PRs related to the http subsystem. semver-minor PRs that contain new features and should be released in the next minor version. stalled Issues and PRs that are stalled.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants