Skip to content

initial cassandra instrumentation#205

Closed
beniwohli wants to merge 1 commit intoelastic:masterfrom
beniwohli:cassandra-instrumentation
Closed

initial cassandra instrumentation#205
beniwohli wants to merge 1 commit intoelastic:masterfrom
beniwohli:cassandra-instrumentation

Conversation

@beniwohli
Copy link
Contributor

@beniwohli beniwohli commented Apr 26, 2018 •
edited by alvarolobato
Loading

This instruments Cluster.connect and Session.execute.
The test matrix only includes version 3.4 and 3.14+, as earlier versions of the client don't work on Python 2.7.11+.

Open questions:

  • our SQL parser (used to get a significant span name from the query) seems to cope quite well with CQL. I'm not a huge Cassandra expert though, so it would be great if somebody with more expertise could contribute some interesting test cases (the more it doesn't look like SQL, the better).
  • there's also a commercial variant of Cassandra, called Datastax Enterprise. The parts of the driver that is interesting to us seems to be more or less identical to the open source cassandra driver, so theoretically we could just instrument it as well (by instrumenting the same methods in the dse namespace). However, we should probably test this. There is a docker image for DSE available, so we might be able to use that, if we deem it necessary to support DSE.

@beniwohli beniwohli force-pushed the cassandra-instrumentation branch from c6485eb to 8e8ea7e Compare April 26, 2018 19:33
@beniwohli beniwohli requested review from jalvz and roncohen April 27, 2018 07:42
@jalvz
Copy link
Contributor

jalvz commented Apr 30, 2018

it looks great!

did you try cassandra.concurrent?
i think instrumenting execute_async could be nice

re. cql is smaller than sql, i can't think of more interesting cases than you have. is great you have a test for CREATE KEYSPACE, maybe one for CREATE COLUMNFAMILY would make sense too?

@beniwohli beniwohli force-pushed the cassandra-instrumentation branch from 8e8ea7e to 42269a1 Compare May 1, 2018 08:09
@beniwohli
Copy link
Contributor Author

beniwohli commented May 1, 2018

@jalvz just added a test for COLUMNFAMILY

As for instrumenting execute_async, I had a look at it, but there's not really a way to do that properly right now, as we don't support async in general (e.g. we can't connect the span with the future that is returned by execute_async, and there is no way to know which transaction to attach it to once it's done).

Once we get around to properly support async frameworks, we should have another look.

@beniwohli beniwohli force-pushed the cassandra-instrumentation branch from 42269a1 to 18527e7 Compare May 1, 2018 12:21
@beniwohli beniwohli force-pushed the cassandra-instrumentation branch from 18527e7 to 717d858 Compare May 29, 2018 09:46
@beniwohli beniwohli mentioned this pull request May 29, 2018
Closed
@beniwohli beniwohli force-pushed the cassandra-instrumentation branch 2 times, most recently from b310a30 to 72dacfa Compare June 12, 2018 11:45
@beniwohli
initial cassandra instrumentation
a6e166a 
This instruments `Cluster.connect` and `Session.execute`.
The test matrix only includes version 3.4 and 3.14+, as earlier versions of the client don't work on Python 2.7.10+.
@beniwohli beniwohli force-pushed the cassandra-instrumentation branch from 72dacfa to a6e166a Compare June 12, 2018 12:08
@beniwohli beniwohli closed this in e071c7a Jun 12, 2018
watson
watson reviewed Jul 7, 2018
View reviewed changes
tests/docker-compose.yml
driver: local
pyesdata2:
driver: local
cassandradata3:
Copy link
Contributor

@watson watson Jul 7, 2018

Choose a reason for hiding this comment

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

@beniwohli I just noticed that all the other volumes is prefixed with py. I don't know exactly why, but my best guess is so it doesn't interferer with the other agent tests. If that's the case, this should probably be prefixed as well right?

Copy link
Contributor Author

@beniwohli beniwohli Jul 7, 2018

Choose a reason for hiding this comment

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

Good point! Feel free to open a PR :)

@beniwohli beniwohli deleted the cassandra-instrumentation branch July 7, 2018 08:37
@watson watson mentioned this pull request Jul 7, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@roncohen roncohen Awaiting requested review from roncohen

2 more reviewers

@watson watson watson left review comments

@jalvz jalvz jalvz approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@beniwohli @jalvz @watson @alvarolobato