From 5e6f99538b1e98120097434f90c291faf8ed8d5f Mon Sep 17 00:00:00 2001
From: Ethan Resnick <ethan.resnick@gmail.com>
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                              | 71 ++++++++++---------
 1 file changed, 39 insertions(+), 32 deletions(-)

diff --git a/plugins/node/opentelemetry-instrumentation-pg/src/utils.ts b/plugins/node/opentelemetry-instrumentation-pg/src/utils.ts
index 24deb06489..a8dc2f46f5 100644
--- a/plugins/node/opentelemetry-instrumentation-pg/src/utils.ts
+++ b/plugins/node/opentelemetry-instrumentation-pg/src/utils.ts
@@ -50,11 +50,34 @@ 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 query The information we have about the query, typed to reflect
+ *   only the validation we've actually done on the `client.query()` args.
+ */
+export function getQuerySpanName(
+  databaseName: string | undefined,
+  queryConfig?: { text: string; name?: unknown }
+) {
+  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
+      : queryConfig.text.split(' ')[0].toUpperCase() || 'unknown';
+
+  return `${PgInstrumentation.BASE_SPAN_NAME}:${command} ${databaseName ?? ''}`;
 }
 
 function getConnectionString(params: PgClientConnectionParams) {
@@ -96,19 +119,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 +127,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);