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

Added Dynatrace support (WORK IN PROGRESS) #258

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

he-is-harry
Copy link
Contributor

  • Added extension/Dynatrace.js to wrap exec / execute / prepare functions with dynatrace tracing
    • Modified the Client to automatically wrap with dynatrace tracing when isDynatraceEnabled() is true (@dynatrace/oneagent-sdk is installed and HDB_NODEJS_SKIP_DYNATRACE is off) and the dynatrace connect option is true
  • Added the dynatrace and dynatraceTenant connect options
  • Modified ResultSet.js to have the getRowCount function
    • Added 2 unit tests for getRowCount
  • Added the isDynatraceSupported field to the driver (Hana class)
  • Added the MockDynatraceSDK for testing and added integration tests for dynatrace

Testing Notes
The Makefile was modified so that make test-dynatrace will test only the dynatrace integration tests

To install the MockDynatraceSDK, copy the contents of the MockDynatraceSDK folder into node_modules/@dynatrace/oneagent-sdk

The integration tests were designed to be able to dynamically run on the mock dynatrace and the real dynatrace depending on the node_modules import

- Added extension/Dynatrace.js to wrap exec / execute / prepare functions with dynatrace tracing
    - Modified the Client to automatically wrap with dynatrace tracing when isDynatraceEnabled()
      is true (@dynatrace/oneagent-sdk is installed and HDB_NODEJS_SKIP_DYNATRACE is off) and
      the dynatrace connect option is true
- Added the dynatrace and dynatraceTenant connect options
- Modified ResultSet.js to have the getRowCount function
    - Added 2 unit tests for getRowCount
- Added the isDynatraceSupported field to the driver (Hana class)
- Added the MockDynatraceSDK for testing and added integration tests for dynatrace

Testing Notes
The Makefile was modified so that `make test-dynatrace` will test only the dynatrace integration tests

To install the MockDynatraceSDK, copy the contents of the MockDynatraceSDK folder into
node_modules/@dynatrace/oneagent-sdk

The integration tests were designed to be able to dynamically run on the mock dynatrace and the real
dynatrace depending on the node_modules import
- Updated the create table hook to allow the tests to be run without a config.json
- Modified Dynatrace.js result set callback to allow array results
  to indicate the rows returned not just result sets like before
- Added an integration test to check that an insert with execute
  would trace the number of rows affected
Copy link
Collaborator

@IanMcCurdy IanMcCurdy left a comment

Choose a reason for hiding this comment

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

Nice work, this looks great. There are just a few very small changes to make, then it's good to go

async.waterfall([prepare, testExecStatement(['1']), testExecStatement(['2']), dropStatement], done);
});

it('should trace a client execute', function (done) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could use a more descriptive string. Right now it's identical to a previous test.

function _prepareStmtUsingDynatrace(conn, prepareFn) {
// args = [sql, options, callback] --> options is optional
return function (...args) {
const cb = args[args.length - 1];
Copy link
Collaborator

Choose a reason for hiding this comment

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

There should be an args.length > 0 check here, similar to _ExecuteWrapperFn. This also applies to hana-client and Ian McH will fix it there

return conn;
}
conn._dbInfo = dbInfo;
// hana-client does not like decorating.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment isn't relevant here, should just remove it

- Added an index check to ensure that the callback when preparing statements for
dynatrace exists and is a function
- Improved some test naming to better indicate client execute tests vs. client
exec and statement execute tests
- Added a comment to indicate when numbers are returned for rows affected
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants