Skip to content

feat(instrumentation-tls): wrap tls.connect API #447

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

Merged
merged 19 commits into from
May 7, 2021
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
1ff9f9d
feat(instrumentation-tls): wrap tls.connect API
svrnm Apr 23, 2021
54315fa
Merge branch 'main' into tls-library-instrumentation
svrnm Apr 23, 2021
326d0a8
style: remove unused import for connect.test.ts in tls plugin
svrnm Apr 23, 2021
dddc0fc
Merge branch 'tls-library-instrumentation' of github.com:svrnm/opente…
svrnm Apr 23, 2021
baeb05d
Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…
svrnm Apr 26, 2021
c5d9f44
Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…
svrnm Apr 26, 2021
81bd589
feat(instrumentation-tls): move tls instrumentation into net package
svrnm Apr 26, 2021
5588ff0
feat(instrumentation-tls): remove comment in net.ts
svrnm Apr 27, 2021
2a5f840
Merge branch 'main' into tls-library-instrumentation
svrnm Apr 28, 2021
67b52d4
Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…
svrnm Apr 30, 2021
4d9fdad
feat(instrumentation-tls): fix stalled tests
svrnm Apr 30, 2021
01c13d4
Merge branch 'tls-library-instrumentation' of github.com:svrnm/opente…
svrnm Apr 30, 2021
198dbee
feat(instrumentation-tls): restructure tls tests and fix tests for al…
svrnm Apr 30, 2021
d6a3f50
feat(instrumentation-tls): fix style
svrnm Apr 30, 2021
6b40e75
feat(instrumentation-tls): add comment that TLSAttributes are not off…
svrnm Apr 30, 2021
1353d56
Merge branch 'main' into tls-library-instrumentation
svrnm May 4, 2021
08ac436
Merge branch 'main' into tls-library-instrumentation
svrnm May 5, 2021
62def9f
Merge branch 'main' into tls-library-instrumentation
svrnm May 5, 2021
9e9c55d
Merge branch 'main' into tls-library-instrumentation
svrnm May 6, 2021
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
51 changes: 51 additions & 0 deletions examples/network/client.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
'use strict';

const { diag, DiagConsoleLogger, DiagLogLevel } = require('@opentelemetry/api');
const { NodeTracerProvider } = require('@opentelemetry/node');
const { HttpInstrumentation } = require('@opentelemetry/instrumentation-http');
const { NetInstrumentation } = require('@opentelemetry/instrumentation-net');
const { DnsInstrumentation } = require('@opentelemetry/instrumentation-dns');
const { registerInstrumentations } = require('@opentelemetry/instrumentation');
const { SimpleSpanProcessor, ConsoleSpanExporter } = require('@opentelemetry/tracing');
const { JaegerExporter } = require('@opentelemetry/exporter-jaeger');

const provider = new NodeTracerProvider();

provider.addSpanProcessor(new SimpleSpanProcessor(new JaegerExporter({
serviceName: 'http-client',
})));

provider.addSpanProcessor(new SimpleSpanProcessor(new ConsoleSpanExporter()));

provider.register();

diag.setLogger(new DiagConsoleLogger(), DiagLogLevel.ALL);

registerInstrumentations({
instrumentations: [
new NetInstrumentation(),
new HttpInstrumentation(),
new DnsInstrumentation({
// Avoid dns lookup loop with http zipkin calls
ignoreHostnames: ['localhost'],
}),
],
tracerProvider: provider,
});

require('net');
require('dns');
const https = require('https');
const http = require('http');

http.get('http://opentelemetry.io/', () => {}).on('error', (e) => {
console.error(e);
});

https.get('https://opentelemetry.io/', () => {}).on('error', (e) => {
console.error(e);
});

