Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature(plugin): implement postgres plugin #417

Merged
merged 25 commits into from
Oct 30, 2019
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
0212dea
feat(pg): implement postgres plugin
Oct 9, 2019
62ecb39
fix: linting
Oct 9, 2019
f8d885b
fix: docker starting not locally
Oct 9, 2019
3ed0cf3
Merge branch 'master' into markwolff/impl-pg-plugin
Oct 9, 2019
9b90fde
fix: compile errors from merge
Oct 9, 2019
c14b33f
fix: linting
Oct 9, 2019
dd1a7d3
refactor: use helper functions for span building
Oct 15, 2019
7a854fe
fix: add callback patching to end span
Oct 16, 2019
eafdb3d
fix: add required attributes, address comments
Oct 18, 2019
7504ba7
fix: lint errors
Oct 18, 2019
9316ff5
Merge branch 'master' into markwolff/impl-pg-plugin
Oct 18, 2019
bfb19d3
refactor: start named spans in query handlers
Oct 21, 2019
64a2a8b
fix: linting errors
Oct 21, 2019
10fd6ca
Merge branch 'master' into markwolff/impl-pg-plugin
Oct 21, 2019
d81e4b0
fix: circleci config, make pg helpers nonexported
Oct 21, 2019
297e699
Merge branch 'markwolff/impl-pg-plugin' of github.com:markwolff/opent…
Oct 21, 2019
208d3a9
fix: linting
Oct 21, 2019
f986476
docs: add supported versions
Oct 21, 2019
644f5f1
fix: pass PG env to spawned container
Oct 21, 2019
b3a83b4
fix: remove hardcoded shouldTest
markwolff Oct 23, 2019
ccefe01
Merge branch 'master' into markwolff/impl-pg-plugin
mayurkale22 Oct 23, 2019
fefb04e
Merge branch 'master' into markwolff/impl-pg-plugin
mayurkale22 Oct 28, 2019
4e24474
test: add span tests for pg driver errors
markwolff Oct 28, 2019
0279784
chore: remove hardcode shouldTest
markwolff Oct 29, 2019
bbdc02d
Merge branch 'master' into markwolff/impl-pg-plugin
mayurkale22 Oct 29, 2019
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions .circleci/config.yml
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
version: 2

test_env: &test_env
TEST_POSTGRES: 1
Copy link
Member

Choose a reason for hiding this comment

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

This can be more descriptive, something like RUN_POSTGRES_TESTS WDYT?


postgres_service: &postgres_service
image: postgres:alpine

node_unit_tests: &node_unit_tests
steps:
- checkout
Expand Down Expand Up @@ -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:
Expand Down
10 changes: 8 additions & 2 deletions packages/opentelemetry-plugin-postgres/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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'",
markwolff marked this conversation as resolved.
Show resolved Hide resolved
"tdd": "yarn test -- --watch-extensions ts --watch",
"clean": "rimraf build/*",
"check": "gts check",
Expand Down Expand Up @@ -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",
Copy link
Member

Choose a reason for hiding this comment

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

Please use gts version ^1.0.0 in order to be consistent with other packages dependencies.

"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",
Expand All @@ -56,6 +60,8 @@
"dependencies": {
"@opentelemetry/core": "^0.1.0",
"@opentelemetry/node": "^0.1.0",
"@opentelemetry/types": "^0.1.0"
"@opentelemetry/tracing": "^0.1.0",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"@opentelemetry/tracing": "^0.1.0",
"@opentelemetry/tracing": "^0.1.1",

"@opentelemetry/types": "^0.1.0",
"shimmer": "^1.2.1"
}
}
24 changes: 24 additions & 0 deletions packages/opentelemetry-plugin-postgres/src/enums.ts
Original file line number Diff line number Diff line change
@@ -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',
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a standard label in the spec?

Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/data-semantic-conventions.md#databases-client-calls

Looks like the spec calls for:

Attribute name Notes and examples Required?
component Database driver name or database name (when known) "JDBI", "jdbc", "odbc", "postgreSQL". Yes
db.type Database type. For any SQL database, "sql". For others, the lower-case database category, e.g. "cassandra", "hbase", or "redis". Yes
db.instance Database instance name. E.g., In java, if the jdbc.url="jdbc:mysql://db.example.com:3306/customers", the instance name is "customers". Yes
db.statement A database statement for the given database type. Note, that the value may be sanitized to exclude sensitive information. E.g., for db.type="sql", "SELECT * FROM wuser_table"; for db.type="redis", "SET mykey 'WuValue'". Yes
db.user Username for accessing database. E.g., "readonly_user" or "reporting_user" No

