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
Closed

[WIP] Instrument tedious MSSQL driver #277

wants to merge 16 commits into from

Conversation

alexashley
Copy link

@alexashley alexashley 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 alexashley marked this pull request as ready for review June 4, 2019 01:49
@astormnewrelic
Copy link
Contributor

@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
Copy link
Author

@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)?

bizob2828 pushed a commit to bizob2828/node-newrelic that referenced this pull request Apr 19, 2024
…c#277)

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
bizob2828 pushed a commit to bizob2828/node-newrelic that referenced this pull request Apr 23, 2024
…c#277)

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
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