add support for node-cassandra-cql driver

[asilvas]

Verified working, but no unit/functional tests have been created at this time. It'll be some time before I can invest in the tests, so was hoping to get this in sooner than later.

[wraithan]

Wow, this is pretty great. Cassandra wasn't on our near future roadmap, but I'll bubble this up so one of us engineers can get some time to look this over and write tests for it. We don't allow features into the code base that don't have tests.

[asilvas]

fyi it's been used in production to log hundreds of millions of hits with no issue.

[txase]

@asilvas Thanks again for providing this PR. We definitely want to include it in our agent.

As @wraithan noted, we only include tested stuff into our agent. If you can provide tests, we would gladly take them. If not, we can write them as well. We can't commit to a timeline, but I can tell you that it will be at least a few weeks before we may be able to tackle writing tests.

Thanks again for your effort, and for your understanding of our timelines!

[asilvas]

Understood, was just giving an update. Thanks guys

[wraithan]

@asilvas We've merged your first commit. We wrote tests around it and found that there were cases where the connection pooling was causing queries to bind to the wrong segment and/or transaction. We've changed how the instrumentation works and this will be released tomorrow afternoon. I'm going to close out this PR without the changes that you've added in the last day or so as they wont cleanly apply to the new code.

If you'd like to continue iterating on this, I encourage you to wait until we've pushed tomorrow and get everything working on top of the new instrumentation code.

Also, would the following be fine with you in our release notes: "Thanks to Aaron Silvas from Go Daddy for the initial implementation of the cassandra instrumentation." We can also omit this if you don't feel comfortable with it, or change the wording if you'd like.

[asilvas]

Sounds good thanks for all your efforts. I'll work on getting statement support added after your update so we can drop our internal version. The kudos would be great, thanks

[wraithan]

@asilvas Howdy, so we cut a release yesterday. So if you pull the source you should see our changes to the cassandra code. You'll noticed that we opted for binding at the client layer instead of the connection layer. This allows us to ignore some of the issues that come up when you are instrumenting things like connection pools.

When we bind earlier (at the client layer) we can assure the right transaction/segment is in play, then when the callback is called things within it can also bind to that transaction/segment. We were finding in some cases, the connection pooling was causing calls within the callbacks passed to the query to bind to the wrong segment. We didn't run anything concurrent enough to make it bind to the wrong transaction but I could definitely imagine that happening with live traffic.

As long as you are using the module as documented (from the client interface) you are fine. If you are reaching in, directly using connection objects, and bypassing the connection pooling this instrumentation wont work for you. If that is the case, let us know and we can iterate on it and see if it is viable to also support that usage case.

[asilvas]

Pretty sure my original implementation (prior to PR) bound to the Client prototype, but it wasn't working for some reason. Perhaps something I was doing wrong at the time. I'll verify it "soon" and hopefully re-integrate the prepared statement tracking (hugely useful). Thanks!

[wraithan]

If you find a bug, totally let us know. The connection stuff looked fine until one deep inspected the transactions and found that things were not nesting correctly. Our tests (which exercise this quite a bit and deep inspect transaction traces) show that binding to the client works, but with how complex this stuff is, it is possible that there may be subtle bugs.