Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 54 commits into
base: main
Choose a base branch
from

Conversation

sudarshan12s
Copy link

@sudarshan12s sudarshan12s commented Dec 20, 2024

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
@sudarshan12s sudarshan12s requested a review from a team as a code owner December 20, 2024 17:52
Copy link

linux-foundation-easycla bot commented Dec 20, 2024

@sudarshan12s
Copy link
Author

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

  1. the copyright to oracle should be addressed (not sure in what way) @open-telemetry/javascript-maintainers
  2. 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

@sudarshan12s sudarshan12s requested a review from blumamir March 5, 2025 15:11
@dyladan
Copy link
Member

dyladan commented Mar 5, 2025

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

Copy link
Member

@blumamir blumamir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

obj: typeof oracleDBTypes
): any {
const traceHandlerBase = getTraceHandlerBaseClass(obj);
if (traceHandlerBase) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@sudarshan12s
Copy link
Author

Hey @opentelemetry/javascript-approvers, I've incorporated the feedback received so far. If anyone else would like to take a look, I'd appreciate additional inputs!

Thanks.


// Oracle specific attributes not covered by standard semantic conventions.
// See: https://github.com/open-telemetry/semantic-conventions/pull/1911
export enum AttributeNames {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this PR should be merged until the Semantic Conventions PR for it is merged, since the definition can change there and you don't want to have something merged that is already out of date even before it gets released. And on that topic, why these attributes were not added to the semantic convention?
I imagine the goal is to have a similar instrumentation for oracle in the other SDKs, but how will Java or .NET know what names to use if they're not defined in the Semantic convention. They will have no idea that it was already implemented in JS and they should follow here. This is why the Semantic Conventions are so important. So please add a complete definition there with all attributes you're using in this instrumentation 😄

Also, as I mentioned in the SIG, really try to use already existing name attributes, for example, we have several attributes for pool already, so there is no need to create specific with oracle in the name.
You should check if we don't have for the others.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @maryliag! I've started making the changes based on the inputs suggested in the semantic conventions PR.

I’ll use the existing attributes and retain only one entry in constants.ts, as it is specific to Oracle DRCP:
ORACLE_IMPLICIT_RELEASE = 'oracle.db.implicit_release',

For pool attributes, I plan to map them as follows:

oracledb.pool.max -> db.client.connection.max  
oracledb.pool.min -> db.client.connection.min  

I also requested clarification on whether db.client.connection.max and db.client.connection.min can be used in DB spans when the oracledb.createPool API is called, in addition to being included in metrics. They will not be dumped in subsequent DB queries.

For now, I’ve removed oracledb.pool_incr (I have not seen in existing attributes) but can add it back to the semantic conventions and instrumentation once I receive clarification on the above.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have incorporated the changes based on the semantic conventions PR feedback received so far. The attribute oracle.db.implicit_release is still under review.

Copy link
Author

@sudarshan12s sudarshan12s Apr 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The semantic conventions PR has been merged. I've updated the implementation to incorporate these conventions—please take a look.

Below is an example showing span attributes aligned with the conventions:

For a SQL query like 'select :name from dual', where :name is a placeholder, the following attributes would be recorded:

spanName-> oracledb.Connection.execute:SELECT FREE|FREEPDB1|freepdb1 (It has Fnname:operationName db.namespace)
db.operation.name ='SELECT'
db.operation.parameter.name ='0'
db.query.text = 'select :name from dual'
db.system.name ='oracle.db'
db.namespace = 'FREE|FREEPDB1|freepdb1'. (It has instanceName | pdbname | serviceName)

@sudarshan12s sudarshan12s requested a review from maryliag April 9, 2025 05:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment