-
Notifications
You must be signed in to change notification settings - Fork 301
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 integration for ms sql server with newrelic #425
Added integration for ms sql server with newrelic #425
Conversation
Added intergation for ms sql server
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGFM 👍
Thanks, @ishahid91 , for this PR! We'll be looking at it and considering it for inclusion in the Go Agent. It looks quite straightforward, and we're hoping to test it against a real MS SQL server soon! Note that development generally slows down at the end of the calendar year, so our team most likely won't be looking closely at it until a few weeks have passed. |
Thanks @RichVanderwal for looking at the PR and considering it to include in Go Agent. I am looking forward to get it included in Go Agent ASAP as this will help us and many other teams to monitor MS SQL Server for many critical Go services. |
Hi @RichVanderwal, Any update on the review ? Looking forward to get it merged so that we can integrate it into our production services. |
@RichVanderwal gentle reminder. |
@ishahid91 Hi, are you still interested in merging this? We will help you :) |
Yes @iamemilio Can you please help merge this PR. |
Overall, it looks good. Could you remove the dependency on ms-sql in the v3/go.mod file and verify that the changes still work? |
Hi @iamemilio I have made the requested changes |
Looks good. I will try to run this just to make sure everything checks out. Once the test is happy, and I run it, I think this will be good to go |
@ishahid91 I was able to run this. Once you clean up the go lint errors, this is ready to merge: https://github.com/newrelic/go-agent/runs/6757623643?check_suite_focus=true Thanks for submitting this, and thank you for being patient with us while we continue to grow our team! |
@iamemilio fixed the linting issue, can you please run once again to verify. Thank you for your time and making it ready to merge :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks!
@iamemilio Thanks for approving the PR. When I can expect this PR to be merged ? |
It will very likely be merged in time for the next release! |
Added intergation for ms sql server
Links
Details
Added integration for ms sql server to show database transactions on new relic. I have used the similar approach which is used in other integrations. I have added the relevant unit test and example as well which I tested on my local with sql server.