For database client calls, peer information can be populated and interpreted as
follows:

Attribute name Notes and examples Required
peer.address JDBC substring like "mysql://db.example.com:3306" Yes
peer.hostname Remote hostname. "db.example.com" Yes
peer.ipv4 Remote IPv4 address as a .-separated tuple. E.g., "127.0.0.1" No
peer.ipv6 Remote IPv6 address as a string of colon-separated 4-char hex tuples. E.g., "2001:0db8:85a3:0000:0000:8a2e:0370:7334" No
peer.port Remote port. E.g., 80 (integer) No
peer.service Remote service name. Can be database friendly name or "db.instance" No

PG_HOST = 'pg.host',
PG_PORT = 'pg.port',
PG_TEXT = 'pg.text',
PG_VALUES = 'pg.values',
PG_PLAN = 'pg.plan',
}
2 changes: 2 additions & 0 deletions packages/opentelemetry-plugin-postgres/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,5 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/

export * from './pg';
160 changes: 160 additions & 0 deletions packages/opentelemetry-plugin-postgres/src/pg.ts
Original file line number Diff line number Diff line change
@@ -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<typeof pgTypes> {
static readonly component = 'pg';
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be COMPONENT since it's immutable? Similar for SUPPORTED_VERIONS?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated to COMPONENT. SUPPORTED_VERSIONS is specified by BasePlugin interface, so it should be changed there too. Maybe in a separate PR?

readonly supportedVersions = ['^7.12.1'];
markwolff marked this conversation as resolved.
Show resolved Hide resolved
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[]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function feels pretty complex. Could the different parts be broken up into helper functions?

Copy link
Member

Choose a reason for hiding this comment

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

Agree. Would rather see this split into: _textQuery _parameterizedQuery and _configQuery

Copy link
Member Author

Choose a reason for hiding this comment

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

Made a big refactor in dd1a7d3 to use these helper functions. They are outside of the class and add attributes onto the span based on the query args (and update the span name). WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

great change thanks

// 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,
Copy link
Member

Choose a reason for hiding this comment

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

I think we should include pg.instance and pg.user. https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/data-semantic-conventions.md#databases-client-calls

const params = (this as any).connectionParameters;
attributes: {
   [AttributeNames.COMPONENT]: PostgresPlugin.component,
   [AttributeNames.PG_HOST]: params.host,
   [AttributeNames.PG_PORT]: params.port,
   [AttributeNames.PG_INSTANCE]: params.database,
   [AttributeNames.PG_USER]: params.user,
}

Copy link
Member

Choose a reason for hiding this comment

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

Also, I think PG_TEXT equivalent to db.statement/pg.statement? If yes, can we use pg.statement as per the specs.

Copy link
Member Author

@markwolff markwolff Oct 15, 2019

Choose a reason for hiding this comment

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

Updated attribute name to pg.statement. Still TODO: other attributes called for by spec

},
}
);

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);
Copy link
Member

Choose a reason for hiding this comment

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

Although setAttribute accepts unknown as a value, do you think we should flattern the array? Otherwise each exporterd needs to implement some logic to handle this.

}
}

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(
Copy link
Member

Choose a reason for hiding this comment

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

Does it makes sense to open an issue on https://github.com/open-telemetry/opentelemetry-specification, about what statues to set in case of databases. Looks like most the status codes are applicable to HTTP and GRPC requests.

`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 [email protected])
if (!callbackProvided) {
const queryResultPromise = (queryResult as unknown) as Promise<
unknown
>;
return plugin._tracer.bind(
markwolff marked this conversation as resolved.
Show resolved Hide resolved
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;
markwolff marked this conversation as resolved.
Show resolved Hide resolved
export const plugin = new PostgresPlugin(PostgresPlugin.component, version);
19 changes: 19 additions & 0 deletions packages/opentelemetry-plugin-postgres/src/types.ts
Original file line number Diff line number Diff line change
@@ -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;
90 changes: 90 additions & 0 deletions packages/opentelemetry-plugin-postgres/test/assertionUtils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
/*!
* 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,
TimedEvent,
} from '@opentelemetry/types';
import * as assert from 'assert';
import { PostgresPlugin } from '../src';
import { ReadableSpan } from '@opentelemetry/tracing';
import {
hrTimeToMilliseconds,
hrTimeToMicroseconds,
} from '@opentelemetry/core';
import { AttributeNames } from '../src/enums';

export const assertSpan = (
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this function be useful to other plugins?

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((_: TimedEvent, index: number) => {
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);
};
Loading