From bb4fb8d48d879beab48097c78f53cc7f011d038c Mon Sep 17 00:00:00 2001 From: Ethan Resnick Date: Fri, 25 Nov 2022 19:37:17 -0800 Subject: [PATCH] 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', {