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

[WIP] Instrument tedious MSSQL driver #277

Closed
wants to merge 16 commits into from

Conversation

@alexashley
Copy link

commented Jun 3, 2019

This PR uses the DatastoreShim API to instrument the Node.js MSSQL driver tedious.
As a reference, I used this closed PR #211 along with tedious-newrelic.

It includes integration tests for select, insert, update, and delete, using the MSSQL Linux Docker image.

I opened the PR as WIP in order to get feedback on the implementation and tests before adding the following:

  • integration tests for:
    • stored procedures
    • transactions
    • bulk insert
    • sql obfuscation
    • operations like cancel/close/reset
  • unit tests

NOTES

alexashley added 9 commits May 31, 2019
@alexashley alexashley marked this pull request as ready for review Jun 4, 2019
alexashley added 7 commits Jun 4, 2019
@astormnewrelic

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2019

@alexashley First and formost -- thank you! This looks like a great start at getting tedious instrumented. Thank you for sharing it!

Second, (after a bit of discussion with the team and some other stakeholders), we're trying to move away from including every single DB package possible in the agent. There's lot of complicated reasons for this (some technical, some long-term-support, some "other"), but we're not sure we'd be able to accept your awesome instrumentation into the agent as a first party thing right now.

So -- next steps?

  1. Make a separate repo for your instrumentation
  2. Including instructions in the README for loading that instrumentation with newrelic.instrument
  3. Optionally -- publish it as a NPM package

That should make it available to folks who want to use it without all the red-tape of getting it into the agent proper.

I'm going to close this PR for now, but if you've got any questions about newrelic.instrument -- or anything really -- please don't hesitate to keep this conversation going.

@alexashley

This comment has been minimized.

Copy link
Author

commented Jun 26, 2019

@astormnewrelic I can definitely understand how it could be a maintenance burden to instrument a number of 3rd party packages. If the future direction of the agent is individual module instrumentations, would it be possible to create a separate package for some of the integration test utilities (ex: agent_helper and metrics_helper)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.