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

add oracle instrumentation #182

Closed
wants to merge 9 commits into from

Conversation

ryanwilliamquinn
Copy link

this is instrumentation based on the node-oracle module:
https://github.com/joeferner/node-oracle

Ryan Quinn added 3 commits October 29, 2014 14:09
doesnt cover connectSync, plus probably a bunch of stuff i dont know about
add tests - not implementing anything too useful,
pretty much copied what existing mysql instrumentation did.
@wraithan
Copy link
Contributor

wraithan commented Nov 4, 2014

@ryanwilliamquinn Wow! Oracle instrumentation! So we have a couple barriers to getting this merged. First and foremost will be getting it tested. This would mean getting tests that run supposing you had an oracle DB around. Second would be dealing with the fact that part of our integration tests wont be able to run in the wild as oracle requires a license and we can't just pull down a docker image of it. Finally is manually testing this and making sure there aren't any problems, this will last one is something I or @hayes will need to do.

If you can provide tests that proves this works supposing we have an oracle db around, that would get us to the next step of merging this. I'll contact our internal IT to see if I can use one of our oracle DBs for testing. Unfortunately this combination of things means it might take a couple days up to a week or two before we can merge this in.

Thank you a ton for building this though!

@wraithan
Copy link
Contributor

wraithan commented Nov 4, 2014

Oh, also regarding the reader API. You can instrument it using a segment. Then on each read from it, you can do a segment.touch() which will update the timer to say it ended at touch() time if you don't explicitly end() it before the transaction gets harvested.

@wraithan
Copy link
Contributor

wraithan commented Nov 5, 2014

Did some research. It appears I don't have ready access to an oracle DB at work. I'll see if they have a dev only license that I can use to test this. That requires us setting up the DB ourselves and getting it working, meaning larger overhead. I can't really make any promises on when we'll be able to actually evaluate this code unfortunately.

@ryanwilliamquinn
Copy link
Author

Thanks for taking the time to look at this. I wonder if we can run the (soon to be written) tests against oracle express? You might not need a license to work with that.

I will look into segment.touch for the reader, thanks for the tip.

@mrlannigan
Copy link

👍

Ryan Quinn added 6 commits November 10, 2014 10:04
release: published 1.13.2
Conflicts:
	lib/metrics/names.js
make oracle driver an optional dependency
check if oracle driver exists before running tests
@ryanwilliamquinn
Copy link
Author

I added integration tests, which I am running against oracle express. I installed oracle express via this tutorial (note that the oracle username and password in the tests is docker/docker):
http://meandmyubuntulinux.blogspot.com/2012/05/installing-oracle-11g-r2-express.html

Also, see here for instructions on installing the node oracle driver:
https://github.com/joeferner/node-oracle/blob/master/INSTALL.md

If the oracle driver isn't installed, the tests will just be ignored, so it shouldn't blow anything up.

Let me know if there is anything else I can do to make this easy for you to test, or if you see anything I should change.

* Since there is no reader.close method, it is unclear how to end the segment. It would make sense
* to end it when the next row is null, signifying that the cursor has iterated all rows.
* Other situations could occur, such as the client not iterating all of the rows,
* which would end up in the segment never ending.
Copy link
Contributor

Choose a reason for hiding this comment

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

we have had similar issues in our mongo instrumentation, I'll have to look into this a bit more, but it might be acceptable to have segments that are not guaranteed to have an end time, the concern would be leaking cursors, so this would only be possible if we can do it such a way that the segment can be gc'd when the client drops their ref to the cursor.

@hayes
Copy link
Contributor

hayes commented Nov 18, 2014

Hey, thanks for adding some tests, we really appreciate it. I have gotten them running with oracle in docker, and have refactored the tests a bit so they are closer to our current style. I still need to look a bit more closely at how the oracle driver works internally to make sure there are no edge cases I have not considered, but I want to try to get this merged soon.

There are still a couple of outstanding concerns I have about the install process. Unfortunately installing the oracle module is not very straight forward. I may look into moving it out of the dependency list and into a make step, since this will cause npm install to log all kinds of errors if your environment is not configured correctly even though its an optional dependency. My understanding of optional decencies is that they will always be attempted to be installed (but not complain if they fail if the module is being installed as a nested dependency). I may also want to add something to the final test output to indicate if tests were skipped due to deps not being installed, tests are not very valuable if they don't complain loudly when they aren't passing. (maybe just a mode that will force the tests to fail for use in CI)

@hayes
Copy link
Contributor

hayes commented Nov 21, 2014

hey, just a quick update on this, we did not have time to get this into this weeks release because we ran into some issue getting oracle working in our CI environment, we are planning to ship it as part of out release next week (probably on Wednesday). We like to credit contributors in our release notes, what would be your preferred way to be credited (github name, full name, etc)

@ryanwilliamquinn
Copy link
Author

Cool, thanks for getting this out so quickly. You can use my github name in the release notes, and thanks for doing that.

@hayes
Copy link
Contributor

hayes commented Nov 25, 2014

released as part on 1.14.0

@hayes hayes closed this Nov 25, 2014
jsumners-nr pushed a commit to jsumners-nr/node-newrelic that referenced this pull request Apr 16, 2024
bizob2828 added a commit to bizob2828/node-newrelic that referenced this pull request Apr 19, 2024
updated agent and test utils to latest
bizob2828 added a commit to bizob2828/node-newrelic that referenced this pull request Apr 23, 2024
updated agent and test utils to latest
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.

None yet

4 participants