https.get('https://opentelemetry.io/', { ca: [] }, () => {}).on('error', (e) => {
console.error(e);
});
44 changes: 44 additions & 0 deletions examples/network/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
{
"name": "tls-example",
"private": true,
"version": "0.15.0",
"description": "Example of NET & TLS integration with OpenTelemetry",
"main": "index.js",
"scripts": {
"zipkin:client": "cross-env EXPORTER=zipkin node ./client.js",
"jaeger:client": "cross-env EXPORTER=jaeger node ./client.js"
},
"repository": {
"type": "git",
"url": "git+ssh://[email protected]/open-telemetry/opentelemetry-js-contrib.git"
},
"keywords": [
"opentelemetry",
"net",
"tls",
"tracing"
],
"engines": {
"node": ">=8.5.0"
},
"author": "OpenTelemetry Authors",
"license": "Apache-2.0",
"bugs": {
"url": "https://github.com/open-telemetry/opentelemetry-js-contrib/issues"
},
"dependencies": {
"@opentelemetry/api": "^0.18.1",
"@opentelemetry/exporter-jaeger": "^0.18.2",
"@opentelemetry/exporter-zipkin": "^0.18.2",
"@opentelemetry/instrumentation": "^0.18.2",
"@opentelemetry/instrumentation-net": "file:../../plugins/node/opentelemetry-instrumentation-net",
"@opentelemetry/instrumentation-http": "^0.19.0",
"@opentelemetry/instrumentation-dns": "^0.15.0",
"@opentelemetry/node": "^0.18.2",
"@opentelemetry/tracing": "^0.18.2"
},
"homepage": "https://github.com/open-telemetry/opentelemetry-js-contrib#readme",
"devDependencies": {
"cross-env": "^6.0.3"
}
}
67 changes: 61 additions & 6 deletions plugins/node/opentelemetry-instrumentation-net/src/net.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,11 @@ import {
SemanticAttributes,
NetTransportValues,
} from '@opentelemetry/semantic-conventions';
import { Net, NormalizedOptions, SocketEvent } from './types';
import { Net, NormalizedOptions, SocketEvent, TLSAttributes } from './types';
import { getNormalizedArgs, IPC_TRANSPORT } from './utils';
import { VERSION } from './version';
import { Socket } from 'net';
import { TLSSocket } from 'tls';

