opentelemetry-js-contrib icon indicating copy to clipboard operation
opentelemetry-js-contrib copied to clipboard

feat(oracledb): Add support for Oracle DB instrumentation

Open sudarshan12s opened this issue 11 months ago • 15 comments

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

sudarshan12s avatar Dec 20 '24 17:12 sudarshan12s

CLA Signed

The committers listed above are authorized under a signed CLA.

  • :white_check_mark: login: sudarshan12s / name: Sudarshan Soma (3cf1bd23deb8a69f71cba46aab8b1d8a15d31206, 77a08a7871a5d835d118b1d293700778257bd72b, 2b310c5d1b47a075187702d0727c5b513abf9dee, ae1b7ac198fd13f2583774cfa0667ad480938c2f, 6219bc425394b1c3244ead2cb334eed844ba48ad, b3bc7eb25120c279baff512de2c8256d27162ce3, 440db8346379a7491e77832b31408908c7b7c943, f670ac0833165dd9f86488981ab14a5478cc60d1, f8ff501446a0a7ab74821369ec405ae81ddcb108, 7cccc2eb961466f971a9f877dc24f1f6f78d237b, 7f00bf6572aa174583124e79cef45e23d93d6b7a, 382680cf67068c3903689b9b29cfddb13c768c2c, 4c363129344d302519eb7f2f71117afda1f66703, 3774f0d83625fc24a2ac94433239037b62b9b9e6, 01b2f84c83238ea72172ad85c19e037a9d7c5a0d, c8e0be6f4e8d0bb73208bddf591fe94dba3c7490, 30a2367de5da8accf917af3f2af5517b6e327c7c, 474b6e36f61ff0e656c587f845494a83f198b3bf, 91d65e4c4ca7effb8a750c20e13685c12df17c64, 6faff54968b136de145e7a155562440e08ab143c, 34afcad1927cca1f465f5029cf9fc65d0cbbdf75, d25ee8e6db2fcd739c0e91c3cdd93b50011e9440, 13e79dbe0bc383ee4ed7d997fc855542a1651631, a06b40f94faba199fcd62a810b8fe43fc6b5f122, 5c980350142020f372be8f191a31eac38ea96b71, 759ae136e1bfc9ff7169b2d6f4e9370b8f2a7a15, bf73ff54fc7c9ef8052cc89c1e306a2f26ba346e, f41f62c0e5daae5b99f062085ce788f4294dfaa1, 604f8333b5f1647913c867cf53a24a0157ede83f, e19e1ea5f3d134ba0e40291d55f2deae2e35cd55, e076936acab62a05e4c48168e69ba709362a8ec6, a0367cc6dfca6aa92d6718690e2614600dc2003d, 34cd4180500bd7302c3045fcd3f93543e3b4c278, 90db2323918f986c46726af711fa8755459d36f2, 07c76fdc575f663f38cae8afcf7c3eae4c6c1d3c, 0290b81038e25bbf47674db2b0c7080d3abbf652, 92c1c803ef0425c90b7ac9fb7e90466bcfed7f05, 34ef02df7749f2494d09413b7ead525b91d71ccf, 365d5238aa46a359e21b94f24e172e269a4ff2cd, c7fcb17c0dbc3c6843cbcd44a03f388653adb9f6, fa0463b86df321719a3988eaffb4275749e462bb, 3ae4d724b6cafbfda87114f39e9d14a17bd1649e, 2296e1804ea5c0439f4b82a96ad92db0a2ddd2df, 5defed847c8c67e8cff321a10d6b2c6cfadb3f1f, e1698e122eb541233515e4040e13e2762dd04cef, 5faff20e4c65774c148af2ec0340d0e1ad60ca26, 5c4f37f434b514f850ed517884d07d928b21e2b1, f98c7a4858b226d3e52c4052d44b5ef22b72bad8, 7e26da939510b296faed2e41d23181a3a768c0ce, badc18a7c2376e9848123bf817594b10148c1f83, 988eab26877539259ff6c1809a8e12bb7e739dcb, d155fb8b6d96792c3e3feed145e47f0fd8902396, 91136ced1d00f4984a6f22136cb71b7367696476, 8ceff01e058023bdec46b87e86b87b1587e590c4, 5117f31630c46b1a18d75c9e685868fb47ff449e, 3c600feffa3ecec367bfecd85cf6a4a7f9feed7d, 0e256522a404cfca6fbc8a820885ed225ec33aa3, 0704e7391a4b4c575e029d02b4d23df861140029, 4a154e51d67f1fd66071eb5652a520140f17b283, c299caa0ed4ea9126fdb2d7f3cf6b6d041ad0f7b, c3a8da99da17962e55feff2cc3c10894d729c674, 93d21e1339f4f232d0b9762c663482fc6aab04b2, f4b43e9c7dd77f5c079e1e6ae8ad994de27a78d2, cbe5bd1165db3bfd975a4f359731d62f72c400b4, 12f6b2b39bc36d706b99a7e37b442a5c5b722dec, be90e6baa1dc5a6fde95c40a8de5391f36158cb4, 23d29235bcf99e9c95b02daa771c6d71f9509c21, 8ce3e882e1f6b769f4a30949267d7909f418efbc, f018f0f258b8d8518cd86792b3e28172261a4d05, 1295e539d787e82399eba240c1bc0f4b00995e40, 966e487c248b9d101d8e0ecc736aaacca5006421, 76b3f21d25e46f2ab4f8bf845cb04985cda87317, 9a0f07ae82c2e20d4d35286edc02f97807b0550e)

