From 07d6c4db6ddb066445b998b42919b20268c95d6f Mon Sep 17 00:00:00 2001 From: Ethan Resnick Date: Fri, 25 Nov 2022 19:37:01 -0800 Subject: [PATCH 1/4] refactor(pg): more defensive, DRYer implementation The prior code had three different utility functions for constructing the span to use to trace the query: handleTextQuery, handleConfigQuery, and handleParameterizedQuery. These functions contained a lot of duplicated logic, and that duplication was gonna make it harder to introduce new features (see next commit) and was already leading to bugs/inconsistencies. For example, `handleConfigQuery` would verify that the parameter `values` provided with the query were provided as an array before attaching them to the span, whereas `handleParameterizedQuery` would do so without this validation. This commit instead normalizes the arguments to `client.query` first, so that only function is needed to create the span. In the process of normalizing the arguments, the new code also performs a bit more validation on them, to make sure that the instrumentation will not throw at runtime. For example, the prior code would've thrown on `client.query(null)`, as it was checking `typeof args[0] === 'object'` without excluding null. The prior code also assumed, if an object was passed, that the `text` and `name` keys held a string; this is now verified before attaching that data to the span. Finally, the prior code had two different code paths for invoking the original `client.query` method; one that went through `handleInvalidQuery` and one directly in `_getClientQueryPatch`. The former wrapped the call in a try-catch, while the latter did not. Now, there's only one call to the original client.query, and it's always in a try-catch, which ensures that other cases of invalid args passed to client.query (which might lead it to throw synchronously) won't be an issue. --- .../src/instrumentation.ts | 136 +++++++++--------- .../src/internal-types.ts | 6 - .../src/utils.ts | 121 ++++++---------- .../test/utils.test.ts | 45 +----- 4 files changed, 117 insertions(+), 191 deletions(-) diff --git a/plugins/node/opentelemetry-instrumentation-pg/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-pg/src/instrumentation.ts index f309555951..9a946cc5f7 100644 --- a/plugins/node/opentelemetry-instrumentation-pg/src/instrumentation.ts +++ b/plugins/node/opentelemetry-instrumentation-pg/src/instrumentation.ts @@ -26,7 +26,6 @@ import { PgClientConnect, PgClientExtended, PgErrorCallback, - NormalizedQueryConfig, PostgresCallback, PgPoolExtended, PgPoolCallback, @@ -173,72 +172,70 @@ export class PgInstrumentation extends InstrumentationBase { `Patching ${PgInstrumentation.COMPONENT}.Client.prototype.query` ); return function query(this: PgClientExtended, ...args: unknown[]) { - let span: Span; + // client.query(text, cb?), client.query(text, values, cb?), and + // client.query(configObj, cb?) are all valid signatures. We construct + // a queryConfig obj from all (valid) signatures to build the span in a + // unified way. We verify that we at least have query text, and code + // defensively when dealing with `queryConfig` after that (to handle all + // the other invalid cases, like a non-array for values being provided). + // The type casts here reflect only what we've actually validated. + const arg0 = args[0]; + const firstArgIsString = typeof arg0 === 'string'; + const firstArgIsQueryObjectWithText = + utils.isObjectWithTextString(arg0); + + // TODO: remove the `as ...` casts below when the TS version is upgraded. + // Newer TS versions will use the result of firstArgIsQueryObjectWithText + // to properly narrow arg0, but TS 4.3.5 does not. + const queryConfig = firstArgIsString + ? { + text: arg0 as string, + values: Array.isArray(args[1]) ? args[1] : undefined, + } + : firstArgIsQueryObjectWithText + ? (arg0 as utils.ObjectWithText) + : undefined; - // Handle different client.query(...) signatures - if (typeof args[0] === 'string') { - const query = args[0]; + const instrumentationConfig = plugin.getConfig(); - if (args.length > 1 && args[1] instanceof Array) { - const params = args[1]; - span = utils.handleParameterizedQuery.call( - this, - plugin.tracer, - plugin.getConfig(), - query, - params - ); - } else { - span = utils.handleTextQuery.call( - this, - plugin.tracer, - plugin.getConfig(), - query - ); - } + const span = utils.handleConfigQuery.call( + this, + plugin.tracer, + instrumentationConfig, + queryConfig + ); - if (plugin.getConfig().addSqlCommenterCommentToQueries) { - // Modify the query with a tracing comment - args[0] = utils.addSqlCommenterComment(span, args[0]); - } - } else if (typeof args[0] === 'object') { - const queryConfig = args[0] as NormalizedQueryConfig; - - span = utils.handleConfigQuery.call( - this, - plugin.tracer, - plugin.getConfig(), - queryConfig - ); - - if (plugin.getConfig().addSqlCommenterCommentToQueries) { - // Copy the query config instead of writing to args[0].text so that we don't modify user's - // original query config reference - args[0] = { - ...queryConfig, - text: utils.addSqlCommenterComment(span, queryConfig.text), - }; - } - } else { - return utils.handleInvalidQuery.call( - this, - plugin.tracer, - plugin.getConfig(), - original, - ...args - ); + // Modify query text w/ a tracing comment before invoking original for + // tracing, but only if args[0] has one of our expected shapes. + // + // TODO: remove the `as ...` casts below when the TS version is upgraded. + // Newer TS versions will use the result of firstArgIsQueryObjectWithText + // to properly narrow arg0, but TS 4.3.5 does not. + if (instrumentationConfig.addSqlCommenterCommentToQueries) { + args[0] = firstArgIsString + ? utils.addSqlCommenterComment(span, arg0 as string) + : firstArgIsQueryObjectWithText + ? { + ...(arg0 as utils.ObjectWithText), + text: utils.addSqlCommenterComment( + span, + (arg0 as utils.ObjectWithText).text + ), + } + : args[0]; } - // Bind callback to parent span + // Bind callback (if any) to parent span (if any) if (args.length > 0) { const parentSpan = trace.getSpan(context.active()); if (typeof args[args.length - 1] === 'function') { // Patch ParameterQuery callback args[args.length - 1] = utils.patchCallback( - plugin.getConfig(), + instrumentationConfig, span, - args[args.length - 1] as PostgresCallback + args[args.length - 1] as PostgresCallback // nb: not type safe. ); + // If a parent span exists, bind the callback if (parentSpan) { args[args.length - 1] = context.bind( @@ -246,28 +243,37 @@ export class PgInstrumentation extends InstrumentationBase { args[args.length - 1] ); } - } else if ( - typeof (args[0] as NormalizedQueryConfig).callback === 'function' - ) { + } else if (typeof queryConfig?.callback === 'function') { // Patch ConfigQuery callback let callback = utils.patchCallback( plugin.getConfig(), span, - (args[0] as NormalizedQueryConfig).callback! + queryConfig.callback as PostgresCallback // nb: not type safe. ); + // If a parent span existed, bind the callback if (parentSpan) { callback = context.bind(context.active(), callback); } - // Copy the callback instead of writing to args.callback so that we don't modify user's - // original callback reference + // 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 }; } } - // Perform the original query - const result: unknown = original.apply(this, args as any); + let result: unknown; + try { + result = original.apply(this, args as never); + } catch (e: unknown) { + // span.recordException(e); + span.setStatus({ + code: SpanStatusCode.ERROR, + message: utils.getErrorMessage(e), + }); + span.end(); + throw e; + } // Bind promise to parent span and end the span if (result instanceof Promise) { @@ -359,10 +365,10 @@ function handleConnectResult(span: Span, connectResult: unknown) { span.end(); return result; }) - .catch((error: Error) => { + .catch((error: unknown) => { span.setStatus({ code: SpanStatusCode.ERROR, - message: error.message, + message: utils.getErrorMessage(error), }); span.end(); return Promise.reject(error); diff --git a/plugins/node/opentelemetry-instrumentation-pg/src/internal-types.ts b/plugins/node/opentelemetry-instrumentation-pg/src/internal-types.ts index dc4e54fe7a..60b2377bc9 100644 --- a/plugins/node/opentelemetry-instrumentation-pg/src/internal-types.ts +++ b/plugins/node/opentelemetry-instrumentation-pg/src/internal-types.ts @@ -32,12 +32,6 @@ export interface PgClientExtended extends pgTypes.Client { connectionParameters: PgClientConnectionParams; } -// Interface name based on original driver implementation -// https://github.com/brianc/node-postgres/blob/2ef55503738eb2cbb6326744381a92c0bc0439ab/packages/pg/lib/utils.js#L157 -export interface NormalizedQueryConfig extends pgTypes.QueryConfig { - callback?: PostgresCallback; -} - export type PgPoolCallback = ( err: Error, client: any, diff --git a/plugins/node/opentelemetry-instrumentation-pg/src/utils.ts b/plugins/node/opentelemetry-instrumentation-pg/src/utils.ts index 048c0d7ad0..970aedaf73 100644 --- a/plugins/node/opentelemetry-instrumentation-pg/src/utils.ts +++ b/plugins/node/opentelemetry-instrumentation-pg/src/utils.ts @@ -35,7 +35,6 @@ import { } from '@opentelemetry/semantic-conventions'; import { PgClientExtended, - NormalizedQueryConfig, PostgresCallback, PgClientConnectionParams, PgErrorCallback, @@ -103,109 +102,54 @@ function startQuerySpan( }); } -// Queries where args[0] is a QueryConfig +// Create a span from our normalized queryConfig object, +// or return a basic span if no queryConfig was given/could be created. export function handleConfigQuery( this: PgClientExtended, tracer: Tracer, instrumentationConfig: PgInstrumentationConfig, - queryConfig: NormalizedQueryConfig + queryConfig?: { text: string; values?: unknown; name?: unknown } ) { + if (!queryConfig) { + return startQuerySpan( + this, + tracer, + instrumentationConfig, + PgInstrumentation.BASE_SPAN_NAME + ); + } + // Set child span name - const queryCommand = getCommandFromText(queryConfig.name || queryConfig.text); - const name = PgInstrumentation.BASE_SPAN_NAME + ':' + queryCommand; + const queryCommand = getCommandFromText( + (typeof queryConfig.name === 'string' ? queryConfig.name : undefined) || + queryConfig.text + ); + const name = `${PgInstrumentation.BASE_SPAN_NAME}:${queryCommand}`; const span = startQuerySpan(this, tracer, instrumentationConfig, name); // Set attributes if (queryConfig.text) { span.setAttribute(SemanticAttributes.DB_STATEMENT, queryConfig.text); } + if ( instrumentationConfig.enhancedDatabaseReporting && - queryConfig.values instanceof Array + Array.isArray(queryConfig.values) ) { span.setAttribute( AttributeNames.PG_VALUES, arrayStringifyHelper(queryConfig.values) ); } + // Set plan name attribute, if present - if (queryConfig.name) { + if (typeof queryConfig.name === 'string') { span.setAttribute(AttributeNames.PG_PLAN, queryConfig.name); } return span; } -// Queries where args[1] is a 'values' array -export function handleParameterizedQuery( - this: PgClientExtended, - tracer: Tracer, - instrumentationConfig: PgInstrumentationConfig, - query: string, - values: unknown[] -) { - // Set child span name - const queryCommand = getCommandFromText(query); - const name = PgInstrumentation.BASE_SPAN_NAME + ':' + queryCommand; - const span = startQuerySpan(this, tracer, instrumentationConfig, name); - - // Set attributes - span.setAttribute(SemanticAttributes.DB_STATEMENT, query); - if (instrumentationConfig.enhancedDatabaseReporting) { - span.setAttribute(AttributeNames.PG_VALUES, arrayStringifyHelper(values)); - } - - return span; -} - -// Queries where args[0] is a text query and 'values' was not specified -export function handleTextQuery( - this: PgClientExtended, - tracer: Tracer, - instrumentationConfig: PgInstrumentationConfig, - query: string -) { - // Set child span name - const queryCommand = getCommandFromText(query); - const name = PgInstrumentation.BASE_SPAN_NAME + ':' + queryCommand; - const span = startQuerySpan(this, tracer, instrumentationConfig, name); - - // Set attributes - span.setAttribute(SemanticAttributes.DB_STATEMENT, query); - - 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: PgClientExtended, - tracer: Tracer, - instrumentationConfig: PgInstrumentationConfig, - originalQuery: typeof pgTypes.Client.prototype.query, - ...args: unknown[] -) { - let result; - const span = startQuerySpan( - this, - tracer, - instrumentationConfig, - PgInstrumentation.BASE_SPAN_NAME - ); - try { - result = originalQuery.apply(this, args as never); - } catch (e) { - // span.recordException(e); - span.setStatus({ code: SpanStatusCode.ERROR, message: e.message }); - throw e; - } finally { - span.end(); - } - return result; -} - export function handleExecutionResult( config: PgInstrumentationConfig, span: Span, @@ -358,3 +302,26 @@ export function addSqlCommenterComment(span: Span, query: string): string { return `${query} /*${commentString}*/`; } + +/** + * Attempt to get a message string from a thrown value, while being quite + * defensive, to recognize the fact that, in JS, any kind of value (even + * primitives) can be thrown. + */ +export function getErrorMessage(e: unknown) { + return typeof e === 'object' && e !== null && 'message' in e + ? String((e as { message?: unknown }).message) + : undefined; +} + +export function isObjectWithTextString(it: unknown): it is ObjectWithText { + return ( + typeof it === 'object' && + typeof (it as null | { text?: unknown })?.text === 'string' + ); +} + +export type ObjectWithText = { + text: string; + [k: string]: unknown; +}; diff --git a/plugins/node/opentelemetry-instrumentation-pg/test/utils.test.ts b/plugins/node/opentelemetry-instrumentation-pg/test/utils.test.ts index 4b43e4da95..1ab6dd0c4d 100644 --- a/plugins/node/opentelemetry-instrumentation-pg/test/utils.test.ts +++ b/plugins/node/opentelemetry-instrumentation-pg/test/utils.test.ts @@ -33,7 +33,7 @@ import * as assert from 'assert'; import * as pg from 'pg'; import { PgInstrumentationConfig } from '../src'; import { AttributeNames } from '../src/enums/AttributeNames'; -import { PgClientExtended, NormalizedQueryConfig } from '../src/internal-types'; +import { PgClientExtended } from '../src/internal-types'; import * as utils from '../src/utils'; const memoryExporter = new InMemorySpanExporter(); @@ -131,7 +131,7 @@ describe('utils.ts', () => { }); describe('.handleConfigQuery()', () => { - const queryConfig: NormalizedQueryConfig = { + const queryConfig = { text: 'SELECT $1::text', values: ['0'], }; @@ -171,47 +171,6 @@ describe('utils.ts', () => { }); }); - describe('.handleParameterizedQuery()', () => { - const query = 'SELECT $1::text'; - const values = ['0']; - - it('does not track pg.values by default', async () => { - const querySpan = utils.handleParameterizedQuery.call( - client, - tracer, - instrumentationConfig, - query, - values - ); - querySpan.end(); - - const readableSpan = getLatestSpan(); - - const pgValues = readableSpan.attributes[AttributeNames.PG_VALUES]; - assert.strictEqual(pgValues, undefined); - }); - - it('tracks pg.values if enabled explicitly', async () => { - const extPluginConfig: PgInstrumentationConfig & InstrumentationConfig = { - ...instrumentationConfig, - enhancedDatabaseReporting: true, - }; - const querySpan = utils.handleParameterizedQuery.call( - client, - tracer, - extPluginConfig, - query, - values - ); - querySpan.end(); - - const readableSpan = getLatestSpan(); - - const pgValues = readableSpan.attributes[AttributeNames.PG_VALUES]; - assert.strictEqual(pgValues, '[0]'); - }); - }); - describe('addSqlCommenterComment', () => { it('adds comment to a simple query', () => { const spanContext: SpanContext = { From abd35fbca249929db1191e580b903ce6ee11f26b Mon Sep 17 00:00:00 2001 From: Ethan Resnick Date: Fri, 25 Nov 2022 19:37:13 -0800 Subject: [PATCH 2/4] refactor(pg): dry up attribute generation --- .../src/instrumentation.ts | 14 ++----------- .../src/utils.ts | 21 ++++++++++++------- 2 files changed, 16 insertions(+), 19 deletions(-) diff --git a/plugins/node/opentelemetry-instrumentation-pg/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-pg/src/instrumentation.ts index 9a946cc5f7..f70e39f671 100644 --- a/plugins/node/opentelemetry-instrumentation-pg/src/instrumentation.ts +++ b/plugins/node/opentelemetry-instrumentation-pg/src/instrumentation.ts @@ -136,12 +136,7 @@ export class PgInstrumentation extends InstrumentationBase { `${PgInstrumentation.COMPONENT}.connect`, { [SemanticAttributes.DB_SYSTEM]: DbSystemValues.POSTGRESQL, - [SemanticAttributes.DB_NAME]: this.database, - [SemanticAttributes.NET_PEER_NAME]: this.host, - [SemanticAttributes.DB_CONNECTION_STRING]: - utils.getConnectionString(this), - [SemanticAttributes.NET_PEER_PORT]: this.port, - [SemanticAttributes.DB_USER]: this.user, + ...utils.getSemanticAttributesFromConnection(this), } ); @@ -308,7 +303,6 @@ export class PgInstrumentation extends InstrumentationBase { const plugin = this; return (originalConnect: typeof pgPoolTypes.prototype.connect) => { return function connect(this: PgPoolExtended, callback?: PgPoolCallback) { - const connString = utils.getConnectionString(this.options); // setup span const span = startSpan( plugin.tracer, @@ -316,11 +310,7 @@ export class PgInstrumentation extends InstrumentationBase { `${PG_POOL_COMPONENT}.connect`, { [SemanticAttributes.DB_SYSTEM]: DbSystemValues.POSTGRESQL, - [SemanticAttributes.DB_NAME]: this.options.database, // required - [SemanticAttributes.NET_PEER_NAME]: this.options.host, // required - [SemanticAttributes.DB_CONNECTION_STRING]: connString, // required - [SemanticAttributes.NET_PEER_PORT]: this.options.port, - [SemanticAttributes.DB_USER]: this.options.user, + ...utils.getSemanticAttributesFromConnection(this.options), [AttributeNames.IDLE_TIMEOUT_MILLIS]: this.options.idleTimeoutMillis, [AttributeNames.MAX_CLIENT]: this.options.maxClient, diff --git a/plugins/node/opentelemetry-instrumentation-pg/src/utils.ts b/plugins/node/opentelemetry-instrumentation-pg/src/utils.ts index 970aedaf73..4148870173 100644 --- a/plugins/node/opentelemetry-instrumentation-pg/src/utils.ts +++ b/plugins/node/opentelemetry-instrumentation-pg/src/utils.ts @@ -57,13 +57,25 @@ function getCommandFromText(text?: string): string { return words[0].length > 0 ? words[0] : 'unknown'; } -export function getConnectionString(params: PgClientConnectionParams) { +function getConnectionString(params: PgClientConnectionParams) { const host = params.host || 'localhost'; const port = params.port || 5432; const database = params.database || ''; return `postgresql://${host}:${port}/${database}`; } +export function getSemanticAttributesFromConnection( + params: PgClientConnectionParams +) { + return { + [SemanticAttributes.DB_NAME]: params.database, // required + [SemanticAttributes.DB_CONNECTION_STRING]: getConnectionString(params), // required + [SemanticAttributes.NET_PEER_NAME]: params.host, // required + [SemanticAttributes.NET_PEER_PORT]: params.port, + [SemanticAttributes.DB_USER]: params.user, + }; +} + export function startSpan( tracer: Tracer, instrumentationConfig: PgInstrumentationConfig, @@ -91,14 +103,9 @@ function startQuerySpan( instrumentationConfig: PgInstrumentationConfig, name: string ) { - const jdbcString = getConnectionString(client.connectionParameters); return startSpan(tracer, instrumentationConfig, name, { - [SemanticAttributes.DB_NAME]: client.connectionParameters.database, // required [SemanticAttributes.DB_SYSTEM]: DbSystemValues.POSTGRESQL, // required - [SemanticAttributes.DB_CONNECTION_STRING]: jdbcString, // required - [SemanticAttributes.NET_PEER_NAME]: client.connectionParameters.host, // required - [SemanticAttributes.NET_PEER_PORT]: client.connectionParameters.port, - [SemanticAttributes.DB_USER]: client.connectionParameters.user, + ...getSemanticAttributesFromConnection(client.connectionParameters), }); } From bb4fb8d48d879beab48097c78f53cc7f011d038c Mon Sep 17 00:00:00 2001 From: Ethan Resnick Date: Fri, 25 Nov 2022 19:37:17 -0800 Subject: [PATCH 3/4] fix(pg): spec-compliant span naming BREAKING CHANGE. This improves the way that OTEL span names are generated in two different ways. First, if the query is a prepared statement with a `name`, the `name` is put into the span name as-is. Before, the `name` was transformed first, as though it were a full SQL query string (i.e., it was split on spaces and only the first 'word' was used). Second, when we do only have a full query string, and we take the first word to form the span name, the code now calls `toUpperCase()` on that string, so that a `select * from x` and a `SELECT * from x` query both end up with a span name like `pg.query:SELECT`. --- .../src/utils.ts | 88 ++++++++++++------- .../test/utils.test.ts | 53 +++++++++++ 2 files changed, 109 insertions(+), 32 deletions(-) diff --git a/plugins/node/opentelemetry-instrumentation-pg/src/utils.ts b/plugins/node/opentelemetry-instrumentation-pg/src/utils.ts index 4148870173..d5e74b714c 100644 --- a/plugins/node/opentelemetry-instrumentation-pg/src/utils.ts +++ b/plugins/node/opentelemetry-instrumentation-pg/src/utils.ts @@ -50,11 +50,51 @@ 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'; +/** + * Helper function to get a low cardinality span name from whatever info we have + * about the query. + * + * This is tricky, because we don't have most of the information (table name, + * operation name, etc) the spec recommends using to build a low-cardinality + * value w/o parsing. So, we use db.name and assume that, if the query's a named + * prepared statement, those `name` values will be low cardinality. If we don't + * have a named prepared statement, we try to parse an operation (despite the + * spec's warnings). + * + * @params dbName The name of the db against which this query is being issued, + * which could be missing if no db name was given at the time that the + * connection was established. + * @params queryConfig Information we have about the query being issued, typed + * to reflect only the validation we've actually done on the args to + * `client.query()`. This will be undefined if `client.query()` was called + * with invalid arguments. + */ +export function getQuerySpanName( + dbName: string | undefined, + queryConfig?: { text: string; name?: unknown } +) { + // NB: when the query config is invalid, we omit the dbName too, so that + // someone (or some tool) reading the span name doesn't misinterpret the + // dbName as being a prepared statement or sql commit name. + if (!queryConfig) return PgInstrumentation.BASE_SPAN_NAME; + + // Either the name of a prepared statement; or an attempted parse + // of the SQL command, normalized to uppercase; or unknown. + const command = + typeof queryConfig.name === 'string' && queryConfig.name + ? queryConfig.name + : parseNormalizedOperationName(queryConfig.text); + + return `${PgInstrumentation.BASE_SPAN_NAME}:${command}${ + dbName ? ` ${dbName}` : '' + }`; +} + +function parseNormalizedOperationName(queryText: string) { + const sqlCommand = queryText.split(' ')[0].toUpperCase(); + + // Handle query text being "COMMIT;", which has an extra semicolon before the space. + return sqlCommand.endsWith(';') ? sqlCommand.slice(0, -1) : sqlCommand; } function getConnectionString(params: PgClientConnectionParams) { @@ -96,19 +136,6 @@ export function startSpan( }); } -// Private helper function to start a span after a connection has been established -function startQuerySpan( - client: PgClientExtended, - tracer: Tracer, - instrumentationConfig: PgInstrumentationConfig, - name: string -) { - return startSpan(tracer, instrumentationConfig, name, { - [SemanticAttributes.DB_SYSTEM]: DbSystemValues.POSTGRESQL, // required - ...getSemanticAttributesFromConnection(client.connectionParameters), - }); -} - // Create a span from our normalized queryConfig object, // or return a basic span if no queryConfig was given/could be created. export function handleConfigQuery( @@ -117,23 +144,20 @@ export function handleConfigQuery( instrumentationConfig: PgInstrumentationConfig, queryConfig?: { text: string; values?: unknown; name?: unknown } ) { + // Create child span. + const { connectionParameters } = this; + const dbName = connectionParameters.database; + + const spanName = getQuerySpanName(dbName, queryConfig); + const span = startSpan(tracer, instrumentationConfig, spanName, { + [SemanticAttributes.DB_SYSTEM]: DbSystemValues.POSTGRESQL, // required + ...getSemanticAttributesFromConnection(connectionParameters), + }); + if (!queryConfig) { - return startQuerySpan( - this, - tracer, - instrumentationConfig, - PgInstrumentation.BASE_SPAN_NAME - ); + return span; } - // Set child span name - const queryCommand = getCommandFromText( - (typeof queryConfig.name === 'string' ? queryConfig.name : undefined) || - queryConfig.text - ); - const name = `${PgInstrumentation.BASE_SPAN_NAME}:${queryCommand}`; - const span = startQuerySpan(this, tracer, instrumentationConfig, name); - // Set attributes if (queryConfig.text) { span.setAttribute(SemanticAttributes.DB_STATEMENT, queryConfig.text); diff --git a/plugins/node/opentelemetry-instrumentation-pg/test/utils.test.ts b/plugins/node/opentelemetry-instrumentation-pg/test/utils.test.ts index 1ab6dd0c4d..099ffa3bf4 100644 --- a/plugins/node/opentelemetry-instrumentation-pg/test/utils.test.ts +++ b/plugins/node/opentelemetry-instrumentation-pg/test/utils.test.ts @@ -75,6 +75,59 @@ describe('utils.ts', () => { context.disable(); }); + describe('.getQuerySpanName()', () => { + const dummyQuery = { + text: 'SELECT $1', + values: ['hello'], + name: 'select-placeholder-val', + }; + + it('uses prepared statement name when given, over query text', () => { + assert.strictEqual( + utils.getQuerySpanName('dbName', dummyQuery), + 'pg.query:select-placeholder-val dbName' + ); + }); + + it('falls back to parsing query text when no (valid) name is available', () => { + assert.strictEqual( + utils.getQuerySpanName('dbName', { ...dummyQuery, name: undefined }), + 'pg.query:SELECT dbName' + ); + }); + + it('normalizes operation names parsed from query text', () => { + const queryUpperCase = { text: dummyQuery.text.toUpperCase() }; + const queryLowerCase = { text: dummyQuery.text.toLowerCase() }; + + assert.strictEqual( + utils.getQuerySpanName('dbName', queryUpperCase), + utils.getQuerySpanName('dbName', queryLowerCase) + ); + }); + + it('ignores trailing semicolons when parsing operation names', () => { + assert.strictEqual( + utils.getQuerySpanName('dbName', { text: 'COMMIT;' }), + 'pg.query:COMMIT dbName' + ); + }); + + it('omits db name if missing', () => { + assert.strictEqual( + utils.getQuerySpanName(undefined, dummyQuery), + 'pg.query:select-placeholder-val' + ); + }); + + it('should omit all info if the queryConfig is invalid', () => { + assert.strictEqual( + utils.getQuerySpanName('db-name-ignored', undefined), + 'pg.query' + ); + }); + }); + describe('.startSpan()', () => { it('starts real span when requireParentSpan=false', async () => { const span = utils.startSpan(tracer, instrumentationConfig, 'spanName', { From e3d7e5e3ecec21c784284f9ac424258a62beec8b Mon Sep 17 00:00:00 2001 From: Ethan Resnick Date: Tue, 29 Nov 2022 22:06:34 -0800 Subject: [PATCH 4/4] test(pg): more error handling tests --- .../test/pg.test.ts | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/plugins/node/opentelemetry-instrumentation-pg/test/pg.test.ts b/plugins/node/opentelemetry-instrumentation-pg/test/pg.test.ts index e91b0034fb..be8e1491cf 100644 --- a/plugins/node/opentelemetry-instrumentation-pg/test/pg.test.ts +++ b/plugins/node/opentelemetry-instrumentation-pg/test/pg.test.ts @@ -203,6 +203,26 @@ describe('pg', () => { runCallbackTest(null, DEFAULT_ATTRIBUTES, [], errorStatus); memoryExporter.reset(); + assert.throws( + () => { + (client as any).query(null); + }, + assertPgError, + 'pg should throw when null provided as only arg' + ); + runCallbackTest(null, DEFAULT_ATTRIBUTES, [], errorStatus); + memoryExporter.reset(); + + assert.throws( + () => { + (client as any).query(undefined); + }, + assertPgError, + 'pg should throw when undefined provided as only arg' + ); + runCallbackTest(null, DEFAULT_ATTRIBUTES, [], errorStatus); + memoryExporter.reset(); + assert.doesNotThrow( () => (client as any).query({ foo: 'bar' }, undefined, () => {