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

fix: inject context http headers early if expect header exists #766

Merged
merged 3 commits into from Jun 2, 2018

Conversation

Projects
None yet
3 participants
@kjin
Copy link
Contributor

commented Jun 1, 2018

Fixes #763

This PR fixes a bug where our HTTP instrumentation fails if Expect is added as an outgoing header. This is because we normally inject a trace context header for a request as part of our HTTP instrumentation, but Node has a special case where it prohibits additional headers if the Expect header is detected. See: https://github.com/nodejs/node/blob/v7.10.1/lib/_http_outgoing.js#L323

To fix this, I brought back the old "slower" way of injecting trace context headers that was removed in #643, but only if the Expect header is detected.

I also added a test that fails without the source change.

@kjin kjin requested review from ofrobots, jinwoo and DominicKramer Jun 1, 2018

@googlebot googlebot added the cla: yes label Jun 1, 2018

@jinwoo
Copy link
Member

left a comment

Do you know why Node throws an error in that case?

* @param options Options for http.request.
*/
function hasExpectHeader(options: ClientRequestArgs|
url.URL): options is ClientRequestArgs {

This comment has been minimized.

Copy link
@jinwoo

jinwoo Jun 1, 2018

Member

Should the return type a boolean?

This comment has been minimized.

Copy link
@kjin

kjin Jun 1, 2018

Author Contributor

A is B is already a boolean.

This comment has been minimized.

Copy link
@jinwoo

jinwoo Jun 1, 2018

Member

Yeah, I know. What I mean here is that this function does NOT test that options is a ClientRequestArgs object but rather that it is a one AND it has an Expect header. Right?

This comment has been minimized.

Copy link
@jinwoo

jinwoo Jun 1, 2018

Member

Forgot to mention that this makes the compiler make bad decisions about type info. E.g.

const options = ...;
...
if (hasExpectHeader(options)) {
  // Compiler thinks `options` is a ClientRequestArgs, which is correct.
} else {
  // Compiler thinks `options` is NOT a ClientRequestArgs, which may or may not be correct.
}
function hasExpectHeader(options: ClientRequestArgs|
url.URL): options is ClientRequestArgs {
// tslint:disable-next-line:no-any
return !!((options as any).headers && (options as any).headers.Expect);

This comment has been minimized.

Copy link
@jinwoo

jinwoo Jun 1, 2018

Member

I think it makes more sense to type-cast as ClientRequestArgs rather than as any.

This comment has been minimized.

Copy link
@kjin

kjin Jun 1, 2018

Author Contributor

Done

traceHeaderPreinjected = true;
// "Clone" the options object -- but don't deep-clone anything except for
// headers.
options = Object.assign({}, options);

This comment has been minimized.

Copy link
@jinwoo

jinwoo Jun 1, 2018

Member

Nit. options = {...options}; would be simpler.

This comment has been minimized.

Copy link
@kjin

kjin Jun 1, 2018

Author Contributor

I think ... syntax isn't available in Node 4?

This comment has been minimized.

Copy link
@jinwoo

jinwoo Jun 1, 2018

Member

I believe TS transpiles it to what you wrote here.

@kjin

This comment has been minimized.

Copy link
Contributor Author

commented Jun 1, 2018

@jinwoo I'm not sure. It could be unintentional -- I think I need to understand more about the Expect header first.

@@ -49,7 +49,9 @@ function getSpanName(options: ClientRequestArgs|url.URL) {
function hasExpectHeader(options: ClientRequestArgs|
url.URL): options is ClientRequestArgs {
// tslint:disable-next-line:no-any

This comment has been minimized.

Copy link
@jinwoo

jinwoo Jun 1, 2018

Member

You don't need this anymore.

@kjin

This comment has been minimized.

Copy link
Contributor Author

commented Jun 1, 2018

@jinwoo Seems like the behavior for a long time (since Node 0.3) has been that Node eagerly sends headers when Expect exists. https://github.com/nodejs/node/blob/d59512f6f406ceae4940fd9c83e8255f0c03173b/lib/http.js#L463

@kjin kjin force-pushed the kjin:http-expect branch from 83ddee3 to 15b6157 Jun 1, 2018

@jinwoo

This comment has been minimized.

Copy link
Member

commented Jun 1, 2018

That makes sense because Expect means that the client is expecting a 100 response before sending the request body.

@kjin kjin force-pushed the kjin:http-expect branch from da6d1c4 to 0b12456 Jun 2, 2018

@jinwoo

jinwoo approved these changes Jun 2, 2018

@kjin kjin merged commit bc877a5 into googleapis:master Jun 2, 2018

7 checks passed

ci/circleci: node4 Your tests passed on CircleCI!
Details
ci/circleci: node6 Your tests passed on CircleCI!
Details
ci/circleci: node8 Your tests passed on CircleCI!
Details
ci/circleci: node9 Your tests passed on CircleCI!
Details
cla/google All necessary CLAs are signed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
security/snyk No dependency changes
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.