Could you rename the new types to TitleCase to follow the rest of the code ?

Yes renamed to TitleCase for types and internal-types exported.

sudarshan12s avatar Jan 07 '25 06:01 sudarshan12s

Codecov Report

Attention: Patch coverage is 96.15385% with 8 lines in your changes missing coverage. Please review.

Project coverage is 89.66%. Comparing base (10da4db) to head (cbe5bd1). Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...tation-oracledb/src/OracleTelemetryTraceHandler.ts 96.75% 5 Missing :warning:
...ry-instrumentation-oracledb/src/instrumentation.ts 90.00% 3 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2612      +/-   ##
==========================================
+ Coverage   89.46%   89.66%   +0.20%     
==========================================
  Files         180      184       +4     
  Lines        8735     8943     +208     
  Branches     1778     1834      +56     
==========================================
+ Hits         7815     8019     +204     
- Misses        920      924       +4     
Files with missing lines Coverage Δ
...tapackages/auto-instrumentations-node/src/utils.ts 98.26% <100.00%> (+0.01%) :arrow_up:
...elemetry-instrumentation-oracledb/src/constants.ts 100.00% <100.00%> (ø)
...ntelemetry-instrumentation-oracledb/src/semconv.ts 100.00% <100.00%> (ø)
...ry-instrumentation-oracledb/src/instrumentation.ts 90.00% <90.00%> (ø)
...tation-oracledb/src/OracleTelemetryTraceHandler.ts 96.75% <96.75%> (ø)

... and 1 file with indirect coverage changes

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar Jan 07 '25 16:01 codecov[bot]

