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(instrumentation-fetch): fetch(string, Request) silently drops request body #2411

Merged
merged 3 commits into from
Aug 14, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 8 additions & 13 deletions packages/opentelemetry-instrumentation-fetch/src/fetch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -284,23 +284,18 @@ export class FetchInstrumentation extends InstrumentationBase<
/**
* Patches the constructor of fetch
*/
private _patchConstructor(): (
original: (input: RequestInfo, init?: RequestInit) => Promise<Response>
) => (input: RequestInfo, init?: RequestInit) => Promise<Response> {
return (
original: (input: RequestInfo, init?: RequestInit) => Promise<Response>
): ((input: RequestInfo, init?: RequestInit) => Promise<Response>) => {
private _patchConstructor(): (original: Window['fetch']) => Window['fetch'] {
return original => {
const plugin = this;
return function patchConstructor(
this: (input: RequestInfo, init?: RequestInit) => Promise<Response>,
input: RequestInfo,
init?: RequestInit
this: Window,
...args: Parameters<Window['fetch']>
): Promise<Response> {
const url = input instanceof Request ? input.url : input;
const options = input instanceof Request ? input : init || {};
const url = args[0] instanceof Request ? args[0].url : args[0];
const options = args[0] instanceof Request ? args[0] : args[1] || {};
const createdSpan = plugin._createSpan(url, options);
if (!createdSpan) {
return original.apply(this, [url, options]);
return original.apply(this, args);
}
const spanData = plugin._prepareSpanData(url);

Expand Down Expand Up @@ -380,7 +375,7 @@ export class FetchInstrumentation extends InstrumentationBase<
plugin._addHeaders(options, url);
plugin._tasksCount++;
return original
.apply(this, [url, options])
.apply(this, options instanceof Request ? [options] : [url, options])
dyladan marked this conversation as resolved.
Show resolved Hide resolved
.then(
(onSuccess as any).bind(this, createdSpan, resolve),
onError.bind(this, createdSpan, reject)
Expand Down
26 changes: 24 additions & 2 deletions packages/opentelemetry-instrumentation-fetch/test/fetch.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -176,11 +176,16 @@ describe('fetch', () => {
};
response.headers = Object.assign({}, init.headers);

if (init.method === 'DELETE') {
if (init instanceof Request) {
// Passing request as 2nd argument causes missing body bug (#2411)
response.status = 400;
response.statusText = 'Bad Request (Request object as 2nd argument)';
reject(new window.Response(JSON.stringify(response), response));
} else if (init.method === 'DELETE') {
response.status = 405;
response.statusText = 'OK';
resolve(new window.Response('foo', response));
} else if (input === url) {
} else if ((input instanceof Request && input.url === url) || input === url) {
response.status = 200;
response.statusText = 'OK';
resolve(new window.Response(JSON.stringify(response), response));
Expand Down Expand Up @@ -530,6 +535,15 @@ describe('fetch', () => {
assert.ok(typeof r.headers.get(X_B3_TRACE_ID) === 'string');
});

it('should pass request object as first parameter to the original function (#2411)', () => {
const r = new Request(url);
return window.fetch(r).then(() => {
assert.ok(true);
}, (response: Response) => {
assert.fail(response.statusText);
});
});

it('should NOT clear the resources', () => {
assert.strictEqual(
clearResourceTimingsSpy.args.length,
Expand Down Expand Up @@ -659,6 +673,14 @@ describe('fetch', () => {
it('should NOT create any span', () => {
assert.strictEqual(exportSpy.args.length, 0, "span shouldn't b exported");
});
it('should pass request object as the first parameter to the original function (#2411)', () => {
const r = new Request(url);
return window.fetch(r).then(() => {
assert.ok(true);
}, (response: Response) => {
assert.fail(response.statusText);
});
});
});

describe('when clearTimingResources is TRUE', () => {
Expand Down