fix: inject context http headers early if expect header exists#766
fix: inject context http headers early if expect header exists#766kjin merged 3 commits intogoogleapis:masterfrom
Conversation
googlebot
added
the
cla: yes
jinwoo
left a comment
jinwoo
left a comment
There was a problem hiding this 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 was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| 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 was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| traceHeaderPreinjected = true; | ||
| // "Clone" the options object -- but don't deep-clone anything except for | ||
| // headers. | ||
| options = Object.assign({}, options); |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
@jinwoo I'm not sure. It could be unintentional -- I think I need to understand more about the |
| @@ -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 was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
@jinwoo Seems like the behavior for a long time (since Node 0.3) has been that Node eagerly sends headers when |
|
That makes sense because |
3 participants
Fixes #763
This PR fixes a bug where our HTTP instrumentation fails if
Expectis 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 theExpectheader is detected. See: https://github.com/nodejs/node/blob/v7.10.1/lib/_http_outgoing.js#L323To fix this, I brought back the old "slower" way of injecting trace context headers that was removed in #643, but only if the
Expectheader is detected.I also added a test that fails without the source change.