From 4fa33f015f6765de12646c974917dc3e17236bb5 Mon Sep 17 00:00:00 2001 From: Michele Azzolari Date: Mon, 11 Dec 2023 13:05:25 +0100 Subject: [PATCH] fix(instrumentation-mysql2): by default do not include parameterized values to db.statement span attribute (#1758) * feat: add configuration to enable inclusion of parameterized values in db.statement span attribute * feat: add configuration to set max length for db.statement span attribute * fix: replace deprecated SpanAttributes with Attributes --- .../README.md | 6 +- .../src/instrumentation.ts | 27 +- .../src/types.ts | 10 + .../src/utils.ts | 27 +- .../test/mysql.test.ts | 46 ++-- .../test/mysql2.test.ts | 237 ++++++++++++++++++ 6 files changed, 315 insertions(+), 38 deletions(-) create mode 100644 plugins/node/opentelemetry-instrumentation-mysql2/test/mysql2.test.ts diff --git a/plugins/node/opentelemetry-instrumentation-mysql2/README.md b/plugins/node/opentelemetry-instrumentation-mysql2/README.md index 04f15355ef..cb491882d1 100644 --- a/plugins/node/opentelemetry-instrumentation-mysql2/README.md +++ b/plugins/node/opentelemetry-instrumentation-mysql2/README.md @@ -44,10 +44,12 @@ registerInstrumentations({ You can set the following instrumentation options: -| Options | Type | Description | -| ------- | ---- | ----------- | +| Options | Type | Description | +| ------- | ---- |------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| | `responseHook` | `MySQL2InstrumentationExecutionResponseHook` (function) | Function for adding custom attributes from db response | | `addSqlCommenterCommentToQueries` | `boolean` | If true, adds [sqlcommenter](https://github.com/open-telemetry/opentelemetry-sqlcommenter) specification compliant comment to queries with tracing context (default false). _NOTE: A comment will not be added to queries that already contain `--` or `/* ... */` in them, even if these are not actually part of comments_ | +| `includeValuesInDbStatement` | `boolean` | If true, adds parameterized values to `db.statement` Span attribute (default false) | +| `dbStatementMaxLength` | `integer` | If set, truncates the value of `db.statement` Span attribute to the configured length (default unlimited) | ## Useful links diff --git a/plugins/node/opentelemetry-instrumentation-mysql2/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-mysql2/src/instrumentation.ts index ea792f8025..7072e8e3ee 100644 --- a/plugins/node/opentelemetry-instrumentation-mysql2/src/instrumentation.ts +++ b/plugins/node/opentelemetry-instrumentation-mysql2/src/instrumentation.ts @@ -38,13 +38,17 @@ import { VERSION } from './version'; type formatType = typeof mysqlTypes.format; -export class MySQL2Instrumentation extends InstrumentationBase { +export class MySQL2Instrumentation extends InstrumentationBase { static readonly COMMON_ATTRIBUTES = { [SemanticAttributes.DB_SYSTEM]: DbSystemValues.MYSQL, }; - constructor(config?: MySQL2InstrumentationConfig) { - super('@opentelemetry/instrumentation-mysql2', VERSION, config); + constructor(protected override _config: MySQL2InstrumentationConfig = {}) { + super('@opentelemetry/instrumentation-mysql2', VERSION, _config); + } + + override setConfig(config: MySQL2InstrumentationConfig = {}) { + this._config = config; } protected init() { @@ -100,9 +104,6 @@ export class MySQL2Instrumentation extends InstrumentationBase { _valuesOrCallback?: unknown[] | Function, _callback?: Function ) { - const thisPluginConfig: MySQL2InstrumentationConfig = - thisPlugin._config; - let values; if (Array.isArray(_valuesOrCallback)) { values = _valuesOrCallback; @@ -117,13 +118,16 @@ export class MySQL2Instrumentation extends InstrumentationBase { ...getConnectionAttributes(this.config), [SemanticAttributes.DB_STATEMENT]: getDbStatement( query, - format, - values + values, + thisPlugin._config.includeValuesInDbStatement + ? format + : undefined, + thisPlugin._config.dbStatementMaxLength ), }, }); - if (!isPrepared && thisPluginConfig.addSqlCommenterCommentToQueries) { + if (!isPrepared && thisPlugin._config.addSqlCommenterCommentToQueries) { arguments[0] = query = typeof query === 'string' ? addSqlCommenterComment(span, query) @@ -139,10 +143,10 @@ export class MySQL2Instrumentation extends InstrumentationBase { message: err.message, }); } else { - if (typeof thisPluginConfig.responseHook === 'function') { + if (typeof thisPlugin._config.responseHook === 'function') { safeExecuteInTheMiddle( () => { - thisPluginConfig.responseHook!(span, { + thisPlugin._config.responseHook!(span, { queryResults: results, }); }, @@ -155,7 +159,6 @@ export class MySQL2Instrumentation extends InstrumentationBase { ); } } - span.end(); }); diff --git a/plugins/node/opentelemetry-instrumentation-mysql2/src/types.ts b/plugins/node/opentelemetry-instrumentation-mysql2/src/types.ts index 01e9f8a434..77eb6b0849 100644 --- a/plugins/node/opentelemetry-instrumentation-mysql2/src/types.ts +++ b/plugins/node/opentelemetry-instrumentation-mysql2/src/types.ts @@ -39,4 +39,14 @@ export interface MySQL2InstrumentationConfig extends InstrumentationConfig { * the tracing context, following the {@link https://github.com/open-telemetry/opentelemetry-sqlcommenter sqlcommenter} format */ addSqlCommenterCommentToQueries?: boolean; + + /** + * If true, queries are modified to include values + */ + includeValuesInDbStatement?: boolean; + + /** + * If set, truncate the query to the specified length + */ + dbStatementMaxLength?: number; } diff --git a/plugins/node/opentelemetry-instrumentation-mysql2/src/utils.ts b/plugins/node/opentelemetry-instrumentation-mysql2/src/utils.ts index 3091569a40..21c6aa22a8 100644 --- a/plugins/node/opentelemetry-instrumentation-mysql2/src/utils.ts +++ b/plugins/node/opentelemetry-instrumentation-mysql2/src/utils.ts @@ -14,16 +14,16 @@ * limitations under the License. */ -import { SpanAttributes } from '@opentelemetry/api'; +import { Attributes } from '@opentelemetry/api'; import { SemanticAttributes } from '@opentelemetry/semantic-conventions'; /* Following types declare an expectation on mysql2 types and define a subset we - use in the instrumentation of the types actually defined in mysql2 pacakge + use in the instrumentation of the types actually defined in mysql2 package We need to import them here so that the installing party of the instrumentation doesn't have to absolutely install the mysql2 package as well - specially - important for auto-loaders and meta-pacakges. + important for auto-loaders and meta-packages. */ interface QueryOptions { sql: string; @@ -41,12 +41,13 @@ interface Config { user?: string; connectionConfig?: Config; } + /** * Get an SpanAttributes map from a mysql connection config object * * @param config ConnectionConfig */ -export function getConnectionAttributes(config: Config): SpanAttributes { +export function getConnectionAttributes(config: Config): Attributes { const { host, port, database, user } = getConfig(config); return { @@ -93,23 +94,31 @@ function getJDBCString( */ export function getDbStatement( query: string | Query | QueryOptions, - format: ( + values?: any[], + format?: ( sql: string, values: any[], stringifyObjects?: boolean, timeZone?: string ) => string, - values?: any[] + statementLimit?: number ): string { + let statement = ''; if (typeof query === 'string') { - return values ? format(query, values) : query; + statement = format ? (values ? format(query, values) : query) : query; } else { // According to https://github.com/mysqljs/mysql#performing-queries // The values argument will override the values in the option object. - return values || (query as QueryOptions).values - ? format(query.sql, values || (query as QueryOptions).values) + statement = format + ? values || (query as QueryOptions).values + ? format(query.sql, values || (query as QueryOptions).values) + : query.sql : query.sql; } + if (statementLimit) { + return statement.substring(0, statementLimit) + '[...]'; + } + return statement; } /** diff --git a/plugins/node/opentelemetry-instrumentation-mysql2/test/mysql.test.ts b/plugins/node/opentelemetry-instrumentation-mysql2/test/mysql.test.ts index 0e81e55a66..bf67e4ac14 100644 --- a/plugins/node/opentelemetry-instrumentation-mysql2/test/mysql.test.ts +++ b/plugins/node/opentelemetry-instrumentation-mysql2/test/mysql.test.ts @@ -39,7 +39,8 @@ const user = process.env.MYSQL_USER || 'otel'; const password = process.env.MYSQL_PASSWORD || 'secret'; const rootPassword = process.env.MYSQL_ROOT_PASSWORD || 'rootpw'; -const instrumentation = new MySQL2Instrumentation(); +const instrumentationConfig = { includeValuesInDbStatement: true }; +const instrumentation = new MySQL2Instrumentation(instrumentationConfig); instrumentation.enable(); instrumentation.disable(); @@ -49,7 +50,7 @@ interface Result extends mysqlTypes.RowDataPacket { solution: number; } -describe('mysql2@2.x', () => { +describe('mysql2@2.x includeValuesInDbStatement=true (old behaviour)', () => { let contextManager: AsyncHooksContextManager; let connection: mysqlTypes.Connection; let rootConnection: mysqlTypes.Connection; @@ -58,6 +59,9 @@ describe('mysql2@2.x', () => { const provider = new BasicTracerProvider(); const testMysql = process.env.RUN_MYSQL_TESTS; // For CI: assumes local mysql db is already available const testMysqlLocally = process.env.RUN_MYSQL_TESTS_LOCAL; // For local: spins up local mysql db via docker + const testMysqlLocallyImage = process.env.RUN_MYSQL_TESTS_LOCAL_USE_MARIADB + ? 'mariadb' + : 'mysql'; // For local: spins up mysql (default) or mariadb const shouldTest = testMysql || testMysqlLocally; // Skips these tests if false (default) const memoryExporter = new InMemorySpanExporter(); @@ -87,19 +91,27 @@ describe('mysql2@2.x', () => { this.skip(); } provider.addSpanProcessor(new SimpleSpanProcessor(memoryExporter)); - rootConnection = mysqlTypes.createConnection({ - port, - user: 'root', - host, - password: rootPassword, - database, - }); + + function openRootConnection() { + rootConnection = mysqlTypes.createConnection({ + port, + user: 'root', + host, + password: rootPassword, + database, + }); + } + if (testMysqlLocally) { - testUtils.startDocker('mysql'); + testUtils.startDocker(testMysqlLocallyImage); // wait 15 seconds for docker container to start this.timeout(20000); - setTimeout(done, 15000); + setTimeout(() => { + openRootConnection(); + done(); + }, 15000); } else { + openRootConnection(); done(); } }); @@ -108,9 +120,13 @@ describe('mysql2@2.x', () => { rootConnection.end(() => { if (testMysqlLocally) { this.timeout(5000); - testUtils.cleanUpDocker('mysql'); + setTimeout(() => { + testUtils.cleanUpDocker(testMysqlLocallyImage); + done(); + }, 1000); + } else { + done(); } - done(); }); }); @@ -149,7 +165,7 @@ describe('mysql2@2.x', () => { afterEach(done => { context.disable(); memoryExporter.reset(); - instrumentation.setConfig(); + instrumentation.setConfig(instrumentationConfig); instrumentation.disable(); connection.end(() => { pool.end(() => { @@ -1101,7 +1117,7 @@ describe('mysql2@2.x', () => { describe('#responseHook', () => { const queryResultAttribute = 'query_result'; - describe('invalid repsonse hook', () => { + describe('invalid response hook', () => { beforeEach(() => { const config: MySQL2InstrumentationConfig = { responseHook: (span, responseHookInfo) => { diff --git a/plugins/node/opentelemetry-instrumentation-mysql2/test/mysql2.test.ts b/plugins/node/opentelemetry-instrumentation-mysql2/test/mysql2.test.ts new file mode 100644 index 0000000000..839e586f9c --- /dev/null +++ b/plugins/node/opentelemetry-instrumentation-mysql2/test/mysql2.test.ts @@ -0,0 +1,237 @@ +/* + * Copyright The 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 { context, trace } from '@opentelemetry/api'; +import { AsyncHooksContextManager } from '@opentelemetry/context-async-hooks'; +import { SemanticAttributes } from '@opentelemetry/semantic-conventions'; +import * as testUtils from '@opentelemetry/contrib-test-utils'; +import { + BasicTracerProvider, + InMemorySpanExporter, + SimpleSpanProcessor, +} from '@opentelemetry/sdk-trace-base'; +import * as assert from 'assert'; +import { MySQL2Instrumentation } from '../src'; + +const LIB_VERSION = testUtils.getPackageVersion('mysql2'); + +const port = Number(process.env.MYSQL_PORT) || 33306; +const database = process.env.MYSQL_DATABASE || 'test_db'; +const host = process.env.MYSQL_HOST || '127.0.0.1'; +const user = process.env.MYSQL_USER || 'otel'; +const password = process.env.MYSQL_PASSWORD || 'secret'; +const rootPassword = process.env.MYSQL_ROOT_PASSWORD || 'rootpw'; + +const instrumentation = new MySQL2Instrumentation(); +instrumentation.enable(); +instrumentation.disable(); + +import * as mysqlTypesPromise from 'mysql2/promise'; + +// FIXME when support to node < 16 is dropped, use -> import { setTimeout } from 'node:timers/promises'; +function _setTimetout(ms: number) { + return new Promise(resolve => { + setTimeout(resolve, ms); + }); +} + +describe('mysql2@' + LIB_VERSION, () => { + let contextManager: AsyncHooksContextManager; + let connection: mysqlTypesPromise.Connection; + let rootConnection: mysqlTypesPromise.Connection; + const provider = new BasicTracerProvider(); + const memoryExporter = new InMemorySpanExporter(); + const testMysql = process.env.RUN_MYSQL_TESTS; // For CI: assumes local mysql db is already available + const testMysqlLocally = process.env.RUN_MYSQL_TESTS_LOCAL; // For local: spins up local mysql db via docker + const testMysqlLocallyImage = process.env.RUN_MYSQL_TESTS_LOCAL_USE_MARIADB + ? 'mariadb' + : 'mysql'; // For local: spins up mysql (default) or mariadb + const shouldTest = testMysql || testMysqlLocally; // Skips these tests if false (default) + + before(async function () { + if (!shouldTest) { + // this.skip() workaround + // https://github.com/mochajs/mocha/issues/2683#issuecomment-375629901 + this.test!.parent!.pending = true; + this.skip(); + } + provider.addSpanProcessor(new SimpleSpanProcessor(memoryExporter)); + if (testMysqlLocally) { + testUtils.startDocker(testMysqlLocallyImage); + // wait 15 seconds for docker container to start + this.timeout(25000); + await _setTimetout(20000); + } + rootConnection = await mysqlTypesPromise.createConnection({ + port, + user: 'root', + host, + password: rootPassword, + database, + }); + }); + + after(async function () { + await rootConnection.end(); + if (testMysqlLocally) { + this.timeout(5000); + testUtils.cleanUpDocker(testMysqlLocallyImage); + } + }); + + beforeEach(async () => { + instrumentation.disable(); + contextManager = new AsyncHooksContextManager().enable(); + context.setGlobalContextManager(contextManager); + instrumentation.setTracerProvider(provider); + instrumentation.enable(); + connection = await mysqlTypesPromise.createConnection({ + port, + user, + host, + password, + database, + }); + }); + + afterEach(async () => { + context.disable(); + memoryExporter.reset(); + instrumentation.setConfig({}); + instrumentation.disable(); + await connection.end(); + }); + + describe('query() when the statement is a string', () => { + it('should name the span accordingly ', async () => { + const span = provider.getTracer('default').startSpan('test span'); + await context.with(trace.setSpan(context.active(), span), async () => { + const sql = 'SELECT 1+? as solution'; + await connection.query(sql, [1]); + const spans = memoryExporter.getFinishedSpans(); + assert.strictEqual(spans[0].name, 'SELECT'); + assert.strictEqual( + spans[0].attributes[SemanticAttributes.DB_STATEMENT], + sql + ); + }); + }); + it('should truncate the statement if dbStatementMaxLength is set', async () => { + instrumentation.setConfig({ dbStatementMaxLength: 13 }); + const span = provider.getTracer('default').startSpan('test span'); + await context.with(trace.setSpan(context.active(), span), async () => { + const sql = 'SELECT 1+? as solution'; + await connection.query(sql, [1]); + const spans = memoryExporter.getFinishedSpans(); + assert.strictEqual(spans[0].name, 'SELECT'); + assert.strictEqual( + spans[0].attributes[SemanticAttributes.DB_STATEMENT], + 'SELECT 1+? as[...]' + ); + }); + }); + }); + + describe('query() when the statement is an object', () => { + it('should name the span accordingly ', async () => { + const span = provider.getTracer('default').startSpan('test span'); + await context.with(trace.setSpan(context.active(), span), async () => { + const sql = 'SELECT 1+? as solution'; + await connection.query({ sql, values: [1] }); + const spans = memoryExporter.getFinishedSpans(); + assert.strictEqual(spans[0].name, 'SELECT'); + assert.strictEqual( + spans[0].attributes[SemanticAttributes.DB_STATEMENT], + sql + ); + }); + }); + it('should truncate the statement if dbStatementMaxLength is set', async () => { + instrumentation.setConfig({ dbStatementMaxLength: 13 }); + const span = provider.getTracer('default').startSpan('test span'); + await context.with(trace.setSpan(context.active(), span), async () => { + const sql = 'SELECT 1+? as solution'; + await connection.query({ sql, values: [1] }); + const spans = memoryExporter.getFinishedSpans(); + assert.strictEqual(spans[0].name, 'SELECT'); + assert.strictEqual( + spans[0].attributes[SemanticAttributes.DB_STATEMENT], + 'SELECT 1+? as[...]' + ); + }); + }); + }); + + describe('execute() when the statement is a string', () => { + it('should name the span accordingly ', async () => { + const span = provider.getTracer('default').startSpan('test span'); + await context.with(trace.setSpan(context.active(), span), async () => { + const sql = 'SELECT 1+? as solution'; + await connection.execute(sql, [1]); + const spans = memoryExporter.getFinishedSpans(); + assert.strictEqual(spans[0].name, 'SELECT'); + assert.strictEqual( + spans[0].attributes[SemanticAttributes.DB_STATEMENT], + sql + ); + }); + }); + it('should truncate the statement if dbStatementMaxLength is set', async () => { + instrumentation.setConfig({ dbStatementMaxLength: 13 }); + const span = provider.getTracer('default').startSpan('test span'); + await context.with(trace.setSpan(context.active(), span), async () => { + const sql = 'SELECT 1+? as solution'; + await connection.execute(sql, [1]); + const spans = memoryExporter.getFinishedSpans(); + assert.strictEqual(spans[0].name, 'SELECT'); + assert.strictEqual( + spans[0].attributes[SemanticAttributes.DB_STATEMENT], + 'SELECT 1+? as[...]' + ); + }); + }); + }); + + describe('execute() when the statement is an object', () => { + it('should name the span accordingly ', async () => { + const span = provider.getTracer('default').startSpan('test span'); + await context.with(trace.setSpan(context.active(), span), async () => { + const sql = 'SELECT 1+? as solution'; + await connection.execute({ sql, values: [1] }); + const spans = memoryExporter.getFinishedSpans(); + assert.strictEqual(spans[0].name, 'SELECT'); + assert.strictEqual( + spans[0].attributes[SemanticAttributes.DB_STATEMENT], + sql + ); + }); + }); + it('should truncate the statement if dbStatementMaxLength is set', async () => { + instrumentation.setConfig({ dbStatementMaxLength: 13 }); + const span = provider.getTracer('default').startSpan('test span'); + await context.with(trace.setSpan(context.active(), span), async () => { + const sql = 'SELECT 1+? as solution'; + await connection.execute({ sql, values: [1] }); + const spans = memoryExporter.getFinishedSpans(); + assert.strictEqual(spans[0].name, 'SELECT'); + assert.strictEqual( + spans[0].attributes[SemanticAttributes.DB_STATEMENT], + 'SELECT 1+? as[...]' + ); + }); + }); + }); +});