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: use socket from the request #1939

Merged
merged 1 commit into from
Feb 23, 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
5 changes: 4 additions & 1 deletion packages/opentelemetry-instrumentation-http/src/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -465,7 +465,10 @@ export const getIncomingRequestAttributesOnResponse = (
request: IncomingMessage & { __ot_middlewares?: string[] },
response: ServerResponse & { socket: Socket }
): SpanAttributes => {
const { statusCode, statusMessage, socket } = response;
// take socket from the request,
// since it may be detached from the response object in keep-alive mode
const { socket } = request;
const { statusCode, statusMessage } = response;
const { localAddress, localPort, remoteAddress, remotePort } = socket;
const { __ot_middlewares } = (request as unknown) as {
[key: string]: unknown;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,7 @@ describe('Utility', () => {
it('should correctly parse the middleware stack if present', () => {
const request = {
__ot_middlewares: ['/test', '/toto', '/'],
socket: {},
} as IncomingMessage & { __ot_middlewares?: string[] };

const attributes = utils.getIncomingRequestAttributesOnResponse(request, {
Expand All @@ -300,7 +301,9 @@ describe('Utility', () => {
});

it('should succesfully process without middleware stack', () => {
const request = {} as IncomingMessage;
const request = {
socket: {},
} as IncomingMessage;
const attributes = utils.getIncomingRequestAttributesOnResponse(request, {
socket: {},
} as ServerResponse & { socket: Socket });
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import * as http from 'http';
import { httpRequest } from '../utils/httpRequest';
import { DummyPropagation } from '../utils/DummyPropagation';
import { Socket } from 'net';
import { sendRequestTwice } from '../utils/rawRequest';

const protocol = 'http';
const serverPort = 32345;
Expand Down Expand Up @@ -345,5 +346,14 @@ describe('HttpInstrumentation Integration tests', () => {
});
});
}

it('should work for multiple active requests in keep-alive mode', async () => {
await sendRequestTwice(hostname, mockServerPort);
const spans = memoryExporter.getFinishedSpans();
const span = spans.find((s: any) => s.kind === SpanKind.SERVER);
assert.ok(span);
assert.strictEqual(spans.length, 2);
assert.strictEqual(span.name, 'HTTP GET');
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
/*
* Copyright The OpenTelemetry Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
import * as net from 'net';

// used to reproduce this issue:
// https://github.com/open-telemetry/opentelemetry-js/pull/1939
export async function sendRequestTwice(
host: string,
port: number
): Promise<Buffer> {
return new Promise((resolve, reject) => {
const request = 'GET /raw HTTP/1.1\n\n';
const socket = net.createConnection({ host, port }, () => {
socket.write(`${request}${request}`, err => {
if (err) reject(err);
});
});
socket.on('data', data => {
// since it's <1kb size, we expect both responses to come in a single chunk
socket.destroy();
resolve(data);
});
socket.on('error', err => reject(err));
});
}
8 changes: 5 additions & 3 deletions packages/opentelemetry-plugin-http/src/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ import {
RequestOptions,
ServerResponse,
} from 'http';
import { Socket } from 'net';
import * as url from 'url';
import { Err, IgnoreMatcher, ParsedRequestOptions } from './types';

Expand Down Expand Up @@ -463,9 +462,12 @@ export const getIncomingRequestAttributes = (
*/
export const getIncomingRequestAttributesOnResponse = (
request: IncomingMessage & { __ot_middlewares?: string[] },
response: ServerResponse & { socket: Socket }
response: ServerResponse
): SpanAttributes => {
const { statusCode, statusMessage, socket } = response;
// use socket from the request,
// since it may be detached from the response object in keep-alive mode
const { socket } = request;
const { statusCode, statusMessage } = response;
const { localAddress, localPort, remoteAddress, remotePort } = socket;
const { __ot_middlewares } = (request as unknown) as {
[key: string]: unknown;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import { HttpAttribute } from '@opentelemetry/semantic-conventions';
import * as assert from 'assert';
import * as http from 'http';
import { IncomingMessage, ServerResponse } from 'http';
import { Socket } from 'net';
import * as sinon from 'sinon';
import * as url from 'url';
import { IgnoreMatcher } from '../../src/types';
Expand Down Expand Up @@ -291,19 +290,23 @@ describe('Utility', () => {
it('should correctly parse the middleware stack if present', () => {
const request = {
__ot_middlewares: ['/test', '/toto', '/'],
} as IncomingMessage & { __ot_middlewares?: string[] };

const attributes = utils.getIncomingRequestAttributesOnResponse(request, {
socket: {},
} as ServerResponse & { socket: Socket });
} as IncomingMessage & { __ot_middlewares?: string[] };
const response = {} as ServerResponse;
const attributes = utils.getIncomingRequestAttributesOnResponse(
request,
response
);
assert.deepEqual(attributes[HttpAttribute.HTTP_ROUTE], '/test/toto');
});

it('should succesfully process without middleware stack', () => {
const request = {} as IncomingMessage;
const attributes = utils.getIncomingRequestAttributesOnResponse(request, {
socket: {},
} as ServerResponse & { socket: Socket });
const request = { socket: {} } as IncomingMessage;
const response = {} as ServerResponse;
const attributes = utils.getIncomingRequestAttributesOnResponse(
request,
response
);
assert.deepEqual(attributes[HttpAttribute.HTTP_ROUTE], undefined);
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import {
import { HttpPluginConfig } from '../../src/types';
import { AsyncHooksContextManager } from '@opentelemetry/context-async-hooks';
import { Socket } from 'net';
import { sendRequestTwice } from '../utils/rawRequest';
const protocol = 'http';
const serverPort = 32345;
const hostname = 'localhost';
Expand Down Expand Up @@ -337,5 +338,14 @@ describe('HttpPlugin Integration tests', () => {
});
});
}

it('should work for multiple active requests in keep-alive mode', async () => {
await sendRequestTwice(hostname, mockServerPort);
const spans = memoryExporter.getFinishedSpans();
const span = spans.find((s: any) => s.kind === SpanKind.SERVER);
assert.ok(span);
assert.strictEqual(spans.length, 2);
assert.strictEqual(span.name, 'HTTP GET');
});
});
});
38 changes: 38 additions & 0 deletions packages/opentelemetry-plugin-http/test/utils/rawRequest.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
/*
* Copyright The OpenTelemetry Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
import * as net from 'net';

// used to reproduce this issue:
// https://github.com/open-telemetry/opentelemetry-js/pull/1939
export async function sendRequestTwice(
host: string,
port: number
): Promise<Buffer> {
return new Promise((resolve, reject) => {
const request = 'GET /raw HTTP/1.1\n\n';
const socket = net.createConnection({ host, port }, () => {
socket.write(`${request}${request}`, err => {
if (err) reject(err);
});
});
socket.on('data', data => {
// since it's <1kb size, we expect both responses to come in a single chunk
socket.destroy();
resolve(data);
});
socket.on('error', err => reject(err));
});
}