export class NetInstrumentation extends InstrumentationBase<Net> {
constructor(protected _config: InstrumentationConfig = {}) {
Expand Down Expand Up @@ -69,11 +70,10 @@ export class NetInstrumentation extends InstrumentationBase<Net> {
return function patchedConnect(this: Socket, ...args: unknown[]) {
const options = getNormalizedArgs(args);

const span = options
? options.path
? plugin._startIpcSpan(options, this)
: plugin._startTcpSpan(options, this)
: plugin._startGenericSpan(this);
const span =
this instanceof TLSSocket
? plugin._startTLSSpan(options, this)
: plugin._startSpan(options, this);

return safeExecuteInTheMiddle(
() => original.apply(this, args),
Expand All @@ -92,6 +92,61 @@ export class NetInstrumentation extends InstrumentationBase<Net> {
};
}

private _startSpan(
options: NormalizedOptions | undefined | null,
socket: Socket
) {
if (!options) {
return this._startGenericSpan(socket);
}
if (options.path) {
return this._startIpcSpan(options, socket);
}
return this._startTcpSpan(options, socket);
}

private _startTLSSpan(
options: NormalizedOptions | undefined | null,
socket: TLSSocket
) {
const tlsSpan = this.tracer.startSpan('tls.connect');

const netSpan = this._startSpan(options, socket);

/* if we use once and tls.connect() uses a callback this is never executed */
socket.prependOnceListener(SocketEvent.SECURE_CONNECT, () => {
const peerCertificate = socket.getPeerCertificate(true);
const cipher = socket.getCipher();
const protocol = socket.getProtocol();
const attributes = {
[TLSAttributes.PROTOCOL]: String(protocol),
[TLSAttributes.AUTHORIZED]: String(socket.authorized),
[TLSAttributes.CIPHER_NAME]: cipher.name,
[TLSAttributes.CIPHER_VERSION]: cipher.version,
[TLSAttributes.CERTIFICATE_FINGERPRINT]: peerCertificate.fingerprint,
[TLSAttributes.CERTIFICATE_SERIAL_NUMBER]: peerCertificate.serialNumber,
[TLSAttributes.CERTIFICATE_VALID_FROM]: peerCertificate.valid_from,
[TLSAttributes.CERTIFICATE_VALID_TO]: peerCertificate.valid_to,
[TLSAttributes.ALPN_PROTOCOL]: '',
};
if (socket.alpnProtocol) {
attributes[TLSAttributes.ALPN_PROTOCOL] = socket.alpnProtocol;
}

tlsSpan.setAttributes(attributes);
tlsSpan.end();
});
socket.once(SocketEvent.ERROR, (e: Error) => {
tlsSpan.setStatus({
code: SpanStatusCode.ERROR,
message: e.message,
});
tlsSpan.end();
});

return netSpan;
}

/* It might still be useful to pick up errors due to invalid connect arguments. */
private _startGenericSpan(socket: Socket) {
const span = this.tracer.startSpan('connect');
Expand Down
13 changes: 13 additions & 0 deletions plugins/node/opentelemetry-instrumentation-net/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,4 +28,17 @@ export enum SocketEvent {
CLOSE = 'close',
CONNECT = 'connect',
ERROR = 'error',
SECURE_CONNECT = 'secureConnect',
}

export enum TLSAttributes {
Copy link
Member

Choose a reason for hiding this comment

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

Are any of these in the Semantic Conventions?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not that I am aware of. I checked the specs and didn't find them there

Copy link
Member

Choose a reason for hiding this comment

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

can you add a comment that these are not "official"

we need guidance from the spec how to add attributes that are not yet specified. If the spec eventually adds one of these attributes and it isn't compatible with ours that will be a problem.

@open-telemetry/technical-committee any guidance how to handle this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I was confused. I thought you talk about the socket events, but you mean the TLSAttributes? I actually opened an issue already: open-telemetry/opentelemetry-specification#1652

can you add a comment that these are not "official"

Yes, I can do that.

I guess they are not the only candidates for that, e.g. the DNS package puts the name that was looked up into net.peer.name, which is probably not correct and their might be a semantic convention for DNS.

Since this is kind of related to open-telemetry/opentelemetry-js#2123: Would it help if I scan js and js-contrib (semi)automatically for attributes that are either in the semantic-specs already or that might be candidates for it?

Copy link
Member

Choose a reason for hiding this comment

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

that would be great

Copy link
Member Author

Choose a reason for hiding this comment

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

@dyladan I found a bunch of them. There's four different cases:

  1. Use of existing semantic conventions (like PG, see Fix semantic conventions for PG and PG Pool #467)
  2. Use of potentially new semantic conventions, I mean things that are used outside of node.js as well (DNS, TLS, graphQL)
  3. Use of custom attributes specific to the instrumented library (express, hapi, restify)
  4. A corner case are browser specific attributes: while those are "unique" to javascript I would still argue that they should be covered by a semantic convention, since they are still universal

Since this is a discussion unrelated to this ticket, can you give me some guidance where to move this discussion? Open another issue with the specification repo?

Copy link
Member

Choose a reason for hiding this comment

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

I would open an issue on this repo to track it at least. If you think some should be spec'd then you should open an issue on the spec repo too and link them.

PROTOCOL = 'tls.protocol',
AUTHORIZED = 'tls.authorized',
CIPHER_NAME = 'tls.cipher.name',
CIPHER_VERSION = 'tls.cipher.version',
CERTIFICATE_FINGERPRINT = 'tls.certificate.fingerprint',
CERTIFICATE_SERIAL_NUMBER = 'tls.certificate.serialNumber',
CERTIFICATE_VALID_FROM = 'tls.certificate.validFrom',
CERTIFICATE_VALID_TO = 'tls.certificate.validTo',
ALPN_PROTOCOL = 'tls.alpnProtocol',
}
113 changes: 112 additions & 1 deletion plugins/node/opentelemetry-instrumentation-net/test/connect.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,20 @@ import { SemanticAttributes } from '@opentelemetry/semantic-conventions';
import { NodeTracerProvider } from '@opentelemetry/node';
import * as net from 'net';
import * as assert from 'assert';
import * as tls from 'tls';
import { NetInstrumentation } from '../src/net';
import { SocketEvent } from '../src/types';
import { assertIpcSpan, assertTcpSpan, IPC_PATH, HOST, PORT } from './utils';
import {
assertIpcSpan,
assertTcpSpan,
assertTLSSpan,
IPC_PATH,
HOST,
PORT,
TLS_SERVER_CERT,
TLS_SERVER_KEY,
TLS_PORT,
} from './utils';

const memoryExporter = new InMemorySpanExporter();
const provider = new NodeTracerProvider();
Expand All @@ -38,11 +49,22 @@ function getSpan() {
return span;
}

function getTLSSpans() {
const spans = memoryExporter.getFinishedSpans();
assert.strictEqual(spans.length, 2);
const [netSpan, tlsSpan] = spans;
return {
netSpan,
tlsSpan,
};
}

describe('NetInstrumentation', () => {
let instrumentation: NetInstrumentation;
let socket: net.Socket;
let tcpServer: net.Server;
let ipcServer: net.Server;
let tlsServer: tls.Server;

before(() => {
instrumentation = new NetInstrumentation();
Expand All @@ -60,6 +82,15 @@ describe('NetInstrumentation', () => {
ipcServer.listen(IPC_PATH, done);
});

before(done => {
tlsServer = tls.createServer({
cert: TLS_SERVER_CERT,
key: TLS_SERVER_KEY,
});

tlsServer.listen(TLS_PORT, done);
});

beforeEach(() => {
socket = new net.Socket();
});
Expand All @@ -73,6 +104,7 @@ describe('NetInstrumentation', () => {
instrumentation.disable();
tcpServer.close();
ipcServer.close();
tlsServer.close();
});

describe('successful net.connect produces a span', () => {
Expand Down Expand Up @@ -162,6 +194,67 @@ describe('NetInstrumentation', () => {
});
});

describe('successful tls.connect produces a span', () => {
it('should produce a span with "onSecure" callback', done => {
const tlsSocket = tls.connect(
TLS_PORT,
HOST,
{
ca: [TLS_SERVER_CERT],
checkServerIdentity: () => {
return undefined;
},
},
() => {
assertTLSSpan(getTLSSpans(), tlsSocket);
done();
// This needs to be here to make sure that mocha can close cleanly.
tlsSocket.destroy();
}
);
});

it('should produce a span without "onSecure" callback', done => {
socket = tls.connect(TLS_PORT, HOST, {
ca: [TLS_SERVER_CERT],
checkServerIdentity: () => {
return undefined;
},
});
tlsServer.on('connection', c => {
c.end();
});
socket.on('end', () => {
assertTLSSpan(getTLSSpans(), socket);
done();
});
});

it('should produce an error span when certificate is not trusted', done => {
socket = tls.connect(
TLS_PORT,
HOST,
{
ca: [],
checkServerIdentity: () => {
return undefined;
},
},
() => {
assertTLSSpan(getTLSSpans(), socket);
done();
}
);
socket.on('error', error => {
const { tlsSpan } = getTLSSpans();
// assertTcpSpan(netSpan, tlsSocket, TLS_PORT)
assert.strictEqual(tlsSpan.status.message, 'self signed certificate');
assert.strictEqual(tlsSpan.status.code, SpanStatusCode.ERROR);
done();
});
});
});

describe('invalid input', () => {
it('should produce an error span when connect throws', done => {
assert.throws(() => {
Expand Down Expand Up @@ -195,6 +288,7 @@ describe('NetInstrumentation', () => {
SocketEvent.CLOSE,
SocketEvent.CONNECT,
SocketEvent.ERROR,
SocketEvent.SECURE_CONNECT,
]) {
assert.equal(events.has(event), false);
}
Expand Down Expand Up @@ -226,5 +320,22 @@ describe('NetInstrumentation', () => {
});
});
});

it('should clean up listeners for tls.connect', done => {
tls.connect(
TLS_PORT,
HOST,
{
ca: [TLS_SERVER_CERT],
checkServerIdentity: () => {
return undefined;
},
},
() => {
assertNoDanglingListeners();
done();
}
);
});
});
});
Loading