feat(oracledb): Add support for Oracle DB instrumentation #2612

sudarshan12s · 2024-12-20T17:52:43Z

Add Oracledb instrumentation

Which problem is this PR solving?

Adding instrumentation support for node-oracledb applications connecting to Oracle database.

Short description of the changes

From node-oracledb version 6.7 onwards, oracledb module exports a class,
TraceHandlerBase which simulates interface kind of functionality with abstract methods to be overwritten by the derived class.

The current OT module derives this class TraceHandlerBase and implements abstract methods; It uses setTraceInstance method of oracledb module to register this new derived class OracleTelemetryTraceHandler
These methods are invoked by oracledb module providing the traceContext which is used to dump the data in to the span after converting to standard span attributes.

Following are the abstract methods implemented:

onEnterFn -> It gets invoked by oracledb module when application invokes getConnection to create a connection to
database or execute method to run a sql. It is invoked before invoking the public API calls made by application.
onExitFn -> It gets invoked after the public API call is completed with success/failure.
onBeginRoundTrip -> It gets invoked before round trip to database which was triggered as part of public API.
onEndRoundTrip -> It gets invoked after round trip to database completes which was triggered as part of public API..

Add Oracledb instrumentation

linux-foundation-easycla · 2024-12-20T17:52:48Z

The committers listed above are authorized under a signed CLA.

✅ login: sudarshan12s / name: Sudarshan Soma (91136ce, 5c4f37f, 4c36312, 759ae13, f41f62c, c299caa, 7f00bf6, 07c76fd, 93d21e1, e076936, e1698e1, 13e79db, 3774f0d, f670ac0, 90db232, 91d65e4, e19e1ea, 6219bc4, 6faff54, 3ae4d72, 34cd418, bf73ff5, ae1b7ac, badc18a, 77a08a7, 440db83, 34ef02d, 34afcad, 0e25652, 30a2367, c8e0be6, 92c1c80, 604f833, d25ee8e, c7fcb17, 8ceff01, 2b310c5, 0290b81, 0704e73, a06b40f, 5c98035, 2296e18, fa0463b, 5faff20, 7cccc2e)

…trib

blumamir

Did another round of review and added few more comments.

Currently marking as "RequestChanges" due to the following 2 issues:

the copyright to oracle should be addressed (not sure in what way) @open-telemetry/javascript-maintainers
the instrumentation must not import from the instrumented package.

Thanks again

blumamir · 2025-02-21T17:04:10Z

plugins/node/opentelemetry-instrumentation-oracledb/src/instrumentation.ts

+
+        // Register the instance with oracledb.
+        newmoduleExports.traceHandler.setTraceInstance(obj);
+        this._tmHandler = obj;


the patch function can be called more than once per instrumentation instance.
It will happen if the instrumented package is installed multiple times in the node_modules (for example - with 2 different versions).

While it's not very likely for oracle db driver, it's an anti pattern for instrumentation.

I am using non null check to see if its patched already, then unregister and register again. Please add if i missed anything.

