feature(plugin): implement postgres plugin #417

markwolff · 2019-10-09T21:33:02Z

Which problem is this PR solving?

Closes [Plugin]: Add postgres plugin #404

Short description of the changes

Adds client.query(...) support. Postgres support will not be complete at this PR. There are also separate packages, pg-pool and pg-cursor, which are separate packages and would require separate plugins as well.

PR TODO

Determine what span.status should be set to.
End spans on queries with callbacks
Test client.query({ plan }).
Add additional attributes where appropriate

New TODOs

pg-pool plugin (docs: https://node-postgres.com/api/pool)
pg-cursor plugin (docs: https://node-postgres.com/api/cursor)

draffensperger · 2019-10-10T14:04:42Z

packages/opentelemetry-plugin-postgres/src/enums.ts

+ */
+
+export enum AttributeNames {
+  COMPONENT = 'component',


Is this a standard label in the spec?

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

draffensperger · 2019-10-10T14:05:24Z

packages/opentelemetry-plugin-postgres/src/pg.ts

+import * as shimmer from 'shimmer';
+
+export class PostgresPlugin extends BasePlugin<typeof pgTypes> {
+  static readonly component = 'pg';


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

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

draffensperger · 2019-10-10T14:07:42Z

packages/opentelemetry-plugin-postgres/src/pg.ts

+      plugin._logger.debug(
+        `Patching ${PostgresPlugin.component}.Client.prototype.query`
+      );
+      return function query(this: pgTypes.Client, ...args: unknown[]) {


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

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

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?

great change thanks

draffensperger · 2019-10-10T14:09:09Z

packages/opentelemetry-plugin-postgres/test/assertionUtils.ts

+} from '@opentelemetry/core';
+import { AttributeNames } from '../src/enums';
+
+export const assertSpan = (


Would this function be useful to other plugins?

draffensperger · 2019-10-10T14:09:40Z

packages/opentelemetry-plugin-postgres/test/pg.test.ts

+  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) {


Nit: should this be an arrow function (ready) => {?

Because I'm using this as Mocha.Context in the before fn to skip the tests, its easier to use a function. Is there a nicer way to feed mocha my intended this with an arrow function?

draffensperger · 2019-10-10T14:10:47Z

So cool to see this happening!

packages/opentelemetry-plugin-postgres/package.json

dyladan · 2019-10-10T16:14:26Z

packages/opentelemetry-plugin-postgres/src/enums.ts

+ */
+
+export enum AttributeNames {
+  COMPONENT = 'component',


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

dyladan · 2019-10-10T16:47:40Z

packages/opentelemetry-plugin-postgres/src/pg.ts

+      plugin._logger.debug(
+        `Patching ${PostgresPlugin.component}.Client.prototype.query`
+      );
+      return function query(this: pgTypes.Client, ...args: unknown[]) {


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

mayurkale22

This is great work! Added a few comments.

mayurkale22 · 2019-10-10T18:28:16Z

.circleci/config.yml

@@ -1,5 +1,11 @@
 version: 2

+test_env: &test_env
+  TEST_POSTGRES: 1


This can be more descriptive, something like RUN_POSTGRES_TESTS WDYT?

packages/opentelemetry-plugin-postgres/src/pg.ts

mayurkale22 · 2019-10-10T18:59:21Z

packages/opentelemetry-plugin-postgres/src/pg.ts

+            attributes: {
+              [AttributeNames.COMPONENT]: PostgresPlugin.component,
+              [AttributeNames.PG_HOST]: (this as any).connectionParameters.host,
+              [AttributeNames.PG_PORT]: (this as any).connectionParameters.port,


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, }

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

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

mayurkale22 · 2019-10-10T19:05:55Z

packages/opentelemetry-plugin-postgres/src/pg.ts

+            }
+          }
+        } catch (e) {
+          plugin._logger.warn(


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.

mayurkale22 · 2019-10-10T19:12:34Z

packages/opentelemetry-plugin-postgres/src/pg.ts

+                span.setAttribute(AttributeNames.PG_TEXT, config.text);
+              }
+              if (config.values instanceof Array) {
+                span.setAttribute(AttributeNames.PG_VALUES, config.values);


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.

packages/opentelemetry-plugin-postgres/test/pg.test.ts

OlivierAlbertini · 2019-10-10T21:47:08Z

Nice work!

packages/opentelemetry-plugin-postgres/src/pg.ts

OlivierAlbertini · 2019-10-12T11:38:28Z

packages/opentelemetry-plugin-postgres/package.json

@@ -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",


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

packages/opentelemetry-plugin-postgres/src/pg.ts

draffensperger · 2019-10-18T19:14:25Z

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",


Why use the earlier version of gts?

AFAIK most of the places we are using 1.0.0, (I think) this change is to make sure we are consistent everywhere. We can update the latest version (1.1.0) in separate PR.

packages/opentelemetry-plugin-postgres/src/pg.ts

packages/opentelemetry-plugin-postgres/src/utils.ts

draffensperger · 2019-10-18T19:16:25Z

packages/opentelemetry-plugin-postgres/test/pg.test.ts

-      });
-      assert.strictEqual(res, undefined, 'No promise is returned');
-    });
+    // it('should not return a promise if callback is provided', done => {


Why is this code being commented out?

mayurkale22 · 2019-10-18T20:35:39Z

packages/opentelemetry-plugin-postgres/package.json

@@ -57,6 +61,8 @@
  "dependencies": {
    "@opentelemetry/core": "^0.1.0",
    "@opentelemetry/node": "^0.1.0",
-    "@opentelemetry/types": "^0.1.0"
+    "@opentelemetry/tracing": "^0.1.0",


Suggested change

"@opentelemetry/tracing": "^0.1.0",

"@opentelemetry/tracing": "^0.1.1",

packages/opentelemetry-plugin-postgres/src/utils.ts

mayurkale22 · 2019-10-18T20:52:35Z

packages/opentelemetry-plugin-postgres/src/utils.ts

+
+  // 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);


It is highly discouraged to update the name of a Span after its creation.

Based on specs:

The method name is called UpdateName to differentiate this method from the regular property setter. It emphasizes that this operation signifies a major change for a Span and may lead to re-calculation of sampling or filtering decisions made previously depending on the implementation. Alternatives for the name update may be late Span creation, when Span is started with the explicit timestamp from the past at the moment where the final Span name is known, or reporting a Span with the desired name as a child Span.

Is it possible to avoid this? Do we need to update the name with query command?

It can be avoided. The spec calls for DB spans to be named with a low cardinality name describing the query, e.g. insert or select. I've moved the span creation to the query handlers so that I can add this information to the span on creation. bfb19d3

Nice! Thanks for doing this 👍

…elemetry-js into markwolff/impl-pg-plugin

mayurkale22 · 2019-10-23T19:07:27Z

@open-telemetry/node-approvers and @open-telemetry/node-maintainers Please review when you get a chance.

dyladan

minor nits but otherwise LGTM

dyladan · 2019-10-28T18:07:50Z

packages/opentelemetry-plugin-postgres/src/utils.ts

+import * as pgTypes from 'pg';
+import { PostgresPlugin } from './pg';
+
+function arrayStringifyHelper<T>(arr: Array<T>): string {


nit: Does this need to be generic?

Suggested change

function arrayStringifyHelper<T>(arr: Array<T>): string {

function arrayStringifyHelper(arr: Array<unknown>): string {

dyladan · 2019-10-28T18:12:00Z

packages/opentelemetry-plugin-postgres/src/utils.ts

+  span: Span,
+  cb: PostgresCallback
+): PostgresCallback {
+  const originalCb = cb;


Why this additional variable? Can this line not be removed and on line 153 just cb.call(...?

dyladan · 2019-10-28T18:12:44Z

packages/opentelemetry-plugin-postgres/src/utils.ts

+      span.setStatus({ code: CanonicalCode.OK });
+    }
+    span.end();
+    return originalCb.call(this, err, res);


Is there a reason to rename cb?

Suggested change

return originalCb.call(this, err, res);

return cb.call(this, err, res);

Nice catch, looks like dead code. Addressed this and your previous comments

OlivierAlbertini

Good work! Added few comments.

OlivierAlbertini · 2019-10-28T20:45:16Z

packages/opentelemetry-plugin-postgres/src/pg.ts

+            })
+            .catch((error: Error) => {
+              return new Promise((_, reject) => {
+                span.setStatus({ code: CanonicalCode.UNKNOWN });


Should we set something like span.setStatus({ code: CanonicalCode.UNKNOWN, message: error.message });

OlivierAlbertini · 2019-10-28T20:49:03Z

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`


supported version is currently ^7.12.1 which is different. Please fix here or supportedVersions field

👍 changed supportedVersions to 7.*

OlivierAlbertini · 2019-10-28T20:56:18Z

packages/opentelemetry-plugin-postgres/test/pg.test.ts

+    );
+  });
+
+  describe('#client.query(...)', () => {


Is it possible to add some tests arounds span.status in order to make sure that status is set to Unknown, INVALID_ARGUMENT etc... ?

Good call. I added some span status tests on each call to assertSpan. I also removed my invalid argument statuses since it would be included in the error message anyways

OlivierAlbertini · 2019-10-29T20:12:34Z

Do you know why codecov comment doesn't appear ?

mayurkale22 · 2019-10-30T19:17:54Z

Do you know why codecov comment doesn't appear ?

Strange, looks like this happens once in a while (same with #466). But this is un-related to this PR, will merge it now.

Maybe we should open an issue to address it separately.

Bumps [eslint](https://github.com/eslint/eslint) from 3.19.0 to 4.18.2. - [Release notes](https://github.com/eslint/eslint/releases) - [Changelog](https://github.com/eslint/eslint/blob/master/CHANGELOG.md) - [Commits](eslint/eslint@v3.19.0...v4.18.2) Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: Valentin Marchaud <[email protected]>

Mark Wolff added 4 commits October 9, 2019 13:35

feat(pg): implement postgres plugin

0212dea

fix: linting

62ecb39

fix: docker starting not locally

f8d885b

Merge branch 'master' into markwolff/impl-pg-plugin

3ed0cf3

markwolff requested review from bg451, danielkhan, iredelmeier, mayurkale22, OlivierAlbertini, rochdev and vmarchaud as code owners October 9, 2019 21:33

markwolff changed the title ~~Markwolff/impl pg plugin~~ Feature(postgres): Implement Postgres plugin Oct 9, 2019

markwolff changed the title ~~Feature(postgres): Implement Postgres plugin~~ feature(plugin): implement postgres plugin Oct 9, 2019

Mark Wolff added 2 commits October 9, 2019 14:48

fix: compile errors from merge

9b90fde

fix: linting

c14b33f

markwolff mentioned this pull request Oct 9, 2019

Request(plugin-postgres): pg-pool #418

Closed

draffensperger reviewed Oct 10, 2019

View reviewed changes

dyladan reviewed Oct 10, 2019

View reviewed changes

mayurkale22 reviewed Oct 10, 2019

View reviewed changes

OlivierAlbertini reviewed Oct 12, 2019

View reviewed changes

mayurkale22 mentioned this pull request Oct 14, 2019

feat: add mongodb plugin #205

Merged

Mark Wolff added 2 commits October 15, 2019 16:38

refactor: use helper functions for span building

dd1a7d3

fix: add callback patching to end span

7a854fe

vmarchaud reviewed Oct 17, 2019

View reviewed changes

packages/opentelemetry-plugin-postgres/src/pg.ts Outdated Show resolved Hide resolved

Mark Wolff added 2 commits October 18, 2019 11:37

fix: add required attributes, address comments

eafdb3d

fix: lint errors

7504ba7

draffensperger reviewed Oct 18, 2019

View reviewed changes

Merge branch 'master' into markwolff/impl-pg-plugin

9316ff5

mayurkale22 reviewed Oct 18, 2019

View reviewed changes

OlivierAlbertini added the feature-request label Oct 19, 2019

Mark Wolff added 2 commits October 21, 2019 12:11

refactor: start named spans in query handlers

bfb19d3

fix: linting errors

64a2a8b

mayurkale22 approved these changes Oct 21, 2019

View reviewed changes

Mark Wolff and others added 8 commits October 21, 2019 12:25

Merge branch 'master' into markwolff/impl-pg-plugin

10fd6ca

fix: circleci config, make pg helpers nonexported

d81e4b0

Merge branch 'markwolff/impl-pg-plugin' of github.com:markwolff/opent…

297e699

…elemetry-js into markwolff/impl-pg-plugin

fix: linting

208d3a9

docs: add supported versions

f986476

fix: pass PG env to spawned container

644f5f1

fix: remove hardcoded shouldTest

b3a83b4

Merge branch 'master' into markwolff/impl-pg-plugin

ccefe01

Merge branch 'master' into markwolff/impl-pg-plugin

fefb04e

dyladan approved these changes Oct 28, 2019

View reviewed changes

OlivierAlbertini reviewed Oct 28, 2019

View reviewed changes

test: add span tests for pg driver errors

4e24474

vmarchaud approved these changes Oct 29, 2019

View reviewed changes

OlivierAlbertini approved these changes Oct 29, 2019

View reviewed changes

markwolff and others added 2 commits October 29, 2019 09:45

chore: remove hardcode shouldTest

0279784

Merge branch 'master' into markwolff/impl-pg-plugin

bbdc02d

mayurkale22 merged commit 5c49c6c into open-telemetry:master Oct 30, 2019

mayurkale22 mentioned this pull request Oct 30, 2019

MIssing Codecov report #467

Closed

pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023

chore: fix bugs in release workflow (open-telemetry#417)

73b9c6b

Co-authored-by: Valentin Marchaud <[email protected]>

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

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

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

	function arrayStringifyHelper<T>(arr: Array<T>): string {
	function arrayStringifyHelper(arr: Array<unknown>): string {

	return originalCb.call(this, err, res);
	return cb.call(this, err, res);

feature(plugin): implement postgres plugin #417

feature(plugin): implement postgres plugin #417

Conversation

markwolff commented Oct 9, 2019 • edited Loading

Which problem is this PR solving?

Short description of the changes

PR TODO

New TODOs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

draffensperger commented Oct 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayurkale22 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markwolff Oct 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OlivierAlbertini commented Oct 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayurkale22 commented Oct 23, 2019

dyladan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OlivierAlbertini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OlivierAlbertini Oct 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OlivierAlbertini commented Oct 29, 2019

mayurkale22 commented Oct 30, 2019

markwolff commented Oct 9, 2019 •

edited

Loading

markwolff Oct 15, 2019 •

edited

Loading

OlivierAlbertini Oct 28, 2019 •

edited

Loading