The earlier triggered CI failure seems to be this. It looks related to some package version dependency , package-lock.json updates and not the changes in PR. Please suggest if there are some changes needed from my side.


 Exception during run: /home/runner/work/opentelemetry-js-contrib/opentelemetry-js-contrib/node_modules/@aws-sdk/client-sqs/dist-cjs/index.js:185
  static {
         ^

SyntaxError: Unexpected token '{'

sudarshan12s avatar Jan 08 '25 06:01 sudarshan12s

Please help me with steps for updating package-lock.json . This PR lock file package-lock.json differs from main and i am not able to keep it in sync easily..

sudarshan12s avatar Jan 15 '25 05:01 sudarshan12s

Please help me with steps for updating package-lock.json . This PR lock file package-lock.json differs from main and i am not able to keep it in sync easily..

I have fixed this discrepancy.

sudarshan12s avatar Jan 28 '25 06:01 sudarshan12s

@sudarshan12s as we discussed during SIG meeting yesterday, please review the requirements here for component ownership, as well as our preferred approach for long-term maintenance of instrumentation libraries.

JamieDanielson avatar Feb 06 '25 16:02 JamieDanielson

@sudarshan12s as we discussed during SIG meeting yesterday, please review the requirements here for component ownership, as well as our preferred approach for long-term maintenance of instrumentation libraries.

To @JamieDanielson Cc @open-telemetry/javascript-maintainers

Hi Jamie, All,

To become a member of CNCF, @sudarshan12s will need two sponsors for his application.

I am myself an Oracle employee, one of the maintainers of the opentelemetry-cpp repository, and can sponsor Sudarshan.

Could you, or someone in javascript-maintainers, be the second sponsor ? I think it makes the most sense, since Sudarshan contribution(s) are in the js-contrib space.

Regards, -- Marc

marcalff avatar Feb 10 '25 14:02 marcalff

Hi @sudarshan12s. I was absent during last week's SIG meeting so I'm not sure if there was an agreement made to not apply the current guidelines, but there are rules and guidelines for new instrumentations. These were introduced since many instrumentations were abandoned in this repository and we would like to avoid this in the future as it diverts resources from development of OTel core components.

Before opening a PR, the following questions should be answered, see Jamie's link:

  • who's going to maintain it (2 people minimum, both must be members of the OTel org prior to opening the PR)
  • why not host it in a separate repo? (hosting it here has maintenance implications for the repo, the less we have to host here, the better the quality of other components like the API and SDK is going to be as we get to spend more time on that)

This is done in the form of an instrumentation request.

pichlermarc avatar Feb 12 '25 13:02 pichlermarc

Hi @sudarshan12s. I was absent during last week's SIG meeting so I'm not sure if there was an agreement made to not apply the current guidelines, but there are rules and guidelines for new instrumentations. These were introduced since many instrumentations were abandoned in this repository and we would like to avoid this in the future as it diverts resources from development of OTel core components.

Before opening a PR, the following questions should be answered, see Jamie's link:

  • who's going to maintain it (2 people minimum, both must be members of the OTel org prior to opening the PR)
  • why not host it in a separate repo? (hosting it here has maintenance implications for the repo, the less we have to host here, the better the quality of other components like the API and SDK is going to be as we get to spend more time on that)

This is done in the form of an instrumentation request.

Thanks @pichlermarc , Yes we were informed on the guidelines/procedure to be followed in the last SIG. I have updated the instrumentation request here with more details. Let me know if this looks fine. For becoming members of OTel org, @marcalff has agreed for first sponsor. Can someone from javascript-maintainers be the second sponsor.

sudarshan12s avatar Feb 12 '25 15:02 sudarshan12s

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

blumamir avatar Feb 22 '25 09:02 blumamir

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.

sudarshan12s avatar Feb 26 '25 16:02 sudarshan12s

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 avatar Mar 05 '25 15:03 sudarshan12s

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

dyladan avatar Mar 05 '25 17:03 dyladan

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.

sudarshan12s avatar Mar 20 '25 17:03 sudarshan12s

Thank you for your work on this! I added a few comments

I have incorporated the comments.. Please see.

sudarshan12s avatar Apr 16 '25 14:04 sudarshan12s

Thanks @maryliag and @blumamir for your review and thanks @marcalff for the sponsor support! @sudarshan12s Nice work!

sharadraju avatar Apr 24 '25 14:04 sharadraju

Great work on this!

Thank you so much...

sudarshan12s avatar Apr 24 '25 14:04 sudarshan12s

It looks like the failed test may be flaky (intermittently asserting different connection counts). Would it be possible to either ignore this failure for this PR, or re-run the CI?


MongoDBInstrumentation-Metrics
       Should add connection usage metrics:

      AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:

2 !== 1

      + expected - actual

      -2
      +1
      
      at Context.<anonymous> (test/mongodb-v4-v5-v6.metrics.test.ts:127:12)


Thanks for your time!

sudarshan12s avatar Apr 29 '25 03:04 sudarshan12s

It looks like the failed test may be flaky (intermittently asserting different connection counts). Would it be possible to either ignore this failure for this PR, or re-run the CI?


MongoDBInstrumentation-Metrics
       Should add connection usage metrics:

      AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:

2 !== 1

      + expected - actual

      -2
      +1
      
      at Context.<anonymous> (test/mongodb-v4-v5-v6.metrics.test.ts:127:12)

Thanks for your time!

@onurtemizkan, I believe this is a flaky error and can be safely ignored.

sudarshan12s avatar May 05 '25 17:05 sudarshan12s

Thanks @blumamir for merging this PR.

sharadraju avatar May 07 '25 09:05 sharadraju