Merge pull request #9 from heliosphere-io/HLSP-3-CR-comments-2

Hlsp 3 cr comments 2
open-telemetry · Jun 13, 2021 · ca1b357 · ca1b357
2 parents 8b168be + e96a05f
commit ca1b357
Show file tree

Hide file tree

Showing 7 changed files with 42 additions and 35 deletions.
diff --git a/plugins/node/opentelemetry-instrumentation-pg/README.md b/plugins/node/opentelemetry-instrumentation-pg/README.md
@@ -46,6 +46,7 @@ PostgreSQL instrumentation has few options available to choose from. You can set
 | Options | Type | Description |
 | ------- | ---- | ----------- |
 | [`enhancedDatabaseReporting`](https://github.com/open-telemetry/opentelemetry-js-contrib/blob/main/plugins/node/opentelemetry-instrumentation-pg/src/pg.ts#L48) | `boolean` | If true, additional information about query parameters and results will be attached (as `attributes`) to spans representing database operations |
+| `responseHook` | `PgInstrumentationExecutionResponseHook` (function) | Function for adding custom attributes from db response |
 
 ## Supported Versions
 

diff --git a/plugins/node/opentelemetry-instrumentation-pg/src/index.ts b/plugins/node/opentelemetry-instrumentation-pg/src/index.ts
@@ -18,4 +18,5 @@ export * from './instrumentation';
 export {
   PgInstrumentationConfig,
   PgInstrumentationExecutionResponseHook,
+  PgResponseHookInformation,
 } from './types';
diff --git a/plugins/node/opentelemetry-instrumentation-pg/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-pg/src/instrumentation.ts
@@ -60,14 +60,6 @@ export class PgInstrumentation extends InstrumentationBase {
     );
   }
 
