Conversation
- 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
IanMcCurdy
left a comment
IanMcCurdy
left a comment
There was a problem hiding this comment.
Nice work, this looks great. There are just a few very small changes to make, then it's good to go
test/acceptance/db.Dynatrace.js
Outdated
| async.waterfall([prepare, testExecStatement(['1']), testExecStatement(['2']), dropStatement], done); | ||
| }); | ||
|
|
||
| it('should trace a client execute', function (done) { |
There was a problem hiding this comment.
This could use a more descriptive string. Right now it's identical to a previous test.
extension/Dynatrace.js
Outdated
| function _prepareStmtUsingDynatrace(conn, prepareFn) { | ||
| // args = [sql, options, callback] --> options is optional | ||
| return function (...args) { | ||
| const cb = args[args.length - 1]; |
There was a problem hiding this comment.
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
extension/Dynatrace.js
Outdated
| return conn; | ||
| } | ||
| conn._dbInfo = dbInfo; | ||
| // hana-client does not like decorating. |
There was a problem hiding this comment.
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
he-is-harry
changed the title
|
@he-is-harry while it is still officially supported, I think Dynatrace is moving away from maintaining their own sdk. The last code update was over 3 years ago. Would it make sense for you to use |
|
@IanMcCurdy is there anything fundamental holding this back from merging? As I understand it, the "fat" @sap/hana-client does have dynatrace support, so it would be nice to get feature parity on this. |
3 participants
Testing Notes
The Makefile was modified so that
make test-dynatracewill test only the dynatrace integration testsTo 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