Skip to content

Commit

Permalink
fix(instrumentation-mysql2): by default do not include parameterized …
Browse files Browse the repository at this point in the history 
…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
  • Loading branch information
@macno
macno committed Dec 12, 2023
1 parent 34d2dc4 commit 4fa33f0
Show file tree
Hide file tree
Showing 6 changed files with 315 additions and 38 deletions.
6 changes: 4 additions & 2 deletions plugins/node/opentelemetry-instrumentation-mysql2/README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
27 changes: 15 additions & 12 deletions plugins/node/opentelemetry-instrumentation-mysql2/src/instrumentation.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,13 +38,17 @@ import { VERSION } from './version';

type formatType = typeof mysqlTypes.format;

export class MySQL2Instrumentation extends InstrumentationBase<any> {
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() {
Expand Down Expand Up @@ -100,9 +104,6 @@ export class MySQL2Instrumentation extends InstrumentationBase<any> {
_valuesOrCallback?: unknown[] | Function,
_callback?: Function
) {
const thisPluginConfig: MySQL2InstrumentationConfig =
thisPlugin._config;

let values;
if (Array.isArray(_valuesOrCallback)) {
values = _valuesOrCallback;
Expand All @@ -117,13 +118,16 @@ export class MySQL2Instrumentation extends InstrumentationBase<any> {
...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)
Expand All @@ -139,10 +143,10 @@ export class MySQL2Instrumentation extends InstrumentationBase<any> {
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,
});
},
Expand All @@ -155,7 +159,6 @@ export class MySQL2Instrumentation extends InstrumentationBase<any> {
);
}
}

span.end();
});

Expand Down
10 changes: 10 additions & 0 deletions plugins/node/opentelemetry-instrumentation-mysql2/src/types.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
27 changes: 18 additions & 9 deletions plugins/node/opentelemetry-instrumentation-mysql2/src/utils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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 {
Expand Down Expand Up @@ -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;
}

/**
Expand Down
46 changes: 31 additions & 15 deletions plugins/node/opentelemetry-instrumentation-mysql2/test/mysql.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand All @@ -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;
Expand All @@ -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();

Expand Down Expand Up @@ -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();
}
});
Expand All @@ -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();
});
});

Expand Down Expand Up @@ -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(() => {
Expand Down Expand Up @@ -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) => {
Expand Down
Loading

0 comments on commit 4fa33f0

Please sign in to comment.