-  private _getConfig(): PgInstrumentationConfig {
-    return this._config as PgInstrumentationConfig & InstrumentationConfig;
-  }
-
-  setConfig(config: PgInstrumentationConfig & InstrumentationConfig = {}) {
-    this._config = Object.assign({}, config);
-  }
-
   protected init() {
     const modulePG = new InstrumentationNodeModuleDefinition<typeof pgTypes>(
       'pg',
@@ -133,7 +125,8 @@ export class PgInstrumentation extends InstrumentationBase {
             span = utils.handleParameterizedQuery.call(
               this,
               plugin.tracer,
-              plugin._getConfig(),
+              plugin.getConfig() as PgInstrumentationConfig &
+                InstrumentationConfig,
               query,
               params
             );
@@ -145,7 +138,8 @@ export class PgInstrumentation extends InstrumentationBase {
           span = utils.handleConfigQuery.call(
             this,
             plugin.tracer,
-            plugin._getConfig(),
+            plugin.getConfig() as PgInstrumentationConfig &
+              InstrumentationConfig,
             queryConfig
           );
         } else {
@@ -163,7 +157,8 @@ export class PgInstrumentation extends InstrumentationBase {
           if (typeof args[args.length - 1] === 'function') {
             // Patch ParameterQuery callback
             args[args.length - 1] = utils.patchCallback(
-              plugin._getConfig(),
+              plugin.getConfig() as PgInstrumentationConfig &
+                InstrumentationConfig,
               span,
               args[args.length - 1] as PostgresCallback
             );
@@ -179,7 +174,8 @@ export class PgInstrumentation extends InstrumentationBase {
           ) {
             // Patch ConfigQuery callback
             let callback = utils.patchCallback(
-              plugin._getConfig(),
+              plugin.getConfig() as PgInstrumentationConfig &
+                InstrumentationConfig,
               span,
               (args[0] as NormalizedQueryConfig).callback!
             );
@@ -203,7 +199,12 @@ export class PgInstrumentation extends InstrumentationBase {
             .then((result: unknown) => {
               // Return a pass-along promise which ends the span and then goes to user's orig resolvers
               return new Promise(resolve => {
-                utils.handleExecutionResult(plugin._getConfig(), span, result);
+                utils.handleExecutionResult(
+                  plugin.getConfig() as PgInstrumentationConfig &
+                    InstrumentationConfig,
+                  span,
+                  result
+                );
                 span.end();
                 resolve(result);
               });

diff --git a/plugins/node/opentelemetry-instrumentation-pg/src/types.ts b/plugins/node/opentelemetry-instrumentation-pg/src/types.ts
@@ -19,22 +19,23 @@ import * as pgPoolTypes from 'pg-pool';
 import type * as api from '@opentelemetry/api';
 import { InstrumentationConfig } from '@opentelemetry/instrumentation';
 
+export interface PgResponseHookInformation {
+  data: pgTypes.QueryResult | pgTypes.QueryArrayResult;
+}
+
 export interface PgInstrumentationExecutionResponseHook {
-  (span: api.Span, data: pgTypes.QueryResult | pgTypes.QueryArrayResult): void;
+  (span: api.Span, responseInfo: PgResponseHookInformation): void;
 }
 
 export interface PgInstrumentationConfig extends InstrumentationConfig {
   /**
-   * If true, additional information about query parameters and
-   * results will be attached (as `attributes`) to spans representing
-   * database operations.
+   * If true, additional information about query parameters will be attached (as `attributes`) to spans representing
    */
   enhancedDatabaseReporting?: boolean;
 
   /**
    * Hook that allows adding custom span attributes based on the data
    * returned from "query" Pg actions.
-   * Using this requires that the `enhancedDatabaseReporting` flag be set to true.
    *
    * @default undefined
    */

diff --git a/plugins/node/opentelemetry-instrumentation-pg/src/utils.ts b/plugins/node/opentelemetry-instrumentation-pg/src/utils.ts
@@ -174,17 +174,12 @@ export function handleExecutionResult(
   span: Span,
   pgResult: pgTypes.QueryResult | pgTypes.QueryArrayResult | unknown
 ) {
-  if (
-    config.enhancedDatabaseReporting &&
-    config.responseHook !== undefined &&
-    pgResult !== undefined
-  ) {
+  if (config.responseHook !== undefined && pgResult !== undefined) {
     safeExecuteInTheMiddle(
       () => {
-        config.responseHook!(
-          span,
-          pgResult as pgTypes.QueryResult | pgTypes.QueryArrayResult
-        );
+        config.responseHook!(span, {
+          data: pgResult as pgTypes.QueryResult | pgTypes.QueryArrayResult,
+        });
       },
       err => {
         if (err) {

diff --git a/plugins/node/opentelemetry-instrumentation-pg/test/pg-pool.test.ts b/plugins/node/opentelemetry-instrumentation-pg/test/pg-pool.test.ts
@@ -24,7 +24,11 @@ import {
   trace,
 } from '@opentelemetry/api';
 import { BasicTracerProvider } from '@opentelemetry/tracing';
-import { PgInstrumentation, PgInstrumentationConfig } from '../src';
+import {
+  PgInstrumentation,
+  PgInstrumentationConfig,
+  PgResponseHookInformation,
+} from '../src';
 import { AsyncHooksContextManager } from '@opentelemetry/context-async-hooks';
 import * as testUtils from '@opentelemetry/test-utils';
 import {
@@ -315,11 +319,11 @@ describe('pg-pool@2.x', () => {
             enhancedDatabaseReporting: true,
             responseHook: (
               span: Span,
-              data: pg.QueryResult | pg.QueryArrayResult
+              responseInfo: PgResponseHookInformation
             ) =>
               span.setAttribute(
                 dataAttributeName,
-                JSON.stringify({ rowCount: data.rowCount })
+                JSON.stringify({ rowCount: responseInfo?.data.rowCount })
               ),
           };
 
@@ -398,7 +402,7 @@ describe('pg-pool@2.x', () => {
             enhancedDatabaseReporting: true,
             responseHook: (
               span: Span,
-              data: pg.QueryResult | pg.QueryArrayResult
+              responseInfo: PgResponseHookInformation
             ) => {
               throw 'some kind of failure!';
             },

diff --git a/plugins/node/opentelemetry-instrumentation-pg/test/pg.test.ts b/plugins/node/opentelemetry-instrumentation-pg/test/pg.test.ts
@@ -32,7 +32,11 @@ import {
 } from '@opentelemetry/tracing';
 import * as assert from 'assert';
 import type * as pg from 'pg';
-import { PgInstrumentation, PgInstrumentationConfig } from '../src';
+import {
+  PgInstrumentation,
+  PgInstrumentationConfig,
+  PgResponseHookInformation,
+} from '../src';
 import { AttributeNames } from '../src/enums/AttributeNames';
 import { TimedEvent } from './types';
 import {
@@ -389,11 +393,11 @@ describe('pg@7.x', () => {
             enhancedDatabaseReporting: true,
             responseHook: (
               span: Span,
-              data: pg.QueryResult | pg.QueryArrayResult
+              responseInfo: PgResponseHookInformation
             ) =>
               span.setAttribute(
                 dataAttributeName,
-                JSON.stringify({ rowCount: data.rowCount })
+                JSON.stringify({ rowCount: responseInfo?.data.rowCount })
               ),
           };
           create(config);
@@ -448,7 +452,7 @@ describe('pg@7.x', () => {
             enhancedDatabaseReporting: true,
             responseHook: (
               span: Span,
-              data: pg.QueryResult | pg.QueryArrayResult
+              responseInfo: PgResponseHookInformation
             ) => {
               throw 'some kind of failure!';
             },