-
Notifications
You must be signed in to change notification settings - Fork 211
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
initial cassandra instrumentation #205
Conversation
c6485eb
to
8e8ea7e
Compare
it looks great! did you try re. cql is smaller than sql, i can't think of more interesting cases than you have. is great you have a test for |
8e8ea7e
to
42269a1
Compare
@jalvz just added a test for As for instrumenting Once we get around to properly support async frameworks, we should have another look. |
42269a1
to
18527e7
Compare
18527e7
to
717d858
Compare
b310a30
to
72dacfa
Compare
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+.
72dacfa
to
a6e166a
Compare
@@ -119,3 +127,5 @@ volumes: | |||
driver: local | |||
pyesdata2: | |||
driver: local | |||
cassandradata3: |
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.
@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?
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.
Good point! Feel free to open a PR :)
This instruments
Cluster.connect
andSession.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:
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.