From 0212deac36cd499d3ac430cd967d3746ce3d6b88 Mon Sep 17 00:00:00 2001 From: Mark Wolff Date: Wed, 9 Oct 2019 13:35:38 -0700 Subject: [PATCH 01/18] feat(pg): implement postgres plugin --- .circleci/config.yml | 14 + .../package.json | 10 +- .../src/enums.ts | 24 ++ .../src/index.ts | 3 + .../opentelemetry-plugin-postgres/src/pg.ts | 160 ++++++++ .../src/types.ts | 19 + .../test/assertionUtils.ts | 84 +++++ .../test/pg.test.ts | 347 ++++++++++++++++++ .../test/testUtils.ts | 54 +++ 9 files changed, 713 insertions(+), 2 deletions(-) create mode 100644 packages/opentelemetry-plugin-postgres/src/enums.ts create mode 100644 packages/opentelemetry-plugin-postgres/src/pg.ts create mode 100644 packages/opentelemetry-plugin-postgres/src/types.ts create mode 100644 packages/opentelemetry-plugin-postgres/test/assertionUtils.ts create mode 100644 packages/opentelemetry-plugin-postgres/test/pg.test.ts create mode 100644 packages/opentelemetry-plugin-postgres/test/testUtils.ts diff --git a/.circleci/config.yml b/.circleci/config.yml index 208c5f711d..83c1699fc4 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -1,5 +1,11 @@ version: 2 +test_env: &test_env + TEST_POSTGRES: 1 + +postgres_service: &postgres_service + image: postgres:alpine + node_unit_tests: &node_unit_tests steps: - checkout @@ -71,18 +77,26 @@ jobs: node8: docker: - image: node:8 + environment: *test_env + - *postgres_service <<: *node_unit_tests node10: docker: - image: node:10 + environment: *test_env + - *postgres_service <<: *node_unit_tests node11: docker: - image: node:11 + environment: *test_env + - *postgres_service <<: *node_unit_tests node12: docker: - image: node:12 + environment: *test_env + - *postgres_service <<: *node_unit_tests node12-browsers: docker: diff --git a/packages/opentelemetry-plugin-postgres/package.json b/packages/opentelemetry-plugin-postgres/package.json index a0fbb6fd86..8f2aedb037 100644 --- a/packages/opentelemetry-plugin-postgres/package.json +++ b/packages/opentelemetry-plugin-postgres/package.json @@ -7,7 +7,8 @@ "types": "build/src/index.d.ts", "repository": "open-telemetry/opentelemetry-js", "scripts": { - "test": "nyc ts-mocha -p tsconfig.json 'test/**/*.ts'", + "test": "nyc ts-mocha -p tsconfig.json 'test/**/*.test.ts'", + "debug": "ts-mocha --inspect-brk --no-timeouts -p tsconfig.json 'test/**/*.test.ts'", "tdd": "yarn test -- --watch-extensions ts --watch", "clean": "rimraf build/*", "check": "gts check", @@ -42,11 +43,14 @@ "devDependencies": { "@types/mocha": "^5.2.7", "@types/node": "^12.6.9", + "@types/pg": "^7.11.2", + "@types/shimmer": "^1.0.1", "codecov": "^3.5.0", "gts": "^1.1.0", "mocha": "^6.2.0", "nyc": "^14.1.1", "rimraf": "^3.0.0", + "pg": "^7.12.1", "tslint-microsoft-contrib": "^6.2.0", "tslint-consistent-codestyle": "^1.15.1", "ts-mocha": "^6.0.0", @@ -56,6 +60,8 @@ "dependencies": { "@opentelemetry/core": "^0.1.0", "@opentelemetry/node-sdk": "^0.1.0", - "@opentelemetry/types": "^0.1.0" + "@opentelemetry/tracer-basic": "^0.1.0", + "@opentelemetry/types": "^0.1.0", + "shimmer": "^1.2.1" } } diff --git a/packages/opentelemetry-plugin-postgres/src/enums.ts b/packages/opentelemetry-plugin-postgres/src/enums.ts new file mode 100644 index 0000000000..1d3944c172 --- /dev/null +++ b/packages/opentelemetry-plugin-postgres/src/enums.ts @@ -0,0 +1,24 @@ +/*! + * Copyright 2019, 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. + */ + +export enum AttributeNames { + COMPONENT = 'component', + PG_HOST = 'pg.host', + PG_PORT = 'pg.port', + PG_TEXT = 'pg.text', + PG_VALUES = 'pg.values', + PG_PLAN = 'pg.plan', +} diff --git a/packages/opentelemetry-plugin-postgres/src/index.ts b/packages/opentelemetry-plugin-postgres/src/index.ts index ae225f6b52..e4bd892b38 100644 --- a/packages/opentelemetry-plugin-postgres/src/index.ts +++ b/packages/opentelemetry-plugin-postgres/src/index.ts @@ -13,3 +13,6 @@ * See the License for the specific language governing permissions and * limitations under the License. */ + +export * from './pg'; +// export * from './pg-pool'; diff --git a/packages/opentelemetry-plugin-postgres/src/pg.ts b/packages/opentelemetry-plugin-postgres/src/pg.ts new file mode 100644 index 0000000000..6a0763f144 --- /dev/null +++ b/packages/opentelemetry-plugin-postgres/src/pg.ts @@ -0,0 +1,160 @@ +/*! + * Copyright 2019, 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 { BasePlugin } from '@opentelemetry/core'; +import { SpanKind } from '@opentelemetry/types'; +import { AttributeNames } from './enums'; +import { PostgresCallback, PostgresPluginOptions } from './types'; +import * as path from 'path'; +import * as pgTypes from 'pg'; +import * as shimmer from 'shimmer'; + +export class PostgresPlugin extends BasePlugin { + static readonly component = 'pg'; + readonly supportedVersions = ['^7.12.1']; + protected _config: PostgresPluginOptions; + + constructor(readonly moduleName: string, readonly version: string) { + super(); + this._config = {}; + } + + protected patch(): typeof pgTypes { + if (this._moduleExports.Client.prototype.query) { + shimmer.wrap( + this._moduleExports.Client.prototype, + 'query', + this._getClientQueryPatch() as never + ); + } + return this._moduleExports; + } + protected unpatch(): void { + if (this._moduleExports.Client.prototype.query) { + shimmer.unwrap(this._moduleExports.Client.prototype, 'query'); + } + } + + private _getClientQueryPatch() { + const plugin = this; + return (original: typeof pgTypes.Client.prototype.query) => { + plugin._logger.debug( + `Patching ${PostgresPlugin.component}.Client.prototype.query` + ); + return function query(this: pgTypes.Client, ...args: unknown[]) { + // setup span + let callbackProvided: boolean = + args.length > 1 && typeof args[args.length - 1] === 'function'; + const span = plugin._tracer.startSpan( + `${PostgresPlugin.component}.query`, + { + kind: SpanKind.CLIENT, + parent: plugin._tracer.getCurrentSpan() || undefined, + attributes: { + [AttributeNames.COMPONENT]: PostgresPlugin.component, + [AttributeNames.PG_HOST]: (this as any).connectionParameters.host, + [AttributeNames.PG_PORT]: (this as any).connectionParameters.port, + }, + } + ); + + try { + if (typeof args[0] === 'string') { + span.setAttribute(AttributeNames.PG_TEXT, args[0]); + if (args[1] instanceof Array) { + span.setAttribute(AttributeNames.PG_VALUES, args[1]); + if (callbackProvided) { + args[2] = plugin._tracer.bind(args[2]); + } + } else { + if (callbackProvided) { + args[1] = plugin._tracer.bind(args[1]); + } + } + } else { + const config = args[0] as pgTypes.QueryConfig & { + callback?: PostgresCallback; + }; + if (typeof config.name === 'string') { + span.setAttribute(AttributeNames.PG_PLAN, config.name); + } else { + if (typeof config.text === 'string') { + span.setAttribute(AttributeNames.PG_TEXT, config.text); + } + if (config.values instanceof Array) { + span.setAttribute(AttributeNames.PG_VALUES, config.values); + } + } + + if (callbackProvided) { + if (typeof args[1] === 'function') { + args[1] = plugin._tracer.bind(args[1]); + } else if (typeof args[2] === 'function') { + args[2] = plugin._tracer.bind(args[2]); + } + } else if ( + config.callback && + typeof config.callback === 'function' + ) { + callbackProvided = true; + config.callback = plugin._tracer.bind(config.callback); + } + } + } catch (e) { + plugin._logger.warn( + `pg Plugin failed to trace query: error: ${e.message}` + ); + const result = original.apply(this, arguments as any); + span.end(); + return result; + } + + const queryResult = original.apply(this, args as any); + + // No callback was provided, return a promise instead (new as of pg@7.x) + if (!callbackProvided) { + const queryResultPromise = (queryResult as unknown) as Promise< + unknown + >; + return plugin._tracer.bind( + queryResultPromise + .then((result: any) => { + // Return a pass-along promise which ends the span and then goes to user's orig resolvers + return new Promise((resolve, _) => { + span.end(); + resolve(result); + }); + }) + .catch((error: Error) => { + return new Promise((_, reject) => { + span.end(); + reject(error); + }); + }) + ); + } + + // Else a callback was provided, so just return the result + span.end(); + return queryResult; + }; + }; + } +} + +const basedir = path.dirname(require.resolve('pg')); +const version = require(path.join(basedir, '../', 'package.json')).version; +export const plugin = new PostgresPlugin(PostgresPlugin.component, version); diff --git a/packages/opentelemetry-plugin-postgres/src/types.ts b/packages/opentelemetry-plugin-postgres/src/types.ts new file mode 100644 index 0000000000..e74906d6ea --- /dev/null +++ b/packages/opentelemetry-plugin-postgres/src/types.ts @@ -0,0 +1,19 @@ +/*! + * Copyright 2019, 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. + */ + +export interface PostgresPluginOptions {} + +export type PostgresCallback = (err: Error, res: object) => unknown; diff --git a/packages/opentelemetry-plugin-postgres/test/assertionUtils.ts b/packages/opentelemetry-plugin-postgres/test/assertionUtils.ts new file mode 100644 index 0000000000..4276e108b8 --- /dev/null +++ b/packages/opentelemetry-plugin-postgres/test/assertionUtils.ts @@ -0,0 +1,84 @@ +/*! + * Copyright 2019, 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 { SpanKind, Attributes, Event, Span } from '@opentelemetry/types'; +import * as assert from 'assert'; +import { PostgresPlugin } from '../src'; +import { ReadableSpan } from '@opentelemetry/tracer-basic'; +import { + hrTimeToMilliseconds, + hrTimeToMicroseconds, +} from '@opentelemetry/core'; +import { AttributeNames } from '../src/enums'; + +export const assertSpan = ( + span: ReadableSpan, + kind: SpanKind, + attributes: Attributes, + events: Event[] +) => { + assert.strictEqual(span.spanContext.traceId.length, 32); + assert.strictEqual(span.spanContext.spanId.length, 16); + assert.strictEqual(span.kind, kind); + + assert.strictEqual( + span.attributes[AttributeNames.COMPONENT], + PostgresPlugin.component + ); + assert.ok(span.endTime); + assert.strictEqual(span.links.length, 0); + + assert.ok( + hrTimeToMicroseconds(span.startTime) < hrTimeToMicroseconds(span.endTime) + ); + assert.ok(hrTimeToMilliseconds(span.endTime) > 0); + + // attributes + assert.strictEqual( + Object.keys(span.attributes).length, + Object.keys(attributes).length, + 'Should contain same number of attributes' + ); + Object.keys(span.attributes).forEach(attribute => { + assert.deepStrictEqual(span.attributes[attribute], attributes[attribute]); + }); + + // events + assert.strictEqual( + span.events.length, + events.length, + 'Should contain same number of events' + ); + span.events.forEach((_, index) => { + assert.deepStrictEqual(span.events[index], events[index]); + }); +}; + +// Check if sourceSpan was propagated to targetSpan +export const assertPropagation = ( + childSpan: ReadableSpan, + parentSpan: Span +) => { + const targetSpanContext = childSpan.spanContext; + const sourceSpanContext = parentSpan.context(); + assert.strictEqual(targetSpanContext.traceId, sourceSpanContext.traceId); + assert.strictEqual(childSpan.parentSpanId, sourceSpanContext.spanId); + assert.strictEqual( + targetSpanContext.traceFlags, + sourceSpanContext.traceFlags + ); + assert.notStrictEqual(targetSpanContext.spanId, sourceSpanContext.spanId); +}; diff --git a/packages/opentelemetry-plugin-postgres/test/pg.test.ts b/packages/opentelemetry-plugin-postgres/test/pg.test.ts new file mode 100644 index 0000000000..072420dce8 --- /dev/null +++ b/packages/opentelemetry-plugin-postgres/test/pg.test.ts @@ -0,0 +1,347 @@ +/*! + * Copyright 2019, 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 { NoopLogger } from '@opentelemetry/core'; +import { NodeTracer } from '@opentelemetry/node-sdk'; +import { + InMemorySpanExporter, + SimpleSpanProcessor, +} from '@opentelemetry/tracer-basic'; +import { SpanKind, Attributes, TimedEvent, Span } from '@opentelemetry/types'; +import { plugin, PostgresPlugin } from '../src'; +import { AttributeNames } from '../src/enums'; +import * as assert from 'assert'; +import * as pg from 'pg'; +import * as semver from 'semver'; +import * as assertionUtils from './assertionUtils'; +import * as testUtils from './testUtils'; + +const memoryExporter = new InMemorySpanExporter(); + +const CONFIG = { + user: 'postgres', + password: 'test', + database: 'postgres', + host: '127.0.0.1', + port: 5432, +}; + +const runCallbackTest = ( + span: Span, + attributes: Attributes, + events: TimedEvent[], + spansLength = 1, + spansIndex = 0 +) => { + const spans = memoryExporter.getFinishedSpans(); + assert.strictEqual(spans.length, spansLength); + const pgSpan = spans[spansIndex]; + assertionUtils.assertSpan(pgSpan, SpanKind.CLIENT, attributes, events); + assertionUtils.assertPropagation(pgSpan, span); +}; + +describe('pg@7.x', () => { + let client: pg.Client; + const tracer = new NodeTracer(); + const logger = new NoopLogger(); + const testPostgres = process.env.TEST_POSTGRES; // For CI: assumes local postgres db is already available + const testPostgresLocally = process.env.TEST_POSTGRES_LOCAL; // For local: spins up local postgres db via docker + const shouldTest = testPostgres || testPostgresLocally; // Skips these tests if false (default) + + before(function(ready) { + if (!shouldTest) { + // this.skip() workaround + // https://github.com/mochajs/mocha/issues/2683#issuecomment-375629901 + this.test!.parent!.pending = true; + this.skip(); + } + tracer.addSpanProcessor(new SimpleSpanProcessor(memoryExporter)); + testUtils.startDocker(); + client = new pg.Client(CONFIG); + + function connect() { + client.connect(err => { + if (err) { + setTimeout(connect, 500); + return; + } + ready(); + }); + } + connect(); + }); + after(done => { + if (testPostgresLocally) { + testUtils.cleanUpDocker(); + } + client.end(() => { + done(); + }); + }); + + beforeEach(function () { + plugin.enable(pg, tracer, logger); + }); + + afterEach(() => { + memoryExporter.reset(); + plugin.disable(); + }); + + it('should return a plugin', () => { + assert.ok(plugin instanceof PostgresPlugin); + }); + + it('should match version', () => { + assert.ok(semver.satisfies(plugin.version, '^7.12.1')); + }); + + it('should have correct moduleName', () => { + assert.strictEqual(plugin.moduleName, 'pg'); + }); + + it('should let the pg module throw its own errors with bad arguments', () => { + const assertPgError = (e: Error) => { + const src = e.stack!.split('\n').map(line => line.trim())[1]; + return /node_modules[/\\]pg/.test(src); + }; + + assert.throws( + () => { + (client as any).query(); + }, + assertPgError, + 'pg should throw when no args provided' + ); + assert.doesNotThrow( + () => (client as any).query({ foo: 'bar' }, undefined, () => null), + 'pg should not throw when invalid config args are provided' + ); + }); + + describe('#client.query(...)', () => { + it('should not return a promise if callback is provided', done => { + const res = client.query('SELECT NOW()', (err, res) => { + assert.strictEqual(err, null); + done(); + }); + assert.strictEqual(res, undefined, 'No promise is returned'); + }); + + it('it should intercept client.query(text, callback)', done => { + const attributes = { + [AttributeNames.COMPONENT]: PostgresPlugin.component, + [AttributeNames.PG_HOST]: CONFIG.host, + [AttributeNames.PG_PORT]: CONFIG.port, + [AttributeNames.PG_TEXT]: 'SELECT NOW()', + }; + const events: TimedEvent[] = []; + const span = tracer.startSpan('test span'); + tracer.withSpan(span, () => { + const res = client.query('SELECT NOW()', (err, res) => { + assert.strictEqual(err, null); + assert.ok(res); + runCallbackTest(span, attributes, events); + done(); + }); + assert.strictEqual(res, undefined, 'No promise is returned'); + }); + }); + + it('should intercept client.query(text, values, callback)', done => { + const query = 'SELECT $1::text'; + const values = ['0']; + const attributes = { + [AttributeNames.COMPONENT]: PostgresPlugin.component, + [AttributeNames.PG_HOST]: CONFIG.host, + [AttributeNames.PG_PORT]: CONFIG.port, + [AttributeNames.PG_TEXT]: query, + [AttributeNames.PG_VALUES]: values, + }; + const events: TimedEvent[] = []; + const span = tracer.startSpan('test span'); + tracer.withSpan(span, () => { + const resNoPromise = client.query(query, values, (err, res) => { + assert.strictEqual(err, null); + assert.ok(res); + runCallbackTest(span, attributes, events); + done(); + }); + assert.strictEqual(resNoPromise, undefined, 'No promise is returned'); + }); + }); + + it('should intercept client.query({text, callback})', done => { + const query = 'SELECT NOW()'; + const attributes = { + [AttributeNames.COMPONENT]: PostgresPlugin.component, + [AttributeNames.PG_HOST]: CONFIG.host, + [AttributeNames.PG_PORT]: CONFIG.port, + [AttributeNames.PG_TEXT]: query, + }; + const events: TimedEvent[] = []; + const span = tracer.startSpan('test span'); + tracer.withSpan(span, () => { + const resNoPromise = client.query({ + text: query, + callback: (err: Error, res: pg.QueryResult) => { + assert.strictEqual(err, null); + assert.ok(res); + runCallbackTest(span, attributes, events); + done(); + }, + } as pg.QueryConfig); + assert.strictEqual(resNoPromise, undefined, 'No promise is returned'); + }); + }); + + it('should intercept client.query({text}, callback)', done => { + const query = 'SELECT NOW()'; + const attributes = { + [AttributeNames.COMPONENT]: PostgresPlugin.component, + [AttributeNames.PG_HOST]: CONFIG.host, + [AttributeNames.PG_PORT]: CONFIG.port, + [AttributeNames.PG_TEXT]: query, + }; + const events: TimedEvent[] = []; + const span = tracer.startSpan('test span'); + tracer.withSpan(span, () => { + const resNoPromise = client.query({ text: query }, (err, res) => { + assert.strictEqual(err, null); + assert.ok(res); + runCallbackTest(span, attributes, events); + done(); + }); + assert.strictEqual(resNoPromise, undefined, 'No promise is returned'); + }); + }); + + it('should intercept client.query(text, values', async () => { + const query = 'SELECT $1::text'; + const values = ['0']; + const attributes = { + [AttributeNames.COMPONENT]: PostgresPlugin.component, + [AttributeNames.PG_HOST]: CONFIG.host, + [AttributeNames.PG_PORT]: CONFIG.port, + [AttributeNames.PG_TEXT]: query, + [AttributeNames.PG_VALUES]: values, + }; + const events: TimedEvent[] = []; + const span = tracer.startSpan('test span'); + await tracer.withSpan(span, async () => { + const resNoPromise = await client.query(query, values); + try { + assert.ok(resNoPromise); + runCallbackTest(span, attributes, events); + } catch (e) { + assert.ok(false, e.message); + } + }); + }); + + it('should intercept client.query({text, values})', async () => { + const query = 'SELECT $1::text'; + const values = ['0']; + const attributes = { + [AttributeNames.COMPONENT]: PostgresPlugin.component, + [AttributeNames.PG_HOST]: CONFIG.host, + [AttributeNames.PG_PORT]: CONFIG.port, + [AttributeNames.PG_TEXT]: query, + [AttributeNames.PG_VALUES]: values, + }; + const events: TimedEvent[] = []; + const span = tracer.startSpan('test span'); + await tracer.withSpan(span, async () => { + const resNoPromise = await client.query({ + text: query, + values: values, + }); + try { + assert.ok(resNoPromise); + runCallbackTest(span, attributes, events); + } catch (e) { + assert.ok(false, e.message); + } + }); + }); + + it('should intercept client.query(text)', async () => { + const query = 'SELECT NOW()'; + const attributes = { + [AttributeNames.COMPONENT]: PostgresPlugin.component, + [AttributeNames.PG_HOST]: CONFIG.host, + [AttributeNames.PG_PORT]: CONFIG.port, + [AttributeNames.PG_TEXT]: query, + }; + const events: TimedEvent[] = []; + const span = tracer.startSpan('test span'); + await tracer.withSpan(span, async () => { + const resNoPromise = await client.query(query); + try { + assert.ok(resNoPromise); + runCallbackTest(span, attributes, events); + } catch (e) { + assert.ok(false, e.message); + } + }); + }); + + it('should handle the same callback being given to multiple client.query()s', done => { + let events = 0; + + const queryHandler = (err: Error, res: pg.QueryResult) => { + if (err) { + throw err; + } + events += 1; + }; + + const config = { + text: 'SELECT NOW()', + callback: queryHandler, + }; + + client.query(config.text, config.callback); // 1 + client.query(config); // 2 + client.query(config.text, queryHandler); // 3 + client.query(config.text); // Not using queryHandler + client.query(config); // 4 + client.query(config.text, (err, res) => { + assert.strictEqual(events, 4); + done(); + }); + }); + + it('should preserve correct context even when using the same callback in client.query()', done => { + const spans = [tracer.startSpan('span 1'), tracer.startSpan('span 2')]; + const currentSpans: (Span | null)[] = []; + const queryHandler = () => { + currentSpans.push(tracer.getCurrentSpan()); + if (currentSpans.length === 2) { + assert.deepStrictEqual(currentSpans, spans); + done(); + } + }; + + tracer.withSpan(spans[0], () => { + client.query('SELECT NOW()', queryHandler); + }); + tracer.withSpan(spans[1], () => { + client.query('SELECT NOW()', queryHandler); + }); + }); + }); +}); diff --git a/packages/opentelemetry-plugin-postgres/test/testUtils.ts b/packages/opentelemetry-plugin-postgres/test/testUtils.ts new file mode 100644 index 0000000000..eec866fc41 --- /dev/null +++ b/packages/opentelemetry-plugin-postgres/test/testUtils.ts @@ -0,0 +1,54 @@ +/*! + * Copyright 2019, 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 childProcess from 'child_process'; +export function startDocker() { + const tasks = [ + run('docker run -d -p 54320:5432 --name otpostgres postgres:alpine'), + ]; + + for (let i = 0; i < tasks.length; i++) { + const task = tasks[i]; + if (task && task.code !== 0) { + console.error('Failed to start container!'); + console.error(task.output); + return false; + } + } + return true; +} + +export function cleanUpDocker() { + run('docker stop otpostgres'); + run('docker rm otpostgres'); +} + +function run(cmd: string) { + try { + const proc = childProcess.spawnSync(cmd, { + shell: true, + }); + return { + code: proc.status, + output: proc.output + .map(v => String.fromCharCode.apply(null, v as any)) + .join(''), + }; + } catch (e) { + console.log(e); + return; + } +} From 62ecb3940c8f933b52e2b0ba34ea8b184dbd2144 Mon Sep 17 00:00:00 2001 From: Mark Wolff Date: Wed, 9 Oct 2019 14:04:38 -0700 Subject: [PATCH 02/18] fix: linting --- packages/opentelemetry-plugin-postgres/test/pg.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/opentelemetry-plugin-postgres/test/pg.test.ts b/packages/opentelemetry-plugin-postgres/test/pg.test.ts index 072420dce8..8a27a9713c 100644 --- a/packages/opentelemetry-plugin-postgres/test/pg.test.ts +++ b/packages/opentelemetry-plugin-postgres/test/pg.test.ts @@ -92,7 +92,7 @@ describe('pg@7.x', () => { }); }); - beforeEach(function () { + beforeEach(function() { plugin.enable(pg, tracer, logger); }); From f8d885b6dc59f68f14f4321fcadfcf7bb5479ab4 Mon Sep 17 00:00:00 2001 From: Mark Wolff Date: Wed, 9 Oct 2019 14:14:54 -0700 Subject: [PATCH 03/18] fix: docker starting not locally --- .../test/pg.test.ts | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/packages/opentelemetry-plugin-postgres/test/pg.test.ts b/packages/opentelemetry-plugin-postgres/test/pg.test.ts index 8a27a9713c..c14c006afa 100644 --- a/packages/opentelemetry-plugin-postgres/test/pg.test.ts +++ b/packages/opentelemetry-plugin-postgres/test/pg.test.ts @@ -69,7 +69,9 @@ describe('pg@7.x', () => { this.skip(); } tracer.addSpanProcessor(new SimpleSpanProcessor(memoryExporter)); - testUtils.startDocker(); + if (testPostgresLocally) { + testUtils.startDocker(); + } client = new pg.Client(CONFIG); function connect() { @@ -242,9 +244,9 @@ describe('pg@7.x', () => { const events: TimedEvent[] = []; const span = tracer.startSpan('test span'); await tracer.withSpan(span, async () => { - const resNoPromise = await client.query(query, values); + const resPromise = await client.query(query, values); try { - assert.ok(resNoPromise); + assert.ok(resPromise); runCallbackTest(span, attributes, events); } catch (e) { assert.ok(false, e.message); @@ -265,12 +267,12 @@ describe('pg@7.x', () => { const events: TimedEvent[] = []; const span = tracer.startSpan('test span'); await tracer.withSpan(span, async () => { - const resNoPromise = await client.query({ + const resPromise = await client.query({ text: query, values: values, }); try { - assert.ok(resNoPromise); + assert.ok(resPromise); runCallbackTest(span, attributes, events); } catch (e) { assert.ok(false, e.message); @@ -289,9 +291,9 @@ describe('pg@7.x', () => { const events: TimedEvent[] = []; const span = tracer.startSpan('test span'); await tracer.withSpan(span, async () => { - const resNoPromise = await client.query(query); + const resPromise = await client.query(query); try { - assert.ok(resNoPromise); + assert.ok(resPromise); runCallbackTest(span, attributes, events); } catch (e) { assert.ok(false, e.message); From 9b90fde85a7293190200c156922fbff5541555c2 Mon Sep 17 00:00:00 2001 From: Mark Wolff Date: Wed, 9 Oct 2019 14:48:05 -0700 Subject: [PATCH 04/18] fix: compile errors from merge --- packages/opentelemetry-plugin-postgres/src/index.ts | 1 - .../opentelemetry-plugin-postgres/test/assertionUtils.ts | 6 +++--- packages/opentelemetry-plugin-postgres/test/pg.test.ts | 8 ++++---- 3 files changed, 7 insertions(+), 8 deletions(-) diff --git a/packages/opentelemetry-plugin-postgres/src/index.ts b/packages/opentelemetry-plugin-postgres/src/index.ts index e4bd892b38..33dab806f8 100644 --- a/packages/opentelemetry-plugin-postgres/src/index.ts +++ b/packages/opentelemetry-plugin-postgres/src/index.ts @@ -15,4 +15,3 @@ */ export * from './pg'; -// export * from './pg-pool'; diff --git a/packages/opentelemetry-plugin-postgres/test/assertionUtils.ts b/packages/opentelemetry-plugin-postgres/test/assertionUtils.ts index 4276e108b8..7fb08868f6 100644 --- a/packages/opentelemetry-plugin-postgres/test/assertionUtils.ts +++ b/packages/opentelemetry-plugin-postgres/test/assertionUtils.ts @@ -14,10 +14,10 @@ * limitations under the License. */ -import { SpanKind, Attributes, Event, Span } from '@opentelemetry/types'; +import { SpanKind, Attributes, Event, Span, TimedEvent } from '@opentelemetry/types'; import * as assert from 'assert'; import { PostgresPlugin } from '../src'; -import { ReadableSpan } from '@opentelemetry/tracer-basic'; +import { ReadableSpan } from '@opentelemetry/tracing'; import { hrTimeToMilliseconds, hrTimeToMicroseconds, @@ -62,7 +62,7 @@ export const assertSpan = ( events.length, 'Should contain same number of events' ); - span.events.forEach((_, index) => { + span.events.forEach((_: TimedEvent, index: number) => { assert.deepStrictEqual(span.events[index], events[index]); }); }; diff --git a/packages/opentelemetry-plugin-postgres/test/pg.test.ts b/packages/opentelemetry-plugin-postgres/test/pg.test.ts index c14c006afa..d84cc0ee42 100644 --- a/packages/opentelemetry-plugin-postgres/test/pg.test.ts +++ b/packages/opentelemetry-plugin-postgres/test/pg.test.ts @@ -15,11 +15,11 @@ */ import { NoopLogger } from '@opentelemetry/core'; -import { NodeTracer } from '@opentelemetry/node-sdk'; +import { NodeTracer } from '@opentelemetry/node'; import { InMemorySpanExporter, SimpleSpanProcessor, -} from '@opentelemetry/tracer-basic'; +} from '@opentelemetry/tracing'; import { SpanKind, Attributes, TimedEvent, Span } from '@opentelemetry/types'; import { plugin, PostgresPlugin } from '../src'; import { AttributeNames } from '../src/enums'; @@ -61,7 +61,7 @@ describe('pg@7.x', () => { const testPostgresLocally = process.env.TEST_POSTGRES_LOCAL; // For local: spins up local postgres db via docker const shouldTest = testPostgres || testPostgresLocally; // Skips these tests if false (default) - before(function(ready) { + before(function (ready) { if (!shouldTest) { // this.skip() workaround // https://github.com/mochajs/mocha/issues/2683#issuecomment-375629901 @@ -94,7 +94,7 @@ describe('pg@7.x', () => { }); }); - beforeEach(function() { + beforeEach(function () { plugin.enable(pg, tracer, logger); }); From c14b33f67881d08e3d780a958912cc7f3532c489 Mon Sep 17 00:00:00 2001 From: Mark Wolff Date: Wed, 9 Oct 2019 14:53:43 -0700 Subject: [PATCH 05/18] fix: linting --- .../opentelemetry-plugin-postgres/test/assertionUtils.ts | 8 +++++++- packages/opentelemetry-plugin-postgres/test/pg.test.ts | 4 ++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/packages/opentelemetry-plugin-postgres/test/assertionUtils.ts b/packages/opentelemetry-plugin-postgres/test/assertionUtils.ts index 7fb08868f6..0533d1093f 100644 --- a/packages/opentelemetry-plugin-postgres/test/assertionUtils.ts +++ b/packages/opentelemetry-plugin-postgres/test/assertionUtils.ts @@ -14,7 +14,13 @@ * limitations under the License. */ -import { SpanKind, Attributes, Event, Span, TimedEvent } from '@opentelemetry/types'; +import { + SpanKind, + Attributes, + Event, + Span, + TimedEvent, +} from '@opentelemetry/types'; import * as assert from 'assert'; import { PostgresPlugin } from '../src'; import { ReadableSpan } from '@opentelemetry/tracing'; diff --git a/packages/opentelemetry-plugin-postgres/test/pg.test.ts b/packages/opentelemetry-plugin-postgres/test/pg.test.ts index d84cc0ee42..c8a71ba02e 100644 --- a/packages/opentelemetry-plugin-postgres/test/pg.test.ts +++ b/packages/opentelemetry-plugin-postgres/test/pg.test.ts @@ -61,7 +61,7 @@ describe('pg@7.x', () => { const testPostgresLocally = process.env.TEST_POSTGRES_LOCAL; // For local: spins up local postgres db via docker const shouldTest = testPostgres || testPostgresLocally; // Skips these tests if false (default) - before(function (ready) { + before(function(ready) { if (!shouldTest) { // this.skip() workaround // https://github.com/mochajs/mocha/issues/2683#issuecomment-375629901 @@ -94,7 +94,7 @@ describe('pg@7.x', () => { }); }); - beforeEach(function () { + beforeEach(function() { plugin.enable(pg, tracer, logger); }); From dd1a7d3c77a7f0bf52806632e6746b5bc62c2018 Mon Sep 17 00:00:00 2001 From: Mark Wolff Date: Tue, 15 Oct 2019 16:38:19 -0700 Subject: [PATCH 06/18] refactor: use helper functions for span building --- .../package.json | 2 +- .../src/enums.ts | 18 +- .../opentelemetry-plugin-postgres/src/pg.ts | 218 ++++++++++-------- .../src/types.ts | 15 ++ .../test/assertionUtils.ts | 2 +- .../test/pg.test.ts | 80 ++++--- 6 files changed, 200 insertions(+), 135 deletions(-) diff --git a/packages/opentelemetry-plugin-postgres/package.json b/packages/opentelemetry-plugin-postgres/package.json index a9e9163ad6..49462f3181 100644 --- a/packages/opentelemetry-plugin-postgres/package.json +++ b/packages/opentelemetry-plugin-postgres/package.json @@ -8,7 +8,7 @@ "repository": "open-telemetry/opentelemetry-js", "scripts": { "test": "nyc ts-mocha -p tsconfig.json 'test/**/*.test.ts'", - "debug": "ts-mocha --inspect-brk --no-timeouts -p tsconfig.json 'test/**/*.test.ts'", + "test:debug": "ts-mocha --inspect-brk --no-timeouts -p tsconfig.json 'test/**/*.test.ts'", "tdd": "yarn test -- --watch-extensions ts --watch", "clean": "rimraf build/*", "check": "gts check", diff --git a/packages/opentelemetry-plugin-postgres/src/enums.ts b/packages/opentelemetry-plugin-postgres/src/enums.ts index 1d3944c172..92578970ef 100644 --- a/packages/opentelemetry-plugin-postgres/src/enums.ts +++ b/packages/opentelemetry-plugin-postgres/src/enums.ts @@ -15,10 +15,22 @@ */ export enum AttributeNames { + // required by https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/data-semantic-conventions.md#databases-client-calls COMPONENT = 'component', - PG_HOST = 'pg.host', - PG_PORT = 'pg.port', - PG_TEXT = 'pg.text', + DB_TYPE = 'db.type', + DB_INSTANCE = 'db.instance', + DB_STATEMENT = 'db.statement', + PEER_ADDRESS = 'peer.address', + PEER_HOST = 'peer.host', + + // optional + DB_USER = 'db.user', + PEER_PORT = 'peer.port', + PEER_IPV4 = 'peer.ipv4', + PEER_IPV6 = 'peer.ipv6', + PEER_SERVICE = 'peer.service', + + // PG specific -- not specified by spec PG_VALUES = 'pg.values', PG_PLAN = 'pg.plan', } diff --git a/packages/opentelemetry-plugin-postgres/src/pg.ts b/packages/opentelemetry-plugin-postgres/src/pg.ts index 6a0763f144..81bf478575 100644 --- a/packages/opentelemetry-plugin-postgres/src/pg.ts +++ b/packages/opentelemetry-plugin-postgres/src/pg.ts @@ -15,15 +15,27 @@ */ import { BasePlugin } from '@opentelemetry/core'; -import { SpanKind } from '@opentelemetry/types'; +import { SpanKind, Span, CanonicalCode } from '@opentelemetry/types'; import { AttributeNames } from './enums'; -import { PostgresCallback, PostgresPluginOptions } from './types'; +import { PostgresPluginOptions, PgClientConnectionParams, PgPluginQueryConfig } from './types'; import * as path from 'path'; import * as pgTypes from 'pg'; import * as shimmer from 'shimmer'; +// Helper function to get a low cardinality command name from the full text query +function getCommandFromText(text?: string): string { + if (text) { + const words = text.split(' '); + if (words && words.length > 0) { + return words[0]; + } + } + return 'unknown'; +} + export class PostgresPlugin extends BasePlugin { - static readonly component = 'pg'; + static readonly COMPONENT = 'pg'; + static readonly BASE_SPAN_NAME = PostgresPlugin.COMPONENT + '.query'; readonly supportedVersions = ['^7.12.1']; protected _config: PostgresPluginOptions; @@ -48,113 +60,131 @@ export class PostgresPlugin extends BasePlugin { } } + // Private helper function to start a span + private _pgStartSpan(client: pgTypes.Client & PgClientConnectionParams) { + return this._tracer.startSpan( + PostgresPlugin.BASE_SPAN_NAME, + { + kind: SpanKind.CLIENT, + parent: this._tracer.getCurrentSpan() || undefined, + attributes: { + [AttributeNames.COMPONENT]: PostgresPlugin.COMPONENT, + [AttributeNames.PEER_HOST]: client.connectionParameters.host, + [AttributeNames.PEER_PORT]: client.connectionParameters.port, + }, + } + ); + } + private _getClientQueryPatch() { const plugin = this; return (original: typeof pgTypes.Client.prototype.query) => { plugin._logger.debug( - `Patching ${PostgresPlugin.component}.Client.prototype.query` + `Patching ${PostgresPlugin.COMPONENT}.Client.prototype.query` ); - return function query(this: pgTypes.Client, ...args: unknown[]) { - // setup span - let callbackProvided: boolean = - args.length > 1 && typeof args[args.length - 1] === 'function'; - const span = plugin._tracer.startSpan( - `${PostgresPlugin.component}.query`, - { - kind: SpanKind.CLIENT, - parent: plugin._tracer.getCurrentSpan() || undefined, - attributes: { - [AttributeNames.COMPONENT]: PostgresPlugin.component, - [AttributeNames.PG_HOST]: (this as any).connectionParameters.host, - [AttributeNames.PG_PORT]: (this as any).connectionParameters.port, - }, - } - ); - - try { - if (typeof args[0] === 'string') { - span.setAttribute(AttributeNames.PG_TEXT, args[0]); - if (args[1] instanceof Array) { - span.setAttribute(AttributeNames.PG_VALUES, args[1]); - if (callbackProvided) { - args[2] = plugin._tracer.bind(args[2]); - } - } else { - if (callbackProvided) { - args[1] = plugin._tracer.bind(args[1]); - } - } + return function query(this: pgTypes.Client & PgClientConnectionParams, ...args: unknown[]) { + let callbackProvided = false; + const span = plugin._pgStartSpan(this); + + // Handle different client.query(...) signatures + if (typeof args[0] === 'string') { + if (args.length > 1 && args[1] instanceof Array) { + _handleParameterizedQuery.call(this, span, ...args); } else { - const config = args[0] as pgTypes.QueryConfig & { - callback?: PostgresCallback; - }; - if (typeof config.name === 'string') { - span.setAttribute(AttributeNames.PG_PLAN, config.name); - } else { - if (typeof config.text === 'string') { - span.setAttribute(AttributeNames.PG_TEXT, config.text); - } - if (config.values instanceof Array) { - span.setAttribute(AttributeNames.PG_VALUES, config.values); - } - } - - if (callbackProvided) { - if (typeof args[1] === 'function') { - args[1] = plugin._tracer.bind(args[1]); - } else if (typeof args[2] === 'function') { - args[2] = plugin._tracer.bind(args[2]); - } - } else if ( - config.callback && - typeof config.callback === 'function' - ) { - callbackProvided = true; - config.callback = plugin._tracer.bind(config.callback); - } + _handleTextQuery.call(this, span, ...args); + } + } else if (typeof args[0] === 'object') { + _handleConfigQuery.call(this, span, ...args); + } + + // Bind callback to parent span + // TODO: end the span + if (args.length > 0) { + if (typeof args[args.length - 1] === 'function') { + args[args.length - 1] = plugin._tracer.bind(args[args.length - 1]); + } else if (typeof (args[0] as PgPluginQueryConfig).callback === 'function') { + (args[0] as PgPluginQueryConfig).callback = plugin._tracer.bind((args[0] as PgPluginQueryConfig).callback); } - } catch (e) { - plugin._logger.warn( - `pg Plugin failed to trace query: error: ${e.message}` - ); - const result = original.apply(this, arguments as any); - span.end(); - return result; } - const queryResult = original.apply(this, args as any); + // Perform the original query + const result: unknown = original.apply(this, args as never); - // No callback was provided, return a promise instead (new as of pg@7.x) + // Bind promise to parent span and end the span + if (result instanceof Promise) { + return plugin._tracer.bind(result + .then((result: unknown) => { + // Return a pass-along promise which ends the span and then goes to user's orig resolvers + return new Promise((resolve, _) => { + span.setStatus({ code: CanonicalCode.OK }); + span.end(); + resolve(result); + }); + }) + .catch((error: Error) => { + return new Promise((_, reject) => { + span.setStatus({ code: CanonicalCode.UNKNOWN }) + span.end(); + reject(error); + }); + })); + } + // else returns void if (!callbackProvided) { - const queryResultPromise = (queryResult as unknown) as Promise< - unknown - >; - return plugin._tracer.bind( - queryResultPromise - .then((result: any) => { - // Return a pass-along promise which ends the span and then goes to user's orig resolvers - return new Promise((resolve, _) => { - span.end(); - resolve(result); - }); - }) - .catch((error: Error) => { - return new Promise((_, reject) => { - span.end(); - reject(error); - }); - }) - ); + span.setStatus({ + code: CanonicalCode.INVALID_ARGUMENT, + message: 'Invalid query provided to the driver' + }); + span.end(); } - - // Else a callback was provided, so just return the result - span.end(); - return queryResult; + return result; // void }; + }; } } + +// Queries where args[0] is a text query and 'values' was not specified +function _handleTextQuery(this: pgTypes.Client & PgClientConnectionParams, span: Span, ...args: unknown[]) { + // Set child span name + const queryCommand = getCommandFromText(args[0] as string); + span.updateName(PostgresPlugin.BASE_SPAN_NAME + ':' + queryCommand); + + // Set attributes + span.setAttribute(AttributeNames.DB_STATEMENT, args[0]); + +} + +// Queries where args[1] is a 'values' array +function _handleParameterizedQuery(this: pgTypes.Client & PgClientConnectionParams, span: Span, ...args: unknown[]) { + // Set child span name + const queryCommand = getCommandFromText(args[0] as string); + span.updateName(PostgresPlugin.BASE_SPAN_NAME + ':' + queryCommand); + + // Set attributes + span.setAttribute(AttributeNames.DB_STATEMENT, args[0]); + span.setAttribute(AttributeNames.PG_VALUES, args[1]); +} + +// Queries where args[0] is a QueryConfig +function _handleConfigQuery(this: pgTypes.Client & PgClientConnectionParams, span: Span, ...args: unknown[]) { + const config = args[0] as PgPluginQueryConfig; + + // Set attributes + span.setAttribute(AttributeNames.DB_STATEMENT, config.text); + if (config.values) { + span.setAttribute(AttributeNames.PG_VALUES, config.values); + } + if (config.name) { + span.setAttribute(AttributeNames.PG_PLAN, config.name); + } + + // Update span name with query command; prefer plan name, if available + const queryCommand = getCommandFromText(config.name || config.text); + span.updateName(PostgresPlugin.BASE_SPAN_NAME + ':' + queryCommand); +} + const basedir = path.dirname(require.resolve('pg')); const version = require(path.join(basedir, '../', 'package.json')).version; -export const plugin = new PostgresPlugin(PostgresPlugin.component, version); +export const plugin = new PostgresPlugin(PostgresPlugin.COMPONENT, version); diff --git a/packages/opentelemetry-plugin-postgres/src/types.ts b/packages/opentelemetry-plugin-postgres/src/types.ts index e74906d6ea..bdc348b2c5 100644 --- a/packages/opentelemetry-plugin-postgres/src/types.ts +++ b/packages/opentelemetry-plugin-postgres/src/types.ts @@ -14,6 +14,21 @@ * limitations under the License. */ +import * as pgTypes from 'pg'; + export interface PostgresPluginOptions {} export type PostgresCallback = (err: Error, res: object) => unknown; + +// These are not included in @types/pg, so manually define them. +// https://github.com/brianc/node-postgres/blob/fde5ec586e49258dfc4a2fcd861fcdecb4794fc3/lib/client.js#L25 +export interface PgClientConnectionParams { + connectionParameters: { + host: string, + port: number + } +} + +export interface PgPluginQueryConfig extends pgTypes.QueryConfig { + callback?: PostgresCallback; +} \ No newline at end of file diff --git a/packages/opentelemetry-plugin-postgres/test/assertionUtils.ts b/packages/opentelemetry-plugin-postgres/test/assertionUtils.ts index 0533d1093f..7daed9c11d 100644 --- a/packages/opentelemetry-plugin-postgres/test/assertionUtils.ts +++ b/packages/opentelemetry-plugin-postgres/test/assertionUtils.ts @@ -42,7 +42,7 @@ export const assertSpan = ( assert.strictEqual( span.attributes[AttributeNames.COMPONENT], - PostgresPlugin.component + PostgresPlugin.COMPONENT ); assert.ok(span.endTime); assert.strictEqual(span.links.length, 0); diff --git a/packages/opentelemetry-plugin-postgres/test/pg.test.ts b/packages/opentelemetry-plugin-postgres/test/pg.test.ts index c8a71ba02e..7bb92e0e0f 100644 --- a/packages/opentelemetry-plugin-postgres/test/pg.test.ts +++ b/packages/opentelemetry-plugin-postgres/test/pg.test.ts @@ -36,7 +36,7 @@ const CONFIG = { password: 'test', database: 'postgres', host: '127.0.0.1', - port: 5432, + port: 54320, }; const runCallbackTest = ( @@ -59,7 +59,7 @@ describe('pg@7.x', () => { const logger = new NoopLogger(); const testPostgres = process.env.TEST_POSTGRES; // For CI: assumes local postgres db is already available const testPostgresLocally = process.env.TEST_POSTGRES_LOCAL; // For local: spins up local postgres db via docker - const shouldTest = testPostgres || testPostgresLocally; // Skips these tests if false (default) + const shouldTest = true || testPostgres || testPostgresLocally; // Skips these tests if false (default) before(function(ready) { if (!shouldTest) { @@ -85,13 +85,11 @@ describe('pg@7.x', () => { } connect(); }); - after(done => { + after((done) => { if (testPostgresLocally) { testUtils.cleanUpDocker(); } - client.end(() => { - done(); - }); + client.end(done); }); beforeEach(function() { @@ -115,7 +113,7 @@ describe('pg@7.x', () => { assert.strictEqual(plugin.moduleName, 'pg'); }); - it('should let the pg module throw its own errors with bad arguments', () => { + it('should maintain pg module error throwing behavior with bad arguments', () => { const assertPgError = (e: Error) => { const src = e.stack!.split('\n').map(line => line.trim())[1]; return /node_modules[/\\]pg/.test(src); @@ -143,12 +141,22 @@ describe('pg@7.x', () => { assert.strictEqual(res, undefined, 'No promise is returned'); }); + it('should return a promise if callback is provided', done => { + const resPromise = client.query('SELECT NOW()'); + resPromise.then(res => { + assert.ok(res); + done(); + }).catch((err: Error) => { + assert.ok(false, err.message); + }); + }); + it('it should intercept client.query(text, callback)', done => { const attributes = { - [AttributeNames.COMPONENT]: PostgresPlugin.component, - [AttributeNames.PG_HOST]: CONFIG.host, - [AttributeNames.PG_PORT]: CONFIG.port, - [AttributeNames.PG_TEXT]: 'SELECT NOW()', + [AttributeNames.COMPONENT]: PostgresPlugin.COMPONENT, + [AttributeNames.PEER_HOST]: CONFIG.host, + [AttributeNames.PEER_PORT]: CONFIG.port, + [AttributeNames.DB_STATEMENT]: 'SELECT NOW()', }; const events: TimedEvent[] = []; const span = tracer.startSpan('test span'); @@ -167,10 +175,10 @@ describe('pg@7.x', () => { const query = 'SELECT $1::text'; const values = ['0']; const attributes = { - [AttributeNames.COMPONENT]: PostgresPlugin.component, - [AttributeNames.PG_HOST]: CONFIG.host, - [AttributeNames.PG_PORT]: CONFIG.port, - [AttributeNames.PG_TEXT]: query, + [AttributeNames.COMPONENT]: PostgresPlugin.COMPONENT, + [AttributeNames.PEER_HOST]: CONFIG.host, + [AttributeNames.PEER_PORT]: CONFIG.port, + [AttributeNames.DB_STATEMENT]: query, [AttributeNames.PG_VALUES]: values, }; const events: TimedEvent[] = []; @@ -189,10 +197,10 @@ describe('pg@7.x', () => { it('should intercept client.query({text, callback})', done => { const query = 'SELECT NOW()'; const attributes = { - [AttributeNames.COMPONENT]: PostgresPlugin.component, - [AttributeNames.PG_HOST]: CONFIG.host, - [AttributeNames.PG_PORT]: CONFIG.port, - [AttributeNames.PG_TEXT]: query, + [AttributeNames.COMPONENT]: PostgresPlugin.COMPONENT, + [AttributeNames.PEER_HOST]: CONFIG.host, + [AttributeNames.PEER_PORT]: CONFIG.port, + [AttributeNames.DB_STATEMENT]: query, }; const events: TimedEvent[] = []; const span = tracer.startSpan('test span'); @@ -213,10 +221,10 @@ describe('pg@7.x', () => { it('should intercept client.query({text}, callback)', done => { const query = 'SELECT NOW()'; const attributes = { - [AttributeNames.COMPONENT]: PostgresPlugin.component, - [AttributeNames.PG_HOST]: CONFIG.host, - [AttributeNames.PG_PORT]: CONFIG.port, - [AttributeNames.PG_TEXT]: query, + [AttributeNames.COMPONENT]: PostgresPlugin.COMPONENT, + [AttributeNames.PEER_HOST]: CONFIG.host, + [AttributeNames.PEER_PORT]: CONFIG.port, + [AttributeNames.DB_STATEMENT]: query, }; const events: TimedEvent[] = []; const span = tracer.startSpan('test span'); @@ -231,14 +239,14 @@ describe('pg@7.x', () => { }); }); - it('should intercept client.query(text, values', async () => { + it('should intercept client.query(text, values)', async () => { const query = 'SELECT $1::text'; const values = ['0']; const attributes = { - [AttributeNames.COMPONENT]: PostgresPlugin.component, - [AttributeNames.PG_HOST]: CONFIG.host, - [AttributeNames.PG_PORT]: CONFIG.port, - [AttributeNames.PG_TEXT]: query, + [AttributeNames.COMPONENT]: PostgresPlugin.COMPONENT, + [AttributeNames.PEER_HOST]: CONFIG.host, + [AttributeNames.PEER_PORT]: CONFIG.port, + [AttributeNames.DB_STATEMENT]: query, [AttributeNames.PG_VALUES]: values, }; const events: TimedEvent[] = []; @@ -258,10 +266,10 @@ describe('pg@7.x', () => { const query = 'SELECT $1::text'; const values = ['0']; const attributes = { - [AttributeNames.COMPONENT]: PostgresPlugin.component, - [AttributeNames.PG_HOST]: CONFIG.host, - [AttributeNames.PG_PORT]: CONFIG.port, - [AttributeNames.PG_TEXT]: query, + [AttributeNames.COMPONENT]: PostgresPlugin.COMPONENT, + [AttributeNames.PEER_HOST]: CONFIG.host, + [AttributeNames.PEER_PORT]: CONFIG.port, + [AttributeNames.DB_STATEMENT]: query, [AttributeNames.PG_VALUES]: values, }; const events: TimedEvent[] = []; @@ -283,10 +291,10 @@ describe('pg@7.x', () => { it('should intercept client.query(text)', async () => { const query = 'SELECT NOW()'; const attributes = { - [AttributeNames.COMPONENT]: PostgresPlugin.component, - [AttributeNames.PG_HOST]: CONFIG.host, - [AttributeNames.PG_PORT]: CONFIG.port, - [AttributeNames.PG_TEXT]: query, + [AttributeNames.COMPONENT]: PostgresPlugin.COMPONENT, + [AttributeNames.PEER_HOST]: CONFIG.host, + [AttributeNames.PEER_PORT]: CONFIG.port, + [AttributeNames.DB_STATEMENT]: query, }; const events: TimedEvent[] = []; const span = tracer.startSpan('test span'); From 7a854fe7257ceca45e99f89bf172a95663504529 Mon Sep 17 00:00:00 2001 From: Mark Wolff Date: Wed, 16 Oct 2019 11:25:04 -0700 Subject: [PATCH 07/18] fix: add callback patching to end span --- .../opentelemetry-plugin-postgres/src/pg.ts | 138 ++++++++++++------ .../src/types.ts | 12 +- .../test/pg.test.ts | 16 +- 3 files changed, 107 insertions(+), 59 deletions(-) diff --git a/packages/opentelemetry-plugin-postgres/src/pg.ts b/packages/opentelemetry-plugin-postgres/src/pg.ts index 81bf478575..01d4009d52 100644 --- a/packages/opentelemetry-plugin-postgres/src/pg.ts +++ b/packages/opentelemetry-plugin-postgres/src/pg.ts @@ -17,7 +17,12 @@ import { BasePlugin } from '@opentelemetry/core'; import { SpanKind, Span, CanonicalCode } from '@opentelemetry/types'; import { AttributeNames } from './enums'; -import { PostgresPluginOptions, PgClientConnectionParams, PgPluginQueryConfig } from './types'; +import { + PostgresPluginOptions, + PgClientConnectionParams, + PgPluginQueryConfig, + PostgresCallback, +} from './types'; import * as path from 'path'; import * as pgTypes from 'pg'; import * as shimmer from 'shimmer'; @@ -34,10 +39,12 @@ function getCommandFromText(text?: string): string { } export class PostgresPlugin extends BasePlugin { + protected _config: PostgresPluginOptions; + static readonly COMPONENT = 'pg'; static readonly BASE_SPAN_NAME = PostgresPlugin.COMPONENT + '.query'; + readonly supportedVersions = ['^7.12.1']; - protected _config: PostgresPluginOptions; constructor(readonly moduleName: string, readonly version: string) { super(); @@ -54,35 +61,23 @@ export class PostgresPlugin extends BasePlugin { } return this._moduleExports; } + protected unpatch(): void { if (this._moduleExports.Client.prototype.query) { shimmer.unwrap(this._moduleExports.Client.prototype, 'query'); } } - // Private helper function to start a span - private _pgStartSpan(client: pgTypes.Client & PgClientConnectionParams) { - return this._tracer.startSpan( - PostgresPlugin.BASE_SPAN_NAME, - { - kind: SpanKind.CLIENT, - parent: this._tracer.getCurrentSpan() || undefined, - attributes: { - [AttributeNames.COMPONENT]: PostgresPlugin.COMPONENT, - [AttributeNames.PEER_HOST]: client.connectionParameters.host, - [AttributeNames.PEER_PORT]: client.connectionParameters.port, - }, - } - ); - } - private _getClientQueryPatch() { const plugin = this; return (original: typeof pgTypes.Client.prototype.query) => { plugin._logger.debug( `Patching ${PostgresPlugin.COMPONENT}.Client.prototype.query` ); - return function query(this: pgTypes.Client & PgClientConnectionParams, ...args: unknown[]) { + return function query( + this: pgTypes.Client & PgClientConnectionParams, + ...args: unknown[] + ) { let callbackProvided = false; const span = plugin._pgStartSpan(this); @@ -98,12 +93,19 @@ export class PostgresPlugin extends BasePlugin { } // Bind callback to parent span - // TODO: end the span if (args.length > 0) { if (typeof args[args.length - 1] === 'function') { - args[args.length - 1] = plugin._tracer.bind(args[args.length - 1]); - } else if (typeof (args[0] as PgPluginQueryConfig).callback === 'function') { - (args[0] as PgPluginQueryConfig).callback = plugin._tracer.bind((args[0] as PgPluginQueryConfig).callback); + args[args.length - 1] = plugin._tracer.bind( + _patchCallback(span, args[args.length - 1] as PostgresCallback) + ); + callbackProvided = true; + } else if ( + typeof (args[0] as PgPluginQueryConfig).callback === 'function' + ) { + (args[0] as PgPluginQueryConfig).callback = plugin._tracer.bind( + _patchCallback(span, (args[0] as PgPluginQueryConfig).callback!) + ); + callbackProvided = true; } } @@ -112,52 +114,72 @@ export class PostgresPlugin extends BasePlugin { // Bind promise to parent span and end the span if (result instanceof Promise) { - return plugin._tracer.bind(result - .then((result: unknown) => { - // Return a pass-along promise which ends the span and then goes to user's orig resolvers - return new Promise((resolve, _) => { - span.setStatus({ code: CanonicalCode.OK }); - span.end(); - resolve(result); - }); - }) - .catch((error: Error) => { - return new Promise((_, reject) => { - span.setStatus({ code: CanonicalCode.UNKNOWN }) - span.end(); - reject(error); - }); - })); + return plugin._tracer.bind( + result + .then((result: unknown) => { + // Return a pass-along promise which ends the span and then goes to user's orig resolvers + return new Promise((resolve, _) => { + span.setStatus({ code: CanonicalCode.OK }); + span.end(); + resolve(result); + }); + }) + .catch((error: Error) => { + return new Promise((_, reject) => { + span.setStatus({ code: CanonicalCode.UNKNOWN }); + span.end(); + reject(error); + }); + }) + ); } // else returns void if (!callbackProvided) { span.setStatus({ code: CanonicalCode.INVALID_ARGUMENT, - message: 'Invalid query provided to the driver' + message: 'Invalid query provided to the driver', }); span.end(); } return result; // void }; - }; } -} + // Private helper function to start a span + private _pgStartSpan(client: pgTypes.Client & PgClientConnectionParams) { + return this._tracer.startSpan(PostgresPlugin.BASE_SPAN_NAME, { + kind: SpanKind.CLIENT, + parent: this._tracer.getCurrentSpan() || undefined, + attributes: { + [AttributeNames.COMPONENT]: PostgresPlugin.COMPONENT, + [AttributeNames.PEER_HOST]: client.connectionParameters.host, + [AttributeNames.PEER_PORT]: client.connectionParameters.port, + }, + }); + } +} // Queries where args[0] is a text query and 'values' was not specified -function _handleTextQuery(this: pgTypes.Client & PgClientConnectionParams, span: Span, ...args: unknown[]) { +function _handleTextQuery( + this: pgTypes.Client & PgClientConnectionParams, + span: Span, + ...args: unknown[] +) { // Set child span name const queryCommand = getCommandFromText(args[0] as string); span.updateName(PostgresPlugin.BASE_SPAN_NAME + ':' + queryCommand); // Set attributes span.setAttribute(AttributeNames.DB_STATEMENT, args[0]); - } // Queries where args[1] is a 'values' array -function _handleParameterizedQuery(this: pgTypes.Client & PgClientConnectionParams, span: Span, ...args: unknown[]) { +function _handleParameterizedQuery( + this: pgTypes.Client & PgClientConnectionParams, + span: Span, + ...args: unknown[] +) { // Set child span name const queryCommand = getCommandFromText(args[0] as string); span.updateName(PostgresPlugin.BASE_SPAN_NAME + ':' + queryCommand); @@ -168,7 +190,11 @@ function _handleParameterizedQuery(this: pgTypes.Client & PgClientConnectionPara } // Queries where args[0] is a QueryConfig -function _handleConfigQuery(this: pgTypes.Client & PgClientConnectionParams, span: Span, ...args: unknown[]) { +function _handleConfigQuery( + this: pgTypes.Client & PgClientConnectionParams, + span: Span, + ...args: unknown[] +) { const config = args[0] as PgPluginQueryConfig; // Set attributes @@ -185,6 +211,26 @@ function _handleConfigQuery(this: pgTypes.Client & PgClientConnectionParams, spa span.updateName(PostgresPlugin.BASE_SPAN_NAME + ':' + queryCommand); } +function _patchCallback(span: Span, cb: PostgresCallback): PostgresCallback { + const originalCb = cb; + return function patchedCallback( + this: pgTypes.Client & PgClientConnectionParams, + err: Error, + res: object + ) { + if (err) { + span.setStatus({ + code: CanonicalCode.UNKNOWN, + message: err.message, + }); + } else if (res) { + span.setStatus({ code: CanonicalCode.OK }); + } + span.end(); + return originalCb.call(this, err, res); + }; +} + const basedir = path.dirname(require.resolve('pg')); const version = require(path.join(basedir, '../', 'package.json')).version; export const plugin = new PostgresPlugin(PostgresPlugin.COMPONENT, version); diff --git a/packages/opentelemetry-plugin-postgres/src/types.ts b/packages/opentelemetry-plugin-postgres/src/types.ts index bdc348b2c5..23ca772fa2 100644 --- a/packages/opentelemetry-plugin-postgres/src/types.ts +++ b/packages/opentelemetry-plugin-postgres/src/types.ts @@ -23,12 +23,12 @@ export type PostgresCallback = (err: Error, res: object) => unknown; // These are not included in @types/pg, so manually define them. // https://github.com/brianc/node-postgres/blob/fde5ec586e49258dfc4a2fcd861fcdecb4794fc3/lib/client.js#L25 export interface PgClientConnectionParams { - connectionParameters: { - host: string, - port: number - } + connectionParameters: { + host: string; + port: number; + }; } export interface PgPluginQueryConfig extends pgTypes.QueryConfig { - callback?: PostgresCallback; -} \ No newline at end of file + callback?: PostgresCallback; +} diff --git a/packages/opentelemetry-plugin-postgres/test/pg.test.ts b/packages/opentelemetry-plugin-postgres/test/pg.test.ts index 7bb92e0e0f..e3eb471946 100644 --- a/packages/opentelemetry-plugin-postgres/test/pg.test.ts +++ b/packages/opentelemetry-plugin-postgres/test/pg.test.ts @@ -85,7 +85,7 @@ describe('pg@7.x', () => { } connect(); }); - after((done) => { + after(done => { if (testPostgresLocally) { testUtils.cleanUpDocker(); } @@ -143,12 +143,14 @@ describe('pg@7.x', () => { it('should return a promise if callback is provided', done => { const resPromise = client.query('SELECT NOW()'); - resPromise.then(res => { - assert.ok(res); - done(); - }).catch((err: Error) => { - assert.ok(false, err.message); - }); + resPromise + .then(res => { + assert.ok(res); + done(); + }) + .catch((err: Error) => { + assert.ok(false, err.message); + }); }); it('it should intercept client.query(text, callback)', done => { From eafdb3d1d77a52ee83ef8c274080e9a6eadcae92 Mon Sep 17 00:00:00 2001 From: Mark Wolff Date: Fri, 18 Oct 2019 11:37:52 -0700 Subject: [PATCH 08/18] fix: add required attributes, address comments --- .../package.json | 2 +- .../src/enums.ts | 2 +- .../opentelemetry-plugin-postgres/src/pg.ts | 182 ++++------ .../src/types.ts | 12 +- .../src/utils.ts | 128 +++++++ .../test/pg.test.ts | 324 ++++++++++-------- 6 files changed, 384 insertions(+), 266 deletions(-) create mode 100644 packages/opentelemetry-plugin-postgres/src/utils.ts diff --git a/packages/opentelemetry-plugin-postgres/package.json b/packages/opentelemetry-plugin-postgres/package.json index 49462f3181..cb2006c5cc 100644 --- a/packages/opentelemetry-plugin-postgres/package.json +++ b/packages/opentelemetry-plugin-postgres/package.json @@ -46,7 +46,7 @@ "@types/pg": "^7.11.2", "@types/shimmer": "^1.0.1", "codecov": "^3.5.0", - "gts": "^1.1.0", + "gts": "^1.0.0", "mocha": "^6.2.0", "nyc": "^14.1.1", "rimraf": "^3.0.0", diff --git a/packages/opentelemetry-plugin-postgres/src/enums.ts b/packages/opentelemetry-plugin-postgres/src/enums.ts index 92578970ef..2df81cef3f 100644 --- a/packages/opentelemetry-plugin-postgres/src/enums.ts +++ b/packages/opentelemetry-plugin-postgres/src/enums.ts @@ -21,7 +21,7 @@ export enum AttributeNames { DB_INSTANCE = 'db.instance', DB_STATEMENT = 'db.statement', PEER_ADDRESS = 'peer.address', - PEER_HOST = 'peer.host', + PEER_HOSTNAME = 'peer.host', // optional DB_USER = 'db.user', diff --git a/packages/opentelemetry-plugin-postgres/src/pg.ts b/packages/opentelemetry-plugin-postgres/src/pg.ts index 01d4009d52..7c98905c56 100644 --- a/packages/opentelemetry-plugin-postgres/src/pg.ts +++ b/packages/opentelemetry-plugin-postgres/src/pg.ts @@ -15,38 +15,29 @@ */ import { BasePlugin } from '@opentelemetry/core'; -import { SpanKind, Span, CanonicalCode } from '@opentelemetry/types'; +import { SpanKind, CanonicalCode } from '@opentelemetry/types'; import { AttributeNames } from './enums'; import { PostgresPluginOptions, - PgClientConnectionParams, + PgClientExtended, PgPluginQueryConfig, PostgresCallback, } from './types'; -import * as path from 'path'; import * as pgTypes from 'pg'; import * as shimmer from 'shimmer'; - -// Helper function to get a low cardinality command name from the full text query -function getCommandFromText(text?: string): string { - if (text) { - const words = text.split(' '); - if (words && words.length > 0) { - return words[0]; - } - } - return 'unknown'; -} +import * as utils from './utils'; export class PostgresPlugin extends BasePlugin { protected _config: PostgresPluginOptions; static readonly COMPONENT = 'pg'; + static readonly DB_TYPE = 'sql'; + static readonly BASE_SPAN_NAME = PostgresPlugin.COMPONENT + '.query'; readonly supportedVersions = ['^7.12.1']; - constructor(readonly moduleName: string, readonly version: string) { + constructor(readonly moduleName: string) { super(); this._config = {}; } @@ -75,7 +66,7 @@ export class PostgresPlugin extends BasePlugin { `Patching ${PostgresPlugin.COMPONENT}.Client.prototype.query` ); return function query( - this: pgTypes.Client & PgClientConnectionParams, + this: pgTypes.Client & PgClientExtended, ...args: unknown[] ) { let callbackProvided = false; @@ -84,27 +75,45 @@ export class PostgresPlugin extends BasePlugin { // Handle different client.query(...) signatures if (typeof args[0] === 'string') { if (args.length > 1 && args[1] instanceof Array) { - _handleParameterizedQuery.call(this, span, ...args); + utils._handleParameterizedQuery.call(this, span, ...args); } else { - _handleTextQuery.call(this, span, ...args); + utils._handleTextQuery.call(this, span, ...args); } } else if (typeof args[0] === 'object') { - _handleConfigQuery.call(this, span, ...args); + utils._handleConfigQuery.call(this, span, ...args); } // Bind callback to parent span if (args.length > 0) { + const parentSpan = plugin._tracer.getCurrentSpan(); if (typeof args[args.length - 1] === 'function') { - args[args.length - 1] = plugin._tracer.bind( - _patchCallback(span, args[args.length - 1] as PostgresCallback) - ); + // Patch ParameterQuery callback + args[args.length - 1] = utils._patchCallback(span, args[ + args.length - 1 + ] as PostgresCallback); + // If a parent span exists, bind the callback + if (parentSpan) { + args[args.length - 1] = plugin._tracer.bind( + args[args.length - 1] + ); + } callbackProvided = true; } else if ( typeof (args[0] as PgPluginQueryConfig).callback === 'function' ) { - (args[0] as PgPluginQueryConfig).callback = plugin._tracer.bind( - _patchCallback(span, (args[0] as PgPluginQueryConfig).callback!) + // Patch ConfigQuery callback + let callback = utils._patchCallback( + span, + (args[0] as PgPluginQueryConfig).callback! ); + // If a parent span existed, bind the callback + if (parentSpan) { + callback = plugin._tracer.bind(callback); + } + + // Copy the callback instead of writing to args.callback so that we don't modify user's + // original callback reference + args[0] = { ...args[0], callback }; callbackProvided = true; } } @@ -114,26 +123,25 @@ export class PostgresPlugin extends BasePlugin { // Bind promise to parent span and end the span if (result instanceof Promise) { - return plugin._tracer.bind( - result - .then((result: unknown) => { - // Return a pass-along promise which ends the span and then goes to user's orig resolvers - return new Promise((resolve, _) => { - span.setStatus({ code: CanonicalCode.OK }); - span.end(); - resolve(result); - }); - }) - .catch((error: Error) => { - return new Promise((_, reject) => { - span.setStatus({ code: CanonicalCode.UNKNOWN }); - span.end(); - reject(error); - }); - }) - ); + return result + .then((result: unknown) => { + // Return a pass-along promise which ends the span and then goes to user's orig resolvers + return new Promise((resolve, _) => { + span.setStatus({ code: CanonicalCode.OK }); + span.end(); + resolve(result); + }); + }) + .catch((error: Error) => { + return new Promise((_, reject) => { + span.setStatus({ code: CanonicalCode.UNKNOWN }); + span.end(); + reject(error); + }); + }); } - // else returns void + + // If a promise was not returned and no callback is provided, we recieved invalid args if (!callbackProvided) { span.setStatus({ code: CanonicalCode.INVALID_ARGUMENT, @@ -141,96 +149,30 @@ export class PostgresPlugin extends BasePlugin { }); span.end(); } + + // else returns void return result; // void }; }; } // Private helper function to start a span - private _pgStartSpan(client: pgTypes.Client & PgClientConnectionParams) { + private _pgStartSpan(client: pgTypes.Client & PgClientExtended) { + const jdbcString = utils._getJDBCString(client.connectionParameters); return this._tracer.startSpan(PostgresPlugin.BASE_SPAN_NAME, { kind: SpanKind.CLIENT, parent: this._tracer.getCurrentSpan() || undefined, attributes: { - [AttributeNames.COMPONENT]: PostgresPlugin.COMPONENT, - [AttributeNames.PEER_HOST]: client.connectionParameters.host, + [AttributeNames.COMPONENT]: PostgresPlugin.COMPONENT, // required + [AttributeNames.DB_INSTANCE]: client.connectionParameters.database, // required + [AttributeNames.DB_TYPE]: PostgresPlugin.DB_TYPE, // required + [AttributeNames.PEER_ADDRESS]: jdbcString, // required + [AttributeNames.PEER_HOSTNAME]: client.connectionParameters.host, // required [AttributeNames.PEER_PORT]: client.connectionParameters.port, + [AttributeNames.DB_USER]: client.connectionParameters.user, }, }); } } -// Queries where args[0] is a text query and 'values' was not specified -function _handleTextQuery( - this: pgTypes.Client & PgClientConnectionParams, - span: Span, - ...args: unknown[] -) { - // Set child span name - const queryCommand = getCommandFromText(args[0] as string); - span.updateName(PostgresPlugin.BASE_SPAN_NAME + ':' + queryCommand); - - // Set attributes - span.setAttribute(AttributeNames.DB_STATEMENT, args[0]); -} - -// Queries where args[1] is a 'values' array -function _handleParameterizedQuery( - this: pgTypes.Client & PgClientConnectionParams, - span: Span, - ...args: unknown[] -) { - // Set child span name - const queryCommand = getCommandFromText(args[0] as string); - span.updateName(PostgresPlugin.BASE_SPAN_NAME + ':' + queryCommand); - - // Set attributes - span.setAttribute(AttributeNames.DB_STATEMENT, args[0]); - span.setAttribute(AttributeNames.PG_VALUES, args[1]); -} - -// Queries where args[0] is a QueryConfig -function _handleConfigQuery( - this: pgTypes.Client & PgClientConnectionParams, - span: Span, - ...args: unknown[] -) { - const config = args[0] as PgPluginQueryConfig; - - // Set attributes - span.setAttribute(AttributeNames.DB_STATEMENT, config.text); - if (config.values) { - span.setAttribute(AttributeNames.PG_VALUES, config.values); - } - if (config.name) { - span.setAttribute(AttributeNames.PG_PLAN, config.name); - } - - // Update span name with query command; prefer plan name, if available - const queryCommand = getCommandFromText(config.name || config.text); - span.updateName(PostgresPlugin.BASE_SPAN_NAME + ':' + queryCommand); -} - -function _patchCallback(span: Span, cb: PostgresCallback): PostgresCallback { - const originalCb = cb; - return function patchedCallback( - this: pgTypes.Client & PgClientConnectionParams, - err: Error, - res: object - ) { - if (err) { - span.setStatus({ - code: CanonicalCode.UNKNOWN, - message: err.message, - }); - } else if (res) { - span.setStatus({ code: CanonicalCode.OK }); - } - span.end(); - return originalCb.call(this, err, res); - }; -} - -const basedir = path.dirname(require.resolve('pg')); -const version = require(path.join(basedir, '../', 'package.json')).version; -export const plugin = new PostgresPlugin(PostgresPlugin.COMPONENT, version); +export const plugin = new PostgresPlugin(PostgresPlugin.COMPONENT); diff --git a/packages/opentelemetry-plugin-postgres/src/types.ts b/packages/opentelemetry-plugin-postgres/src/types.ts index 23ca772fa2..c9d1f70974 100644 --- a/packages/opentelemetry-plugin-postgres/src/types.ts +++ b/packages/opentelemetry-plugin-postgres/src/types.ts @@ -23,10 +23,14 @@ export type PostgresCallback = (err: Error, res: object) => unknown; // These are not included in @types/pg, so manually define them. // https://github.com/brianc/node-postgres/blob/fde5ec586e49258dfc4a2fcd861fcdecb4794fc3/lib/client.js#L25 export interface PgClientConnectionParams { - connectionParameters: { - host: string; - port: number; - }; + database: string; + host: string; + port: number; + user: string; +} + +export interface PgClientExtended { + connectionParameters: PgClientConnectionParams; } export interface PgPluginQueryConfig extends pgTypes.QueryConfig { diff --git a/packages/opentelemetry-plugin-postgres/src/utils.ts b/packages/opentelemetry-plugin-postgres/src/utils.ts new file mode 100644 index 0000000000..d7d1edd112 --- /dev/null +++ b/packages/opentelemetry-plugin-postgres/src/utils.ts @@ -0,0 +1,128 @@ +/*! + * Copyright 2019, 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 { Span, CanonicalCode } from '@opentelemetry/types'; +import { AttributeNames } from './enums'; +import { + PgClientExtended, + PgPluginQueryConfig, + PostgresCallback, + PgClientConnectionParams, +} from './types'; +import * as pgTypes from 'pg'; +import { PostgresPlugin } from './pg'; + +export function _arrayStringifyHelper(arr: Array): string { + return '[' + arr.toString() + ']'; +} + +// Helper function to get a low cardinality command name from the full text query +export function _getCommandFromText(text?: string): string { + if (text) { + const words = text.split(' '); + if (words && words.length > 0) { + return words[0]; + } + } + return 'unknown'; +} + +export function _getJDBCString(params: PgClientConnectionParams) { + const host = params.host || 'localhost'; // postgres defaults to localhost + const port = params.port || 5432; // postgres defaults to port 5432 + const database = params.database || ''; + return `jdbc:postgresql://${host}:${port}/${database}`; +} + +// Queries where args[0] is a QueryConfig +export function _handleConfigQuery( + this: pgTypes.Client & PgClientExtended, + span: Span, + ...args: unknown[] +) { + const argsConfig = args[0] as PgPluginQueryConfig; + + // Set attributes + span.setAttribute(AttributeNames.DB_STATEMENT, argsConfig.text); + if (argsConfig.values instanceof Array) { + span.setAttribute( + AttributeNames.PG_VALUES, + _arrayStringifyHelper(argsConfig.values) + ); + } + if (argsConfig.name) { + span.setAttribute(AttributeNames.PG_PLAN, argsConfig.name); + } + + // Update span name with query command; prefer plan name, if available + const queryCommand = _getCommandFromText(argsConfig.name || argsConfig.text); + span.updateName(PostgresPlugin.BASE_SPAN_NAME + ':' + queryCommand); +} + +// Queries where args[1] is a 'values' array +export function _handleParameterizedQuery( + this: pgTypes.Client & PgClientExtended, + span: Span, + ...args: unknown[] +) { + + // Set child span name + const queryCommand = _getCommandFromText(args[0] as string); + span.updateName(PostgresPlugin.BASE_SPAN_NAME + ':' + queryCommand); + + // Set attributes + span.setAttribute(AttributeNames.DB_STATEMENT, args[0]); + if (args[1] instanceof Array) { + span.setAttribute(AttributeNames.PG_VALUES, _arrayStringifyHelper(args[1])); + } +} + +// Queries where args[0] is a text query and 'values' was not specified +export function _handleTextQuery( + this: pgTypes.Client & PgClientExtended, + span: Span, + ...args: unknown[] +) { + // Set child span name + const queryCommand = _getCommandFromText(args[0] as string); + span.updateName(PostgresPlugin.BASE_SPAN_NAME + ':' + queryCommand); + + // Set attributes + span.setAttribute(AttributeNames.DB_STATEMENT, args[0]); +} + +export function _patchCallback( + span: Span, + cb: PostgresCallback +): PostgresCallback { + const originalCb = cb; + return function patchedCallback( + this: pgTypes.Client & PgClientExtended, + err: Error, + res: object + ) { + if (err) { + span.setStatus({ + code: CanonicalCode.UNKNOWN, + message: err.message, + }); + } else if (res) { + span.setStatus({ code: CanonicalCode.OK }); + } + span.end(); + return originalCb.call(this, err, res); + }; +} diff --git a/packages/opentelemetry-plugin-postgres/test/pg.test.ts b/packages/opentelemetry-plugin-postgres/test/pg.test.ts index e3eb471946..b8f6ff4c95 100644 --- a/packages/opentelemetry-plugin-postgres/test/pg.test.ts +++ b/packages/opentelemetry-plugin-postgres/test/pg.test.ts @@ -25,9 +25,9 @@ import { plugin, PostgresPlugin } from '../src'; import { AttributeNames } from '../src/enums'; import * as assert from 'assert'; import * as pg from 'pg'; -import * as semver from 'semver'; import * as assertionUtils from './assertionUtils'; import * as testUtils from './testUtils'; +import { _arrayStringifyHelper } from '../src/utils'; const memoryExporter = new InMemorySpanExporter(); @@ -39,6 +39,16 @@ const CONFIG = { port: 54320, }; +const DEFAULT_ATTRIBUTES = { + [AttributeNames.COMPONENT]: PostgresPlugin.COMPONENT, + [AttributeNames.DB_INSTANCE]: CONFIG.database, + [AttributeNames.DB_TYPE]: PostgresPlugin.DB_TYPE, + [AttributeNames.PEER_HOSTNAME]: CONFIG.host, + [AttributeNames.PEER_ADDRESS]: 'jdbc:postgresql://127.0.0.1:54320/postgres', + [AttributeNames.PEER_PORT]: CONFIG.port, + [AttributeNames.DB_USER]: CONFIG.user, +}; + const runCallbackTest = ( span: Span, attributes: Attributes, @@ -105,10 +115,6 @@ describe('pg@7.x', () => { assert.ok(plugin instanceof PostgresPlugin); }); - it('should match version', () => { - assert.ok(semver.satisfies(plugin.version, '^7.12.1')); - }); - it('should have correct moduleName', () => { assert.strictEqual(plugin.moduleName, 'pg'); }); @@ -133,75 +139,69 @@ describe('pg@7.x', () => { }); describe('#client.query(...)', () => { - it('should not return a promise if callback is provided', done => { - const res = client.query('SELECT NOW()', (err, res) => { - assert.strictEqual(err, null); - done(); - }); - assert.strictEqual(res, undefined, 'No promise is returned'); - }); + // it('should not return a promise if callback is provided', done => { + // const res = client.query('SELECT NOW()', (err, res) => { + // assert.strictEqual(err, null); + // done(); + // }); + // assert.strictEqual(res, undefined, 'No promise is returned'); + // }); - it('should return a promise if callback is provided', done => { - const resPromise = client.query('SELECT NOW()'); - resPromise - .then(res => { - assert.ok(res); - done(); - }) - .catch((err: Error) => { - assert.ok(false, err.message); - }); - }); + // it('should return a promise if callback is provided', done => { + // const resPromise = client.query('SELECT NOW()'); + // resPromise + // .then(res => { + // assert.ok(res); + // done(); + // }) + // .catch((err: Error) => { + // assert.ok(false, err.message); + // }); + // }); - it('it should intercept client.query(text, callback)', done => { - const attributes = { - [AttributeNames.COMPONENT]: PostgresPlugin.COMPONENT, - [AttributeNames.PEER_HOST]: CONFIG.host, - [AttributeNames.PEER_PORT]: CONFIG.port, - [AttributeNames.DB_STATEMENT]: 'SELECT NOW()', - }; - const events: TimedEvent[] = []; - const span = tracer.startSpan('test span'); - tracer.withSpan(span, () => { - const res = client.query('SELECT NOW()', (err, res) => { - assert.strictEqual(err, null); - assert.ok(res); - runCallbackTest(span, attributes, events); - done(); - }); - assert.strictEqual(res, undefined, 'No promise is returned'); - }); - }); + // it('should intercept client.query(text, callback)', done => { + // const attributes = { + // ...DEFAULT_ATTRIBUTES, + // [AttributeNames.DB_STATEMENT]: 'SELECT NOW()', + // }; + // const events: TimedEvent[] = []; + // const span = tracer.startSpan('test span'); + // tracer.withSpan(span, () => { + // const res = client.query('SELECT NOW()', (err, res) => { + // assert.strictEqual(err, null); + // assert.ok(res); + // runCallbackTest(span, attributes, events); + // done(); + // }); + // assert.strictEqual(res, undefined, 'No promise is returned'); + // }); + // }); - it('should intercept client.query(text, values, callback)', done => { - const query = 'SELECT $1::text'; - const values = ['0']; - const attributes = { - [AttributeNames.COMPONENT]: PostgresPlugin.COMPONENT, - [AttributeNames.PEER_HOST]: CONFIG.host, - [AttributeNames.PEER_PORT]: CONFIG.port, - [AttributeNames.DB_STATEMENT]: query, - [AttributeNames.PG_VALUES]: values, - }; - const events: TimedEvent[] = []; - const span = tracer.startSpan('test span'); - tracer.withSpan(span, () => { - const resNoPromise = client.query(query, values, (err, res) => { - assert.strictEqual(err, null); - assert.ok(res); - runCallbackTest(span, attributes, events); - done(); - }); - assert.strictEqual(resNoPromise, undefined, 'No promise is returned'); - }); - }); + // it('should intercept client.query(text, values, callback)', done => { + // const query = 'SELECT $1::text'; + // const values = ['0']; + // const attributes = { + // ...DEFAULT_ATTRIBUTES, + // [AttributeNames.DB_STATEMENT]: query, + // [AttributeNames.PG_VALUES]: '[0]', + // }; + // const events: TimedEvent[] = []; + // const span = tracer.startSpan('test span'); + // tracer.withSpan(span, () => { + // const resNoPromise = client.query(query, values, (err, res) => { + // assert.strictEqual(err, null); + // assert.ok(res); + // runCallbackTest(span, attributes, events); + // done(); + // }); + // assert.strictEqual(resNoPromise, undefined, 'No promise is returned'); + // }); + // }); it('should intercept client.query({text, callback})', done => { const query = 'SELECT NOW()'; const attributes = { - [AttributeNames.COMPONENT]: PostgresPlugin.COMPONENT, - [AttributeNames.PEER_HOST]: CONFIG.host, - [AttributeNames.PEER_PORT]: CONFIG.port, + ...DEFAULT_ATTRIBUTES, [AttributeNames.DB_STATEMENT]: query, }; const events: TimedEvent[] = []; @@ -220,89 +220,109 @@ describe('pg@7.x', () => { }); }); - it('should intercept client.query({text}, callback)', done => { - const query = 'SELECT NOW()'; - const attributes = { - [AttributeNames.COMPONENT]: PostgresPlugin.COMPONENT, - [AttributeNames.PEER_HOST]: CONFIG.host, - [AttributeNames.PEER_PORT]: CONFIG.port, - [AttributeNames.DB_STATEMENT]: query, - }; - const events: TimedEvent[] = []; - const span = tracer.startSpan('test span'); - tracer.withSpan(span, () => { - const resNoPromise = client.query({ text: query }, (err, res) => { - assert.strictEqual(err, null); - assert.ok(res); - runCallbackTest(span, attributes, events); - done(); - }); - assert.strictEqual(resNoPromise, undefined, 'No promise is returned'); - }); - }); + // it('should intercept client.query({text}, callback)', done => { + // const query = 'SELECT NOW()'; + // const attributes = { + // ...DEFAULT_ATTRIBUTES, + // [AttributeNames.DB_STATEMENT]: query, + // }; + // const events: TimedEvent[] = []; + // const span = tracer.startSpan('test span'); + // tracer.withSpan(span, () => { + // const resNoPromise = client.query({ text: query }, (err, res) => { + // assert.strictEqual(err, null); + // assert.ok(res); + // runCallbackTest(span, attributes, events); + // done(); + // }); + // assert.strictEqual(resNoPromise, undefined, 'No promise is returned'); + // }); + // }); - it('should intercept client.query(text, values)', async () => { - const query = 'SELECT $1::text'; - const values = ['0']; - const attributes = { - [AttributeNames.COMPONENT]: PostgresPlugin.COMPONENT, - [AttributeNames.PEER_HOST]: CONFIG.host, - [AttributeNames.PEER_PORT]: CONFIG.port, - [AttributeNames.DB_STATEMENT]: query, - [AttributeNames.PG_VALUES]: values, - }; - const events: TimedEvent[] = []; - const span = tracer.startSpan('test span'); - await tracer.withSpan(span, async () => { - const resPromise = await client.query(query, values); - try { - assert.ok(resPromise); - runCallbackTest(span, attributes, events); - } catch (e) { - assert.ok(false, e.message); - } - }); - }); + // it('should intercept client.query(text, values)', async () => { + // const query = 'SELECT $1::text'; + // const values = ['0']; + // const attributes = { + // ...DEFAULT_ATTRIBUTES, + // [AttributeNames.DB_STATEMENT]: query, + // [AttributeNames.PG_VALUES]: '[0]', + // }; + // const events: TimedEvent[] = []; + // const span = tracer.startSpan('test span'); + // await tracer.withSpan(span, async () => { + // const resPromise = await client.query(query, values); + // try { + // assert.ok(resPromise); + // runCallbackTest(span, attributes, events); + // } catch (e) { + // assert.ok(false, e.message); + // } + // }); + // }); - it('should intercept client.query({text, values})', async () => { - const query = 'SELECT $1::text'; - const values = ['0']; - const attributes = { - [AttributeNames.COMPONENT]: PostgresPlugin.COMPONENT, - [AttributeNames.PEER_HOST]: CONFIG.host, - [AttributeNames.PEER_PORT]: CONFIG.port, - [AttributeNames.DB_STATEMENT]: query, - [AttributeNames.PG_VALUES]: values, - }; - const events: TimedEvent[] = []; - const span = tracer.startSpan('test span'); - await tracer.withSpan(span, async () => { - const resPromise = await client.query({ - text: query, - values: values, - }); - try { - assert.ok(resPromise); - runCallbackTest(span, attributes, events); - } catch (e) { - assert.ok(false, e.message); - } - }); - }); + // it('should intercept client.query({text, values})', async () => { + // const query = 'SELECT $1::text'; + // const values = ['0']; + // const attributes = { + // ...DEFAULT_ATTRIBUTES, + // [AttributeNames.DB_STATEMENT]: query, + // [AttributeNames.PG_VALUES]: '[0]', + // }; + // const events: TimedEvent[] = []; + // const span = tracer.startSpan('test span'); + // await tracer.withSpan(span, async () => { + // const resPromise = await client.query({ + // text: query, + // values: values, + // }); + // try { + // assert.ok(resPromise); + // runCallbackTest(span, attributes, events); + // } catch (e) { + // assert.ok(false, e.message); + // } + // }); + // }); + + // it('should intercept client.query(plan)', async () => { + // const name = 'fetch-text'; + // const query = 'SELECT $1::text'; + // const values = ['0']; + // const attributes = { + // ...DEFAULT_ATTRIBUTES, + // [AttributeNames.PG_PLAN]: name, + // [AttributeNames.DB_STATEMENT]: query, + // [AttributeNames.PG_VALUES]: '[0]', + // }; + // const events: TimedEvent[] = []; + // const span = tracer.startSpan('test span'); + + // await tracer.withSpan(span, async () => { + // try { + // const resPromise = await client.query({ + // name: name, + // text: query, + // values: values, + // }); + // assert.strictEqual(resPromise.command, 'SELECT'); + // runCallbackTest(span, attributes, events); + // } catch (e) { + // assert.ok(false, e.message); + // } + // }); + // }); it('should intercept client.query(text)', async () => { const query = 'SELECT NOW()'; const attributes = { - [AttributeNames.COMPONENT]: PostgresPlugin.COMPONENT, - [AttributeNames.PEER_HOST]: CONFIG.host, - [AttributeNames.PEER_PORT]: CONFIG.port, + ...DEFAULT_ATTRIBUTES, [AttributeNames.DB_STATEMENT]: query, }; const events: TimedEvent[] = []; const span = tracer.startSpan('test span'); await tracer.withSpan(span, async () => { - const resPromise = await client.query(query); try { + const resPromise = await client.query(query); assert.ok(resPromise); runCallbackTest(span, attributes, events); } catch (e) { @@ -315,6 +335,9 @@ describe('pg@7.x', () => { let events = 0; const queryHandler = (err: Error, res: pg.QueryResult) => { + const span = tracer.getCurrentSpan(); + assert.ok(span); + assert.strictEqual((span as any)['_ended'], false) if (err) { throw err; } @@ -329,10 +352,12 @@ describe('pg@7.x', () => { client.query(config.text, config.callback); // 1 client.query(config); // 2 client.query(config.text, queryHandler); // 3 + client.query(config.text, queryHandler); // 4 client.query(config.text); // Not using queryHandler - client.query(config); // 4 + client.query(config); // 5 + client.query(config); // 6 client.query(config.text, (err, res) => { - assert.strictEqual(events, 4); + assert.strictEqual(events, 6); done(); }); }); @@ -355,5 +380,24 @@ describe('pg@7.x', () => { client.query('SELECT NOW()', queryHandler); }); }); + + it('should preserve correct context even when using the same promise resolver in client.query()', done => { + const spans = [tracer.startSpan('span 1'), tracer.startSpan('span 2')]; + const currentSpans: (Span | null)[] = []; + const queryHandler = () => { + currentSpans.push(tracer.getCurrentSpan()); + if (currentSpans.length === 2) { + assert.deepStrictEqual(currentSpans, spans); + done(); + } + }; + + tracer.withSpan(spans[0], () => { + client.query('SELECT NOW()').then(queryHandler); + }); + tracer.withSpan(spans[1], () => { + client.query('SELECT NOW()').then(queryHandler); + }); + }); }); }); From 7504ba78e43bb37b8cd36bb7f463a635ae4b4b02 Mon Sep 17 00:00:00 2001 From: Mark Wolff Date: Fri, 18 Oct 2019 11:39:39 -0700 Subject: [PATCH 09/18] fix: lint errors --- .../src/utils.ts | 1 - .../test/pg.test.ts | 300 +++++++++--------- 2 files changed, 150 insertions(+), 151 deletions(-) diff --git a/packages/opentelemetry-plugin-postgres/src/utils.ts b/packages/opentelemetry-plugin-postgres/src/utils.ts index d7d1edd112..50baa1594b 100644 --- a/packages/opentelemetry-plugin-postgres/src/utils.ts +++ b/packages/opentelemetry-plugin-postgres/src/utils.ts @@ -78,7 +78,6 @@ export function _handleParameterizedQuery( span: Span, ...args: unknown[] ) { - // Set child span name const queryCommand = _getCommandFromText(args[0] as string); span.updateName(PostgresPlugin.BASE_SPAN_NAME + ':' + queryCommand); diff --git a/packages/opentelemetry-plugin-postgres/test/pg.test.ts b/packages/opentelemetry-plugin-postgres/test/pg.test.ts index b8f6ff4c95..a81b683d56 100644 --- a/packages/opentelemetry-plugin-postgres/test/pg.test.ts +++ b/packages/opentelemetry-plugin-postgres/test/pg.test.ts @@ -139,64 +139,64 @@ describe('pg@7.x', () => { }); describe('#client.query(...)', () => { - // it('should not return a promise if callback is provided', done => { - // const res = client.query('SELECT NOW()', (err, res) => { - // assert.strictEqual(err, null); - // done(); - // }); - // assert.strictEqual(res, undefined, 'No promise is returned'); - // }); - - // it('should return a promise if callback is provided', done => { - // const resPromise = client.query('SELECT NOW()'); - // resPromise - // .then(res => { - // assert.ok(res); - // done(); - // }) - // .catch((err: Error) => { - // assert.ok(false, err.message); - // }); - // }); - - // it('should intercept client.query(text, callback)', done => { - // const attributes = { - // ...DEFAULT_ATTRIBUTES, - // [AttributeNames.DB_STATEMENT]: 'SELECT NOW()', - // }; - // const events: TimedEvent[] = []; - // const span = tracer.startSpan('test span'); - // tracer.withSpan(span, () => { - // const res = client.query('SELECT NOW()', (err, res) => { - // assert.strictEqual(err, null); - // assert.ok(res); - // runCallbackTest(span, attributes, events); - // done(); - // }); - // assert.strictEqual(res, undefined, 'No promise is returned'); - // }); - // }); - - // it('should intercept client.query(text, values, callback)', done => { - // const query = 'SELECT $1::text'; - // const values = ['0']; - // const attributes = { - // ...DEFAULT_ATTRIBUTES, - // [AttributeNames.DB_STATEMENT]: query, - // [AttributeNames.PG_VALUES]: '[0]', - // }; - // const events: TimedEvent[] = []; - // const span = tracer.startSpan('test span'); - // tracer.withSpan(span, () => { - // const resNoPromise = client.query(query, values, (err, res) => { - // assert.strictEqual(err, null); - // assert.ok(res); - // runCallbackTest(span, attributes, events); - // done(); - // }); - // assert.strictEqual(resNoPromise, undefined, 'No promise is returned'); - // }); - // }); + it('should not return a promise if callback is provided', done => { + const res = client.query('SELECT NOW()', (err, res) => { + assert.strictEqual(err, null); + done(); + }); + assert.strictEqual(res, undefined, 'No promise is returned'); + }); + + it('should return a promise if callback is provided', done => { + const resPromise = client.query('SELECT NOW()'); + resPromise + .then(res => { + assert.ok(res); + done(); + }) + .catch((err: Error) => { + assert.ok(false, err.message); + }); + }); + + it('should intercept client.query(text, callback)', done => { + const attributes = { + ...DEFAULT_ATTRIBUTES, + [AttributeNames.DB_STATEMENT]: 'SELECT NOW()', + }; + const events: TimedEvent[] = []; + const span = tracer.startSpan('test span'); + tracer.withSpan(span, () => { + const res = client.query('SELECT NOW()', (err, res) => { + assert.strictEqual(err, null); + assert.ok(res); + runCallbackTest(span, attributes, events); + done(); + }); + assert.strictEqual(res, undefined, 'No promise is returned'); + }); + }); + + it('should intercept client.query(text, values, callback)', done => { + const query = 'SELECT $1::text'; + const values = ['0']; + const attributes = { + ...DEFAULT_ATTRIBUTES, + [AttributeNames.DB_STATEMENT]: query, + [AttributeNames.PG_VALUES]: '[0]', + }; + const events: TimedEvent[] = []; + const span = tracer.startSpan('test span'); + tracer.withSpan(span, () => { + const resNoPromise = client.query(query, values, (err, res) => { + assert.strictEqual(err, null); + assert.ok(res); + runCallbackTest(span, attributes, events); + done(); + }); + assert.strictEqual(resNoPromise, undefined, 'No promise is returned'); + }); + }); it('should intercept client.query({text, callback})', done => { const query = 'SELECT NOW()'; @@ -220,97 +220,97 @@ describe('pg@7.x', () => { }); }); - // it('should intercept client.query({text}, callback)', done => { - // const query = 'SELECT NOW()'; - // const attributes = { - // ...DEFAULT_ATTRIBUTES, - // [AttributeNames.DB_STATEMENT]: query, - // }; - // const events: TimedEvent[] = []; - // const span = tracer.startSpan('test span'); - // tracer.withSpan(span, () => { - // const resNoPromise = client.query({ text: query }, (err, res) => { - // assert.strictEqual(err, null); - // assert.ok(res); - // runCallbackTest(span, attributes, events); - // done(); - // }); - // assert.strictEqual(resNoPromise, undefined, 'No promise is returned'); - // }); - // }); - - // it('should intercept client.query(text, values)', async () => { - // const query = 'SELECT $1::text'; - // const values = ['0']; - // const attributes = { - // ...DEFAULT_ATTRIBUTES, - // [AttributeNames.DB_STATEMENT]: query, - // [AttributeNames.PG_VALUES]: '[0]', - // }; - // const events: TimedEvent[] = []; - // const span = tracer.startSpan('test span'); - // await tracer.withSpan(span, async () => { - // const resPromise = await client.query(query, values); - // try { - // assert.ok(resPromise); - // runCallbackTest(span, attributes, events); - // } catch (e) { - // assert.ok(false, e.message); - // } - // }); - // }); - - // it('should intercept client.query({text, values})', async () => { - // const query = 'SELECT $1::text'; - // const values = ['0']; - // const attributes = { - // ...DEFAULT_ATTRIBUTES, - // [AttributeNames.DB_STATEMENT]: query, - // [AttributeNames.PG_VALUES]: '[0]', - // }; - // const events: TimedEvent[] = []; - // const span = tracer.startSpan('test span'); - // await tracer.withSpan(span, async () => { - // const resPromise = await client.query({ - // text: query, - // values: values, - // }); - // try { - // assert.ok(resPromise); - // runCallbackTest(span, attributes, events); - // } catch (e) { - // assert.ok(false, e.message); - // } - // }); - // }); - - // it('should intercept client.query(plan)', async () => { - // const name = 'fetch-text'; - // const query = 'SELECT $1::text'; - // const values = ['0']; - // const attributes = { - // ...DEFAULT_ATTRIBUTES, - // [AttributeNames.PG_PLAN]: name, - // [AttributeNames.DB_STATEMENT]: query, - // [AttributeNames.PG_VALUES]: '[0]', - // }; - // const events: TimedEvent[] = []; - // const span = tracer.startSpan('test span'); - - // await tracer.withSpan(span, async () => { - // try { - // const resPromise = await client.query({ - // name: name, - // text: query, - // values: values, - // }); - // assert.strictEqual(resPromise.command, 'SELECT'); - // runCallbackTest(span, attributes, events); - // } catch (e) { - // assert.ok(false, e.message); - // } - // }); - // }); + it('should intercept client.query({text}, callback)', done => { + const query = 'SELECT NOW()'; + const attributes = { + ...DEFAULT_ATTRIBUTES, + [AttributeNames.DB_STATEMENT]: query, + }; + const events: TimedEvent[] = []; + const span = tracer.startSpan('test span'); + tracer.withSpan(span, () => { + const resNoPromise = client.query({ text: query }, (err, res) => { + assert.strictEqual(err, null); + assert.ok(res); + runCallbackTest(span, attributes, events); + done(); + }); + assert.strictEqual(resNoPromise, undefined, 'No promise is returned'); + }); + }); + + it('should intercept client.query(text, values)', async () => { + const query = 'SELECT $1::text'; + const values = ['0']; + const attributes = { + ...DEFAULT_ATTRIBUTES, + [AttributeNames.DB_STATEMENT]: query, + [AttributeNames.PG_VALUES]: '[0]', + }; + const events: TimedEvent[] = []; + const span = tracer.startSpan('test span'); + await tracer.withSpan(span, async () => { + const resPromise = await client.query(query, values); + try { + assert.ok(resPromise); + runCallbackTest(span, attributes, events); + } catch (e) { + assert.ok(false, e.message); + } + }); + }); + + it('should intercept client.query({text, values})', async () => { + const query = 'SELECT $1::text'; + const values = ['0']; + const attributes = { + ...DEFAULT_ATTRIBUTES, + [AttributeNames.DB_STATEMENT]: query, + [AttributeNames.PG_VALUES]: '[0]', + }; + const events: TimedEvent[] = []; + const span = tracer.startSpan('test span'); + await tracer.withSpan(span, async () => { + const resPromise = await client.query({ + text: query, + values: values, + }); + try { + assert.ok(resPromise); + runCallbackTest(span, attributes, events); + } catch (e) { + assert.ok(false, e.message); + } + }); + }); + + it('should intercept client.query(plan)', async () => { + const name = 'fetch-text'; + const query = 'SELECT $1::text'; + const values = ['0']; + const attributes = { + ...DEFAULT_ATTRIBUTES, + [AttributeNames.PG_PLAN]: name, + [AttributeNames.DB_STATEMENT]: query, + [AttributeNames.PG_VALUES]: '[0]', + }; + const events: TimedEvent[] = []; + const span = tracer.startSpan('test span'); + + await tracer.withSpan(span, async () => { + try { + const resPromise = await client.query({ + name: name, + text: query, + values: values, + }); + assert.strictEqual(resPromise.command, 'SELECT'); + runCallbackTest(span, attributes, events); + } catch (e) { + assert.ok(false, e.message); + } + }); + }); it('should intercept client.query(text)', async () => { const query = 'SELECT NOW()'; @@ -337,7 +337,7 @@ describe('pg@7.x', () => { const queryHandler = (err: Error, res: pg.QueryResult) => { const span = tracer.getCurrentSpan(); assert.ok(span); - assert.strictEqual((span as any)['_ended'], false) + assert.strictEqual((span as any)['_ended'], false); if (err) { throw err; } From bfb19d33aef6a075950a5426f8f9cb67e92efb82 Mon Sep 17 00:00:00 2001 From: Mark Wolff Date: Mon, 21 Oct 2019 12:11:18 -0700 Subject: [PATCH 10/18] refactor: start named spans in query handlers --- .circleci/config.yml | 10 ++- .../package.json | 10 ++- .../opentelemetry-plugin-postgres/src/pg.ts | 37 +++------ .../src/utils.ts | 80 +++++++++++++------ .../test/pg.test.ts | 39 ++++----- 5 files changed, 95 insertions(+), 81 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 83c1699fc4..1d1ca5b982 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -1,10 +1,16 @@ version: 2 test_env: &test_env - TEST_POSTGRES: 1 + RUN_POSTGRES_TESTS: 1 postgres_service: &postgres_service - image: postgres:alpine + image: circleci/postgres:9.6-alpine + environment: + POSTGRES_USER: circle_user + POSTGRES_DB: circle_database + POSTGRES_PASSWORD: circle_password + POSTGRES_HOST: localhost + POSTGRES_PORT: 5432 node_unit_tests: &node_unit_tests steps: diff --git a/packages/opentelemetry-plugin-postgres/package.json b/packages/opentelemetry-plugin-postgres/package.json index cdd608215d..1a785a4f7f 100644 --- a/packages/opentelemetry-plugin-postgres/package.json +++ b/packages/opentelemetry-plugin-postgres/package.json @@ -9,8 +9,10 @@ "scripts": { "test": "nyc ts-mocha -p tsconfig.json 'test/**/*.test.ts'", "test:debug": "ts-mocha --inspect-brk --no-timeouts -p tsconfig.json 'test/**/*.test.ts'", + "test:local": "cross-env RUN_POSTGRES_TESTS_LOCAL=true yarn test", "tdd": "yarn test -- --watch-extensions ts --watch", "clean": "rimraf build/*", + "codecov": "nyc report --reporter=json && codecov -f coverage/*.json -p ../../", "check": "gts check", "compile": "tsc -p .", "fix": "gts fix", @@ -59,10 +61,10 @@ "typescript": "^3.6.3" }, "dependencies": { - "@opentelemetry/core": "^0.1.0", - "@opentelemetry/node": "^0.1.0", - "@opentelemetry/tracing": "^0.1.0", - "@opentelemetry/types": "^0.1.0", + "@opentelemetry/core": "^0.1.1", + "@opentelemetry/node": "^0.1.1", + "@opentelemetry/tracing": "^0.1.1", + "@opentelemetry/types": "^0.1.1", "shimmer": "^1.2.1" } } diff --git a/packages/opentelemetry-plugin-postgres/src/pg.ts b/packages/opentelemetry-plugin-postgres/src/pg.ts index 7c98905c56..116f54ef0d 100644 --- a/packages/opentelemetry-plugin-postgres/src/pg.ts +++ b/packages/opentelemetry-plugin-postgres/src/pg.ts @@ -15,8 +15,7 @@ */ import { BasePlugin } from '@opentelemetry/core'; -import { SpanKind, CanonicalCode } from '@opentelemetry/types'; -import { AttributeNames } from './enums'; +import { CanonicalCode, Span } from '@opentelemetry/types'; import { PostgresPluginOptions, PgClientExtended, @@ -70,17 +69,19 @@ export class PostgresPlugin extends BasePlugin { ...args: unknown[] ) { let callbackProvided = false; - const span = plugin._pgStartSpan(this); + let span: Span; // Handle different client.query(...) signatures if (typeof args[0] === 'string') { if (args.length > 1 && args[1] instanceof Array) { - utils._handleParameterizedQuery.call(this, span, ...args); + span = utils.handleParameterizedQuery.call(this, plugin._tracer, ...args); } else { - utils._handleTextQuery.call(this, span, ...args); + span = utils.handleTextQuery.call(this, plugin._tracer, ...args); } } else if (typeof args[0] === 'object') { - utils._handleConfigQuery.call(this, span, ...args); + span = utils.handleConfigQuery.call(this, plugin._tracer, ...args); + } else { + return original.apply(this, args as never); } // Bind callback to parent span @@ -88,7 +89,7 @@ export class PostgresPlugin extends BasePlugin { const parentSpan = plugin._tracer.getCurrentSpan(); if (typeof args[args.length - 1] === 'function') { // Patch ParameterQuery callback - args[args.length - 1] = utils._patchCallback(span, args[ + args[args.length - 1] = utils.patchCallback(span, args[ args.length - 1 ] as PostgresCallback); // If a parent span exists, bind the callback @@ -102,7 +103,7 @@ export class PostgresPlugin extends BasePlugin { typeof (args[0] as PgPluginQueryConfig).callback === 'function' ) { // Patch ConfigQuery callback - let callback = utils._patchCallback( + let callback = utils.patchCallback( span, (args[0] as PgPluginQueryConfig).callback! ); @@ -113,7 +114,7 @@ export class PostgresPlugin extends BasePlugin { // Copy the callback instead of writing to args.callback so that we don't modify user's // original callback reference - args[0] = { ...args[0], callback }; + args[0] = { ...args[0] as object, callback }; callbackProvided = true; } } @@ -155,24 +156,6 @@ export class PostgresPlugin extends BasePlugin { }; }; } - - // Private helper function to start a span - private _pgStartSpan(client: pgTypes.Client & PgClientExtended) { - const jdbcString = utils._getJDBCString(client.connectionParameters); - return this._tracer.startSpan(PostgresPlugin.BASE_SPAN_NAME, { - kind: SpanKind.CLIENT, - parent: this._tracer.getCurrentSpan() || undefined, - attributes: { - [AttributeNames.COMPONENT]: PostgresPlugin.COMPONENT, // required - [AttributeNames.DB_INSTANCE]: client.connectionParameters.database, // required - [AttributeNames.DB_TYPE]: PostgresPlugin.DB_TYPE, // required - [AttributeNames.PEER_ADDRESS]: jdbcString, // required - [AttributeNames.PEER_HOSTNAME]: client.connectionParameters.host, // required - [AttributeNames.PEER_PORT]: client.connectionParameters.port, - [AttributeNames.DB_USER]: client.connectionParameters.user, - }, - }); - } } export const plugin = new PostgresPlugin(PostgresPlugin.COMPONENT); diff --git a/packages/opentelemetry-plugin-postgres/src/utils.ts b/packages/opentelemetry-plugin-postgres/src/utils.ts index 50baa1594b..730d8f25fe 100644 --- a/packages/opentelemetry-plugin-postgres/src/utils.ts +++ b/packages/opentelemetry-plugin-postgres/src/utils.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import { Span, CanonicalCode } from '@opentelemetry/types'; +import { Span, CanonicalCode, Tracer, SpanKind } from '@opentelemetry/types'; import { AttributeNames } from './enums'; import { PgClientExtended, @@ -25,12 +25,19 @@ import { import * as pgTypes from 'pg'; import { PostgresPlugin } from './pg'; -export function _arrayStringifyHelper(arr: Array): string { +function getJDBCString(params: PgClientConnectionParams) { + const host = params.host || 'localhost'; // postgres defaults to localhost + const port = params.port || 5432; // postgres defaults to port 5432 + const database = params.database || ''; + return `jdbc:postgresql://${host}:${port}/${database}`; +} + +export function arrayStringifyHelper(arr: Array): string { return '[' + arr.toString() + ']'; } // Helper function to get a low cardinality command name from the full text query -export function _getCommandFromText(text?: string): string { +export function getCommandFromText(text?: string): string { if (text) { const words = text.split(' '); if (words && words.length > 0) { @@ -40,70 +47,73 @@ export function _getCommandFromText(text?: string): string { return 'unknown'; } -export function _getJDBCString(params: PgClientConnectionParams) { - const host = params.host || 'localhost'; // postgres defaults to localhost - const port = params.port || 5432; // postgres defaults to port 5432 - const database = params.database || ''; - return `jdbc:postgresql://${host}:${port}/${database}`; -} - // Queries where args[0] is a QueryConfig -export function _handleConfigQuery( +export function handleConfigQuery( this: pgTypes.Client & PgClientExtended, - span: Span, + tracer: Tracer, ...args: unknown[] ) { const argsConfig = args[0] as PgPluginQueryConfig; + // Set child span name + const queryCommand = getCommandFromText(argsConfig.name || argsConfig.text); + const name = PostgresPlugin.BASE_SPAN_NAME + ':' + queryCommand + const span = pgStartSpan(tracer, this, name); + // Set attributes span.setAttribute(AttributeNames.DB_STATEMENT, argsConfig.text); if (argsConfig.values instanceof Array) { span.setAttribute( AttributeNames.PG_VALUES, - _arrayStringifyHelper(argsConfig.values) + arrayStringifyHelper(argsConfig.values) ); } + // Set plan name attribute, if present if (argsConfig.name) { span.setAttribute(AttributeNames.PG_PLAN, argsConfig.name); } - // Update span name with query command; prefer plan name, if available - const queryCommand = _getCommandFromText(argsConfig.name || argsConfig.text); - span.updateName(PostgresPlugin.BASE_SPAN_NAME + ':' + queryCommand); + return span; } // Queries where args[1] is a 'values' array -export function _handleParameterizedQuery( +export function handleParameterizedQuery( this: pgTypes.Client & PgClientExtended, - span: Span, + tracer: Tracer, ...args: unknown[] ) { // Set child span name - const queryCommand = _getCommandFromText(args[0] as string); - span.updateName(PostgresPlugin.BASE_SPAN_NAME + ':' + queryCommand); + const queryCommand = getCommandFromText(args[0] as string); + const name = PostgresPlugin.BASE_SPAN_NAME + ':' + queryCommand; + const span = pgStartSpan(tracer, this, name); // Set attributes span.setAttribute(AttributeNames.DB_STATEMENT, args[0]); if (args[1] instanceof Array) { - span.setAttribute(AttributeNames.PG_VALUES, _arrayStringifyHelper(args[1])); + span.setAttribute(AttributeNames.PG_VALUES, arrayStringifyHelper(args[1])); } + + return span; } // Queries where args[0] is a text query and 'values' was not specified -export function _handleTextQuery( +export function handleTextQuery( this: pgTypes.Client & PgClientExtended, - span: Span, + tracer: Tracer, ...args: unknown[] ) { // Set child span name - const queryCommand = _getCommandFromText(args[0] as string); - span.updateName(PostgresPlugin.BASE_SPAN_NAME + ':' + queryCommand); + const queryCommand = getCommandFromText(args[0] as string); + const name = PostgresPlugin.BASE_SPAN_NAME + ':' + queryCommand; + const span = pgStartSpan(tracer, this, name); // Set attributes span.setAttribute(AttributeNames.DB_STATEMENT, args[0]); + + return span; } -export function _patchCallback( +export function patchCallback( span: Span, cb: PostgresCallback ): PostgresCallback { @@ -125,3 +135,21 @@ export function _patchCallback( return originalCb.call(this, err, res); }; } + +// Private helper function to start a span +export function pgStartSpan(tracer: Tracer, client: pgTypes.Client & PgClientExtended, name: string) { + const jdbcString = getJDBCString(client.connectionParameters); + return tracer.startSpan(name, { + kind: SpanKind.CLIENT, + parent: tracer.getCurrentSpan() || undefined, + attributes: { + [AttributeNames.COMPONENT]: PostgresPlugin.COMPONENT, // required + [AttributeNames.DB_INSTANCE]: client.connectionParameters.database, // required + [AttributeNames.DB_TYPE]: PostgresPlugin.DB_TYPE, // required + [AttributeNames.PEER_ADDRESS]: jdbcString, // required + [AttributeNames.PEER_HOSTNAME]: client.connectionParameters.host, // required + [AttributeNames.PEER_PORT]: client.connectionParameters.port, + [AttributeNames.DB_USER]: client.connectionParameters.user, + }, + }); +} diff --git a/packages/opentelemetry-plugin-postgres/test/pg.test.ts b/packages/opentelemetry-plugin-postgres/test/pg.test.ts index a81b683d56..7add72c3e6 100644 --- a/packages/opentelemetry-plugin-postgres/test/pg.test.ts +++ b/packages/opentelemetry-plugin-postgres/test/pg.test.ts @@ -27,16 +27,15 @@ import * as assert from 'assert'; import * as pg from 'pg'; import * as assertionUtils from './assertionUtils'; import * as testUtils from './testUtils'; -import { _arrayStringifyHelper } from '../src/utils'; const memoryExporter = new InMemorySpanExporter(); const CONFIG = { - user: 'postgres', - password: 'test', - database: 'postgres', - host: '127.0.0.1', - port: 54320, + user: process.env.POSTGRES_USER || 'postgres', + password: process.env.POSTGRESS_PASSWORD || 'test', + database: process.env.POSTGRES_DB || 'postgres', + host: process.env.POSTGRES_HOST || 'localhost', + port: process.env.POSTGRES_PORT ? parseInt(process.env.POSTGRES_PORT, 10) : 54320, }; const DEFAULT_ATTRIBUTES = { @@ -44,7 +43,7 @@ const DEFAULT_ATTRIBUTES = { [AttributeNames.DB_INSTANCE]: CONFIG.database, [AttributeNames.DB_TYPE]: PostgresPlugin.DB_TYPE, [AttributeNames.PEER_HOSTNAME]: CONFIG.host, - [AttributeNames.PEER_ADDRESS]: 'jdbc:postgresql://127.0.0.1:54320/postgres', + [AttributeNames.PEER_ADDRESS]: `jdbc:postgresql://${CONFIG.host}:${CONFIG.port}/${CONFIG.database}`, [AttributeNames.PEER_PORT]: CONFIG.port, [AttributeNames.DB_USER]: CONFIG.user, }; @@ -67,11 +66,11 @@ describe('pg@7.x', () => { let client: pg.Client; const tracer = new NodeTracer(); const logger = new NoopLogger(); - const testPostgres = process.env.TEST_POSTGRES; // For CI: assumes local postgres db is already available - const testPostgresLocally = process.env.TEST_POSTGRES_LOCAL; // For local: spins up local postgres db via docker + const testPostgres = process.env.RUN_POSTGRES_TESTS; // For CI: assumes local postgres db is already available + const testPostgresLocally = process.env.RUN_POSTGRES_TESTS_LOCAL; // For local: spins up local postgres db via docker const shouldTest = true || testPostgres || testPostgresLocally; // Skips these tests if false (default) - before(function(ready) { + before(async function() { if (!shouldTest) { // this.skip() workaround // https://github.com/mochajs/mocha/issues/2683#issuecomment-375629901 @@ -82,24 +81,20 @@ describe('pg@7.x', () => { if (testPostgresLocally) { testUtils.startDocker(); } - client = new pg.Client(CONFIG); - function connect() { - client.connect(err => { - if (err) { - setTimeout(connect, 500); - return; - } - ready(); - }); + client = new pg.Client(CONFIG); + try { + await client.connect(); + } catch (e) { + throw e; } - connect(); }); - after(done => { + + after(async () => { if (testPostgresLocally) { testUtils.cleanUpDocker(); } - client.end(done); + await client.end(); }); beforeEach(function() { From 64a2a8b67f3e7a50a20d72d56d6314e028a9b5d3 Mon Sep 17 00:00:00 2001 From: Mark Wolff Date: Mon, 21 Oct 2019 12:11:48 -0700 Subject: [PATCH 11/18] fix: linting errors --- packages/opentelemetry-plugin-postgres/src/pg.ts | 8 ++++++-- packages/opentelemetry-plugin-postgres/src/utils.ts | 8 ++++++-- packages/opentelemetry-plugin-postgres/test/pg.test.ts | 4 +++- 3 files changed, 15 insertions(+), 5 deletions(-) diff --git a/packages/opentelemetry-plugin-postgres/src/pg.ts b/packages/opentelemetry-plugin-postgres/src/pg.ts index 116f54ef0d..0052fa2cca 100644 --- a/packages/opentelemetry-plugin-postgres/src/pg.ts +++ b/packages/opentelemetry-plugin-postgres/src/pg.ts @@ -74,7 +74,11 @@ export class PostgresPlugin extends BasePlugin { // Handle different client.query(...) signatures if (typeof args[0] === 'string') { if (args.length > 1 && args[1] instanceof Array) { - span = utils.handleParameterizedQuery.call(this, plugin._tracer, ...args); + span = utils.handleParameterizedQuery.call( + this, + plugin._tracer, + ...args + ); } else { span = utils.handleTextQuery.call(this, plugin._tracer, ...args); } @@ -114,7 +118,7 @@ export class PostgresPlugin extends BasePlugin { // Copy the callback instead of writing to args.callback so that we don't modify user's // original callback reference - args[0] = { ...args[0] as object, callback }; + args[0] = { ...(args[0] as object), callback }; callbackProvided = true; } } diff --git a/packages/opentelemetry-plugin-postgres/src/utils.ts b/packages/opentelemetry-plugin-postgres/src/utils.ts index 730d8f25fe..d8af368bad 100644 --- a/packages/opentelemetry-plugin-postgres/src/utils.ts +++ b/packages/opentelemetry-plugin-postgres/src/utils.ts @@ -57,7 +57,7 @@ export function handleConfigQuery( // Set child span name const queryCommand = getCommandFromText(argsConfig.name || argsConfig.text); - const name = PostgresPlugin.BASE_SPAN_NAME + ':' + queryCommand + const name = PostgresPlugin.BASE_SPAN_NAME + ':' + queryCommand; const span = pgStartSpan(tracer, this, name); // Set attributes @@ -137,7 +137,11 @@ export function patchCallback( } // Private helper function to start a span -export function pgStartSpan(tracer: Tracer, client: pgTypes.Client & PgClientExtended, name: string) { +export function pgStartSpan( + tracer: Tracer, + client: pgTypes.Client & PgClientExtended, + name: string +) { const jdbcString = getJDBCString(client.connectionParameters); return tracer.startSpan(name, { kind: SpanKind.CLIENT, diff --git a/packages/opentelemetry-plugin-postgres/test/pg.test.ts b/packages/opentelemetry-plugin-postgres/test/pg.test.ts index 7add72c3e6..0ff0d18183 100644 --- a/packages/opentelemetry-plugin-postgres/test/pg.test.ts +++ b/packages/opentelemetry-plugin-postgres/test/pg.test.ts @@ -35,7 +35,9 @@ const CONFIG = { password: process.env.POSTGRESS_PASSWORD || 'test', database: process.env.POSTGRES_DB || 'postgres', host: process.env.POSTGRES_HOST || 'localhost', - port: process.env.POSTGRES_PORT ? parseInt(process.env.POSTGRES_PORT, 10) : 54320, + port: process.env.POSTGRES_PORT + ? parseInt(process.env.POSTGRES_PORT, 10) + : 54320, }; const DEFAULT_ATTRIBUTES = { From d81e4b0e922072b55ee83f6828ba787e4ddaeaef Mon Sep 17 00:00:00 2001 From: Mark Wolff Date: Mon, 21 Oct 2019 12:51:34 -0700 Subject: [PATCH 12/18] fix: circleci config, make pg helpers nonexported --- .circleci/config.yml | 11 ++-- .../src/utils.ts | 66 +++++++++---------- 2 files changed, 36 insertions(+), 41 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 1d1ca5b982..0ee223264a 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -2,15 +2,14 @@ version: 2 test_env: &test_env RUN_POSTGRES_TESTS: 1 + POSTGRES_USER: circle_user + POSTGRES_DB: circle_database + POSTGRES_PASSWORD: circle_password + POSTGRES_HOST: localhost + POSTGRES_PORT: 5432 postgres_service: &postgres_service image: circleci/postgres:9.6-alpine - environment: - POSTGRES_USER: circle_user - POSTGRES_DB: circle_database - POSTGRES_PASSWORD: circle_password - POSTGRES_HOST: localhost - POSTGRES_PORT: 5432 node_unit_tests: &node_unit_tests steps: diff --git a/packages/opentelemetry-plugin-postgres/src/utils.ts b/packages/opentelemetry-plugin-postgres/src/utils.ts index d8af368bad..745c6e5bd7 100644 --- a/packages/opentelemetry-plugin-postgres/src/utils.ts +++ b/packages/opentelemetry-plugin-postgres/src/utils.ts @@ -25,6 +25,17 @@ import { import * as pgTypes from 'pg'; import { PostgresPlugin } from './pg'; +function arrayStringifyHelper(arr: Array): string { + return '[' + arr.toString() + ']'; +} + +// Helper function to get a low cardinality command name from the full text query +function getCommandFromText(text?: string): string { + if (!text) return 'unknown'; + const words = text.split(' '); + return (words[0].length > 0) ? words[0] : 'unknown'; +} + function getJDBCString(params: PgClientConnectionParams) { const host = params.host || 'localhost'; // postgres defaults to localhost const port = params.port || 5432; // postgres defaults to port 5432 @@ -32,19 +43,26 @@ function getJDBCString(params: PgClientConnectionParams) { return `jdbc:postgresql://${host}:${port}/${database}`; } -export function arrayStringifyHelper(arr: Array): string { - return '[' + arr.toString() + ']'; -} - -// Helper function to get a low cardinality command name from the full text query -export function getCommandFromText(text?: string): string { - if (text) { - const words = text.split(' '); - if (words && words.length > 0) { - return words[0]; - } - } - return 'unknown'; +// Private helper function to start a span +function pgStartSpan( + tracer: Tracer, + client: pgTypes.Client & PgClientExtended, + name: string +) { + const jdbcString = getJDBCString(client.connectionParameters); + return tracer.startSpan(name, { + kind: SpanKind.CLIENT, + parent: tracer.getCurrentSpan() || undefined, + attributes: { + [AttributeNames.COMPONENT]: PostgresPlugin.COMPONENT, // required + [AttributeNames.DB_INSTANCE]: client.connectionParameters.database, // required + [AttributeNames.DB_TYPE]: PostgresPlugin.DB_TYPE, // required + [AttributeNames.PEER_ADDRESS]: jdbcString, // required + [AttributeNames.PEER_HOSTNAME]: client.connectionParameters.host, // required + [AttributeNames.PEER_PORT]: client.connectionParameters.port, + [AttributeNames.DB_USER]: client.connectionParameters.user, + }, + }); } // Queries where args[0] is a QueryConfig @@ -135,25 +153,3 @@ export function patchCallback( return originalCb.call(this, err, res); }; } - -// Private helper function to start a span -export function pgStartSpan( - tracer: Tracer, - client: pgTypes.Client & PgClientExtended, - name: string -) { - const jdbcString = getJDBCString(client.connectionParameters); - return tracer.startSpan(name, { - kind: SpanKind.CLIENT, - parent: tracer.getCurrentSpan() || undefined, - attributes: { - [AttributeNames.COMPONENT]: PostgresPlugin.COMPONENT, // required - [AttributeNames.DB_INSTANCE]: client.connectionParameters.database, // required - [AttributeNames.DB_TYPE]: PostgresPlugin.DB_TYPE, // required - [AttributeNames.PEER_ADDRESS]: jdbcString, // required - [AttributeNames.PEER_HOSTNAME]: client.connectionParameters.host, // required - [AttributeNames.PEER_PORT]: client.connectionParameters.port, - [AttributeNames.DB_USER]: client.connectionParameters.user, - }, - }); -} From 208d3a99636ca3d92ffb4c6e9f4ae8b36ad58cd0 Mon Sep 17 00:00:00 2001 From: Mark Wolff Date: Mon, 21 Oct 2019 12:52:12 -0700 Subject: [PATCH 13/18] fix: linting --- packages/opentelemetry-plugin-postgres/src/utils.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/opentelemetry-plugin-postgres/src/utils.ts b/packages/opentelemetry-plugin-postgres/src/utils.ts index 745c6e5bd7..062236848a 100644 --- a/packages/opentelemetry-plugin-postgres/src/utils.ts +++ b/packages/opentelemetry-plugin-postgres/src/utils.ts @@ -33,7 +33,7 @@ function arrayStringifyHelper(arr: Array): string { function getCommandFromText(text?: string): string { if (!text) return 'unknown'; const words = text.split(' '); - return (words[0].length > 0) ? words[0] : 'unknown'; + return words[0].length > 0 ? words[0] : 'unknown'; } function getJDBCString(params: PgClientConnectionParams) { From f98647689340ce38313fd839acced523f25a7d79 Mon Sep 17 00:00:00 2001 From: Mark Wolff Date: Mon, 21 Oct 2019 13:08:26 -0700 Subject: [PATCH 14/18] docs: add supported versions --- .circleci/config.yml | 2 +- packages/opentelemetry-plugin-postgres/README.md | 4 ++++ packages/opentelemetry-plugin-postgres/test/pg.test.ts | 1 - 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 0ee223264a..057e959cd5 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -2,7 +2,7 @@ version: 2 test_env: &test_env RUN_POSTGRES_TESTS: 1 - POSTGRES_USER: circle_user + POSTGRES_USER: root POSTGRES_DB: circle_database POSTGRES_PASSWORD: circle_password POSTGRES_HOST: localhost diff --git a/packages/opentelemetry-plugin-postgres/README.md b/packages/opentelemetry-plugin-postgres/README.md index db2d211a1d..f18233a45e 100644 --- a/packages/opentelemetry-plugin-postgres/README.md +++ b/packages/opentelemetry-plugin-postgres/README.md @@ -23,6 +23,10 @@ const opentelemetry = require('@opentelemetry/plugin-postgres'); // TODO: DEMONSTRATE API ``` +## Supported Versions + +- [pg](https://npmjs.com/package/pg): `7.x` + ## Useful links - For more information on OpenTelemetry, visit: - For more about OpenTelemetry JavaScript: diff --git a/packages/opentelemetry-plugin-postgres/test/pg.test.ts b/packages/opentelemetry-plugin-postgres/test/pg.test.ts index 0ff0d18183..15fa62ab35 100644 --- a/packages/opentelemetry-plugin-postgres/test/pg.test.ts +++ b/packages/opentelemetry-plugin-postgres/test/pg.test.ts @@ -32,7 +32,6 @@ const memoryExporter = new InMemorySpanExporter(); const CONFIG = { user: process.env.POSTGRES_USER || 'postgres', - password: process.env.POSTGRESS_PASSWORD || 'test', database: process.env.POSTGRES_DB || 'postgres', host: process.env.POSTGRES_HOST || 'localhost', port: process.env.POSTGRES_PORT From 644f5f1ca3e682d156fa9f6e0e5ac0a9ea843802 Mon Sep 17 00:00:00 2001 From: Mark Wolff Date: Mon, 21 Oct 2019 13:50:58 -0700 Subject: [PATCH 15/18] fix: pass PG env to spawned container --- .circleci/config.yml | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 057e959cd5..478f60275f 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -2,14 +2,16 @@ version: 2 test_env: &test_env RUN_POSTGRES_TESTS: 1 - POSTGRES_USER: root + POSTGRES_USER: postgres POSTGRES_DB: circle_database - POSTGRES_PASSWORD: circle_password POSTGRES_HOST: localhost POSTGRES_PORT: 5432 postgres_service: &postgres_service image: circleci/postgres:9.6-alpine + environment: # env to pass to CircleCI, specified values must match test_env + POSTGRES_USER: postgres + POSTGRES_DB: circle_database node_unit_tests: &node_unit_tests steps: From b3a83b4254e6fb9827ecb7fd4a5146eb2db5d834 Mon Sep 17 00:00:00 2001 From: Mark Wolff Date: Wed, 23 Oct 2019 09:30:33 -0700 Subject: [PATCH 16/18] fix: remove hardcoded shouldTest --- packages/opentelemetry-plugin-postgres/test/pg.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/opentelemetry-plugin-postgres/test/pg.test.ts b/packages/opentelemetry-plugin-postgres/test/pg.test.ts index 15fa62ab35..3869ca61fa 100644 --- a/packages/opentelemetry-plugin-postgres/test/pg.test.ts +++ b/packages/opentelemetry-plugin-postgres/test/pg.test.ts @@ -69,7 +69,7 @@ describe('pg@7.x', () => { const logger = new NoopLogger(); const testPostgres = process.env.RUN_POSTGRES_TESTS; // For CI: assumes local postgres db is already available const testPostgresLocally = process.env.RUN_POSTGRES_TESTS_LOCAL; // For local: spins up local postgres db via docker - const shouldTest = true || testPostgres || testPostgresLocally; // Skips these tests if false (default) + const shouldTest = testPostgres || testPostgresLocally; // Skips these tests if false (default) before(async function() { if (!shouldTest) { From 4e24474ace5fc2b81dbe52106b2c8aaf1c25ffbb Mon Sep 17 00:00:00 2001 From: Mark Wolff Date: Mon, 28 Oct 2019 16:32:07 -0700 Subject: [PATCH 17/18] test: add span tests for pg driver errors --- .../opentelemetry-plugin-postgres/src/pg.ts | 26 +++++----- .../src/utils.ts | 34 +++++++++++-- .../test/assertionUtils.ts | 28 ++++------- .../test/pg.test.ts | 48 ++++++++++++++++--- 4 files changed, 93 insertions(+), 43 deletions(-) diff --git a/packages/opentelemetry-plugin-postgres/src/pg.ts b/packages/opentelemetry-plugin-postgres/src/pg.ts index 0052fa2cca..e41ceee08a 100644 --- a/packages/opentelemetry-plugin-postgres/src/pg.ts +++ b/packages/opentelemetry-plugin-postgres/src/pg.ts @@ -34,7 +34,7 @@ export class PostgresPlugin extends BasePlugin { static readonly BASE_SPAN_NAME = PostgresPlugin.COMPONENT + '.query'; - readonly supportedVersions = ['^7.12.1']; + readonly supportedVersions = ['7.*']; constructor(readonly moduleName: string) { super(); @@ -68,7 +68,6 @@ export class PostgresPlugin extends BasePlugin { this: pgTypes.Client & PgClientExtended, ...args: unknown[] ) { - let callbackProvided = false; let span: Span; // Handle different client.query(...) signatures @@ -85,7 +84,12 @@ export class PostgresPlugin extends BasePlugin { } else if (typeof args[0] === 'object') { span = utils.handleConfigQuery.call(this, plugin._tracer, ...args); } else { - return original.apply(this, args as never); + return utils.handleInvalidQuery.call( + this, + plugin._tracer, + original, + ...args + ); } // Bind callback to parent span @@ -102,7 +106,6 @@ export class PostgresPlugin extends BasePlugin { args[args.length - 1] ); } - callbackProvided = true; } else if ( typeof (args[0] as PgPluginQueryConfig).callback === 'function' ) { @@ -119,7 +122,6 @@ export class PostgresPlugin extends BasePlugin { // Copy the callback instead of writing to args.callback so that we don't modify user's // original callback reference args[0] = { ...(args[0] as object), callback }; - callbackProvided = true; } } @@ -139,22 +141,16 @@ export class PostgresPlugin extends BasePlugin { }) .catch((error: Error) => { return new Promise((_, reject) => { - span.setStatus({ code: CanonicalCode.UNKNOWN }); + span.setStatus({ + code: CanonicalCode.UNKNOWN, + message: error.message, + }); span.end(); reject(error); }); }); } - // If a promise was not returned and no callback is provided, we recieved invalid args - if (!callbackProvided) { - span.setStatus({ - code: CanonicalCode.INVALID_ARGUMENT, - message: 'Invalid query provided to the driver', - }); - span.end(); - } - // else returns void return result; // void }; diff --git a/packages/opentelemetry-plugin-postgres/src/utils.ts b/packages/opentelemetry-plugin-postgres/src/utils.ts index 062236848a..5e74204b3b 100644 --- a/packages/opentelemetry-plugin-postgres/src/utils.ts +++ b/packages/opentelemetry-plugin-postgres/src/utils.ts @@ -25,7 +25,7 @@ import { import * as pgTypes from 'pg'; import { PostgresPlugin } from './pg'; -function arrayStringifyHelper(arr: Array): string { +function arrayStringifyHelper(arr: Array): string { return '[' + arr.toString() + ']'; } @@ -79,7 +79,10 @@ export function handleConfigQuery( const span = pgStartSpan(tracer, this, name); // Set attributes - span.setAttribute(AttributeNames.DB_STATEMENT, argsConfig.text); + if (argsConfig.text) { + span.setAttribute(AttributeNames.DB_STATEMENT, argsConfig.text); + } + if (argsConfig.values instanceof Array) { span.setAttribute( AttributeNames.PG_VALUES, @@ -131,11 +134,34 @@ export function handleTextQuery( return span; } +/** + * Invalid query handler. We should never enter this function unless invalid args were passed to the driver. + * Create and immediately end a new span + */ +export function handleInvalidQuery( + this: pgTypes.Client & PgClientExtended, + tracer: Tracer, + originalQuery: typeof pgTypes.Client.prototype.query, + ...args: unknown[] +) { + let result; + const span = pgStartSpan(tracer, this, PostgresPlugin.BASE_SPAN_NAME); + try { + result = originalQuery.apply(this, args as never); + span.setStatus({ code: CanonicalCode.OK }); // this will never happen, but set a status anyways + } catch (e) { + span.setStatus({ code: CanonicalCode.UNKNOWN, message: e.message }); + throw e; + } finally { + span.end(); + } + return result; +} + export function patchCallback( span: Span, cb: PostgresCallback ): PostgresCallback { - const originalCb = cb; return function patchedCallback( this: pgTypes.Client & PgClientExtended, err: Error, @@ -150,6 +176,6 @@ export function patchCallback( span.setStatus({ code: CanonicalCode.OK }); } span.end(); - return originalCb.call(this, err, res); + cb.call(this, err, res); }; } diff --git a/packages/opentelemetry-plugin-postgres/test/assertionUtils.ts b/packages/opentelemetry-plugin-postgres/test/assertionUtils.ts index 7daed9c11d..2c81918305 100644 --- a/packages/opentelemetry-plugin-postgres/test/assertionUtils.ts +++ b/packages/opentelemetry-plugin-postgres/test/assertionUtils.ts @@ -19,7 +19,7 @@ import { Attributes, Event, Span, - TimedEvent, + Status, } from '@opentelemetry/types'; import * as assert from 'assert'; import { PostgresPlugin } from '../src'; @@ -34,7 +34,8 @@ export const assertSpan = ( span: ReadableSpan, kind: SpanKind, attributes: Attributes, - events: Event[] + events: Event[], + status: Status ) => { assert.strictEqual(span.spanContext.traceId.length, 32); assert.strictEqual(span.spanContext.spanId.length, 16); @@ -53,24 +54,15 @@ export const assertSpan = ( assert.ok(hrTimeToMilliseconds(span.endTime) > 0); // attributes - assert.strictEqual( - Object.keys(span.attributes).length, - Object.keys(attributes).length, - 'Should contain same number of attributes' - ); - Object.keys(span.attributes).forEach(attribute => { - assert.deepStrictEqual(span.attributes[attribute], attributes[attribute]); - }); + assert.deepStrictEqual(span.attributes, attributes); // events - assert.strictEqual( - span.events.length, - events.length, - 'Should contain same number of events' - ); - span.events.forEach((_: TimedEvent, index: number) => { - assert.deepStrictEqual(span.events[index], events[index]); - }); + assert.deepStrictEqual(span.events, events); + + assert.strictEqual(span.status.code, status.code); + if (status.message) { + assert.strictEqual(span.status.message, status.message); + } }; // Check if sourceSpan was propagated to targetSpan diff --git a/packages/opentelemetry-plugin-postgres/test/pg.test.ts b/packages/opentelemetry-plugin-postgres/test/pg.test.ts index 3869ca61fa..1bfd273ebe 100644 --- a/packages/opentelemetry-plugin-postgres/test/pg.test.ts +++ b/packages/opentelemetry-plugin-postgres/test/pg.test.ts @@ -20,7 +20,14 @@ import { InMemorySpanExporter, SimpleSpanProcessor, } from '@opentelemetry/tracing'; -import { SpanKind, Attributes, TimedEvent, Span } from '@opentelemetry/types'; +import { + SpanKind, + Attributes, + TimedEvent, + Span, + CanonicalCode, + Status, +} from '@opentelemetry/types'; import { plugin, PostgresPlugin } from '../src'; import { AttributeNames } from '../src/enums'; import * as assert from 'assert'; @@ -49,18 +56,34 @@ const DEFAULT_ATTRIBUTES = { [AttributeNames.DB_USER]: CONFIG.user, }; +const okStatus: Status = { + code: CanonicalCode.OK, +}; +const unknownStatus: Status = { + code: CanonicalCode.UNKNOWN, +}; + const runCallbackTest = ( - span: Span, + span: Span | null, attributes: Attributes, events: TimedEvent[], + status: Status = okStatus, spansLength = 1, spansIndex = 0 ) => { const spans = memoryExporter.getFinishedSpans(); assert.strictEqual(spans.length, spansLength); const pgSpan = spans[spansIndex]; - assertionUtils.assertSpan(pgSpan, SpanKind.CLIENT, attributes, events); - assertionUtils.assertPropagation(pgSpan, span); + assertionUtils.assertSpan( + pgSpan, + SpanKind.CLIENT, + attributes, + events, + status + ); + if (span) { + assertionUtils.assertPropagation(pgSpan, span); + } }; describe('pg@7.x', () => { @@ -69,7 +92,7 @@ describe('pg@7.x', () => { const logger = new NoopLogger(); const testPostgres = process.env.RUN_POSTGRES_TESTS; // For CI: assumes local postgres db is already available const testPostgresLocally = process.env.RUN_POSTGRES_TESTS_LOCAL; // For local: spins up local postgres db via docker - const shouldTest = testPostgres || testPostgresLocally; // Skips these tests if false (default) + const shouldTest = true || testPostgres || testPostgresLocally; // Skips these tests if false (default) before(async function() { if (!shouldTest) { @@ -128,8 +151,21 @@ describe('pg@7.x', () => { assertPgError, 'pg should throw when no args provided' ); + runCallbackTest(null, DEFAULT_ATTRIBUTES, [], unknownStatus); + memoryExporter.reset(); + assert.doesNotThrow( - () => (client as any).query({ foo: 'bar' }, undefined, () => null), + () => + (client as any).query({ foo: 'bar' }, undefined, () => { + runCallbackTest( + null, + { + ...DEFAULT_ATTRIBUTES, + }, + [], + unknownStatus + ); + }), 'pg should not throw when invalid config args are provided' ); }); From 02797844eeef774165c8288b2463bd969676be7c Mon Sep 17 00:00:00 2001 From: Mark Wolff Date: Tue, 29 Oct 2019 09:45:04 -0700 Subject: [PATCH 18/18] chore: remove hardcode shouldTest --- packages/opentelemetry-plugin-postgres/test/pg.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/opentelemetry-plugin-postgres/test/pg.test.ts b/packages/opentelemetry-plugin-postgres/test/pg.test.ts index 1bfd273ebe..d30ad3a325 100644 --- a/packages/opentelemetry-plugin-postgres/test/pg.test.ts +++ b/packages/opentelemetry-plugin-postgres/test/pg.test.ts @@ -92,7 +92,7 @@ describe('pg@7.x', () => { const logger = new NoopLogger(); const testPostgres = process.env.RUN_POSTGRES_TESTS; // For CI: assumes local postgres db is already available const testPostgresLocally = process.env.RUN_POSTGRES_TESTS_LOCAL; // For local: spins up local postgres db via docker - const shouldTest = true || testPostgres || testPostgresLocally; // Skips these tests if false (default) + const shouldTest = testPostgres || testPostgresLocally; // Skips these tests if false (default) before(async function() { if (!shouldTest) {