if (this._tmHandler) {

blumamir · 2025-02-21T17:07:58Z

plugins/node/opentelemetry-instrumentation-oracledb/src/instrumentation.ts

+      'oracledb',
+      ['>= 6.7 < 7'],
+      (moduleExports: typeof oracleDBTypes) => {
+        const newmoduleExports: any = moduleExports;


nit: it's not actually new, right?
the changes we apply here will modify the moduleExports itself.

I think all other instrumentation are updating the moduleExports var directly so it can be nice to align for consistency and to reflect that the original object is modified and not a copy.

same for the unpatch

Yes removed newmoduleExports variable.

blumamir · 2025-02-22T08:08:01Z

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

+  ATTR_DB_NAMESPACE,
+  ATTR_DB_OPERATION_NAME,
+} from './semconv';
+import * as oracledbTypes from 'oracledb';


when this instrumentation is added to the auto-instrumentation package, or any other otel distribution, this line will ALWAYS run.

If the user has the oracledb package in the node_modules, with an up-to-date version, things will work fine. When the instrumented package is not install, this line will crash the app as the import will fail.

read more here

Why do you need to extend a class and not implement an interface? unfortunately this cannot merge as is

blumamir · 2025-02-22T08:23:43Z

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

+        if (this._instrumentConfig.enhancedDatabaseReporting && !roundTrip) {
+          const values = this._getValues(callConfig.values);
+          if (values) {
+            span.setAttribute(AttributeNames.ORACLE_BIND_VALUES, values);


semantic conventions defined an attribute per value:
https://github.com/open-telemetry/semantic-conventions/blob/main/docs/attributes-registry/db.md#db-operation-parameter

should we align this instrumentation?

I will try. But since it was more for debugging and its a sensitive information, I think its less priority.

plugins/node/opentelemetry-instrumentation-oracledb/src/types.ts

blumamir · 2025-02-22T08:32:11Z

plugins/node/opentelemetry-instrumentation-oracledb/src/types.ts

+  enhancedDatabaseReporting?: boolean;
+
+  /**
+   * If true, db.statement will have sql in the spans generated.


also here - the query will be recorded and can potentially contain sensitive unparameterized data. can be nice to add a note here to let users know this option might be dangerous.

Added the following:

If true, db.statement will have sql which could potentially contain sensitive unparameterized data in the spans generated.

blumamir · 2025-02-22T08:36:36Z

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

this file is named utils but it's not hosting util functions.
consider renaming it to OracleTelemetryTraceHandler.ts

Renamed to OracleTelemetryTraceHandler.ts

blumamir · 2025-02-22T08:44:14Z

plugins/node/opentelemetry-instrumentation-oracledb/src/types.ts

+
+export interface OracleRequestHookInformation {
+  inputArgs: any;
+  connection: SpanConnectionConfig | undefined;


when will this be undefined?
or is it only for safety?

removed undefined.

blumamir · 2025-02-22T09:22:42Z

For becoming members of OTel org, @marcalff has agreed for first sponsor. Can someone from javascript-maintainers be the second sponsor.

Once this PR is finalized and ready to merge, I can be your second sponsor for the organization membership

…trib

plugins/node/opentelemetry-instrumentation-oracledb/src/types.ts

sudarshan12s · 2025-02-26T16:27:28Z

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

Renamed to OracleTelemetryTraceHandler.ts

sudarshan12s · 2025-02-26T16:27:32Z

plugins/node/opentelemetry-instrumentation-oracledb/src/types.ts

+  enhancedDatabaseReporting?: boolean;
+
+  /**
+   * If true, db.statement will have sql in the spans generated.


Added the following:

If true, db.statement will have sql which could potentially contain sensitive unparameterized data in the spans generated.

sudarshan12s · 2025-02-26T16:28:33Z

plugins/node/opentelemetry-instrumentation-oracledb/src/instrumentation.ts

+      'oracledb',
+      ['>= 6.7 < 7'],
+      (moduleExports: typeof oracleDBTypes) => {
+        const newmoduleExports: any = moduleExports;


Yes removed newmoduleExports variable.

sudarshan12s · 2025-02-26T16:30:14Z

plugins/node/opentelemetry-instrumentation-oracledb/src/instrumentation.ts

+
+        // Register the instance with oracledb.
+        newmoduleExports.traceHandler.setTraceInstance(obj);
+        this._tmHandler = obj;


I am using non null check to see if its patched already, then unregister and register again. Please add if i missed anything.

if (this._tmHandler) {

sudarshan12s · 2025-02-26T16:31:47Z

plugins/node/opentelemetry-instrumentation-oracledb/src/types.ts

+
+export interface OracleRequestHookInformation {
+  inputArgs: any;
+  connection: SpanConnectionConfig | undefined;


removed undefined.

sudarshan12s · 2025-02-26T16:39:46Z

plugins/node/opentelemetry-instrumentation-oracledb/src/OracleTelemetryTraceHandler.ts

+import type { traceHandler } from 'oracledb'; // Import only for type checking
+
+// It returns the TraceHandlerBase class, if oracledb module is available.
+function getTraceHandlerBaseClass(): TraceHandlerBaseConstructor | null {


For the issue with requiring oracledb to be present in node_modules with this line:
import * as oracledbTypes from 'oracledb';

I made changes to import types instead:
import type { traceHandler } from 'oracledb';

Now since oracledb module exposes TraceHandlerBase as abstract class where some default implementations (enable) are available, I can not do implements TraceHandlerBase.
Added a function, getTraceHandlerBaseClass which gives the constructor for TraceHandlerBase dynamically during patch avoiding global require("oracledb") in generated js files.

The types added inside this file will be removed once this PR and release including this change is done.

Please confirm if i am missed anything or any suggestions.

Thanks for pointing it out with details.

Note that the instance of oracledb that's being required by the instrumentation package here might differ from the one that is being patched. Given you have access to the exports from the patch method it would be a better idea to pass them here as a parameter and get the base class from the instance that's being instrumented.

got it. I made the changes accordingly.

sudarshan12s · 2025-02-26T16:42:42Z

Did another round of review and added few more comments.

Currently marking as "RequestChanges" due to the following 2 issues:

the copyright to oracle should be addressed (not sure in what way) @open-telemetry/javascript-maintainers

the instrumentation must not import from the instrumented package.

Thanks again

I have addressed the second point by including types , Please see.

…trib

moduleExports passed from patch function

sudarshan12s · 2025-03-05T15:10:50Z

Did another round of review and added few more comments.
Currently marking as "RequestChanges" due to the following 2 issues:

the copyright to oracle should be addressed (not sure in what way) @open-telemetry/javascript-maintainers

the instrumentation must not import from the instrumented package.

Thanks again

I have addressed the second point by including types , Please see.

I have removed the copyrights from src/. It is only added in component_owners.yml

dyladan · 2025-03-05T17:28:56Z

https://github.com/cncf/foundation/blob/main/copyright-notices.md#what-if-i-want-my-copyright-notice-included

removed copyright from component_owners.yml

blumamir

LGTM.

adding the relevant part from OpenTelemetry community regarding the copyright notice for future reference:

https://github.com/cncf/foundation/blob/main/copyright-notices.md#what-if-i-want-my-copyright-notice-included

blumamir · 2025-03-17T14:52:28Z

plugins/node/opentelemetry-instrumentation-oracledb/src/OracleTelemetryTraceHandler.ts

+  obj: typeof oracleDBTypes
+): any {
+  const traceHandlerBase = getTraceHandlerBaseClass(obj);
+  if (traceHandlerBase) {


nit: instead of creating an if with >300 lines of code, we can revert the check and exist fast:

Suggested change

if (traceHandlerBase) {

if (!traceHandlerBase) {

return undefined;

}

// else - all the code below, not nested and no indentation.

…trib

use new propery, instrumentationScope and replaced addSpanProcessor with new syntax of BasicTracerProvider constructor.

…trib

Add oracledb instrumentation (#1)

90db232

Add Oracledb instrumentation

sudarshan12s requested a review from a team as a code owner December 20, 2024 17:52

sudarshan12s added 3 commits February 18, 2025 21:11

Merge branch 'main' of github.com:open-telemetry/opentelemetry-js-con…

e1698e1

…trib

- update link for semconv

e076936

- rename the attribute name

92c1c80

blumamir requested changes Feb 22, 2025

View reviewed changes

sudarshan12s added 3 commits February 24, 2025 11:30

Merge branch 'main' of github.com:open-telemetry/opentelemetry-js-con…

8ceff01

…trib

- Incorporate review comments

30a2367

Merge branch 'main' of github.com:open-telemetry/opentelemetry-js-con…

2b310c5

…trib

sudarshan12s commented Feb 26, 2025

View reviewed changes

This was referenced Mar 3, 2025

Getting debug information and trace bind values oracle/node-oracledb#753

Open

Logging oracle/node-oracledb#839

Open

sudarshan12s added 3 commits March 4, 2025 12:52

Merge branch 'main' of github.com:open-telemetry/opentelemetry-js-con…

fa0463b

…trib

- get the baseclass, TraceHandlerBase from the parameter,

34afcad

moduleExports passed from patch function

- Remove copyright from source , add it in component_owners.yml

a06b40f

sudarshan12s requested a review from blumamir March 5, 2025 15:11

sudarshan12s mentioned this pull request Mar 6, 2025

Add Semantic definitions for Oracle Database open-telemetry/semantic-conventions#1911

Open

3 tasks

sudarshan12s added 2 commits March 6, 2025 21:48

- update dependencies as with other instrumentations

77a08a7

- retain copyrights to oracledb specific source files.

c7fcb17

removed copyright from component_owners.yml

blumamir approved these changes Mar 17, 2025

View reviewed changes

sudarshan12s added 8 commits March 17, 2025 22:38

Merge branch 'main' of github.com:open-telemetry/opentelemetry-js-con…

5faff20

…trib

- incorporate review comments

d25ee8e

Merge branch 'main' of github.com:open-telemetry/opentelemetry-js-con…

604f833

…trib

- update dependency versions. Test is changed to

0704e73

use new propery, instrumentationScope and replaced addSpanProcessor with new syntax of BasicTracerProvider constructor.

- update generated dependencies for oracledb and minor change in test

c8e0be6

- Add knip to ignore generated version file.

5c98035

Merge branch 'main' of github.com:open-telemetry/opentelemetry-js-con…

2296e18

…trib

Merge branch 'main' of github.com:open-telemetry/opentelemetry-js-con…

7cccc2e

…trib

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(oracledb): Add support for Oracle DB instrumentation #2612

feat(oracledb): Add support for Oracle DB instrumentation #2612

sudarshan12s commented Dec 20, 2024 •

edited

Loading

linux-foundation-easycla bot commented Dec 20, 2024 •

edited

Loading

blumamir left a comment

blumamir Feb 21, 2025

sudarshan12s Feb 26, 2025

blumamir Feb 21, 2025

sudarshan12s Feb 26, 2025

blumamir Feb 22, 2025

blumamir Feb 22, 2025

sudarshan12s Feb 27, 2025

blumamir Feb 22, 2025

sudarshan12s Feb 26, 2025

blumamir Feb 22, 2025

sudarshan12s Feb 26, 2025

blumamir Feb 22, 2025

sudarshan12s Feb 26, 2025

blumamir commented Feb 22, 2025

sudarshan12s Feb 26, 2025

sudarshan12s Feb 26, 2025

sudarshan12s Feb 26, 2025

sudarshan12s Feb 26, 2025

sudarshan12s Feb 26, 2025

sudarshan12s Feb 26, 2025

raphael-theriault-swi Mar 4, 2025

sudarshan12s Mar 5, 2025

sudarshan12s commented Feb 26, 2025

sudarshan12s commented Mar 5, 2025

dyladan commented Mar 5, 2025

blumamir left a comment

blumamir Mar 17, 2025

sudarshan12s Mar 17, 2025

-  if (traceHandlerBase) {
+  if (!traceHandlerBase) {
+    return undefined;
+  }
+  // else - all the code below, not nested and no indentation.

feat(oracledb): Add support for Oracle DB instrumentation #2612

Are you sure you want to change the base?

feat(oracledb): Add support for Oracle DB instrumentation #2612

Conversation

sudarshan12s commented Dec 20, 2024 • edited Loading

Which problem is this PR solving?

Short description of the changes

linux-foundation-easycla bot commented Dec 20, 2024 • edited Loading

blumamir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blumamir commented Feb 22, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sudarshan12s commented Feb 26, 2025

sudarshan12s commented Mar 5, 2025

dyladan commented Mar 5, 2025

blumamir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sudarshan12s commented Dec 20, 2024 •

edited

Loading

linux-foundation-easycla bot commented Dec 20, 2024 •

edited

Loading