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`.

plugins/node/opentelemetry-instrumentation-pg/src/utils.ts

@@ -50,11 +50,51 @@ function arrayStringifyHelper(arr: Array<unknown>): 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);

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', {