-
Notifications
You must be signed in to change notification settings - Fork 419
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
Raphael/gocql #73
Raphael/gocql #73
Conversation
tracer/contrib/gocql/gocqltrace.go
Outdated
|
||
// TraceQuery wraps a gocql.Query into a TracedQuery | ||
func TraceQuery(service string, tracer *tracer.Tracer, q *gocql.Query) *TracedQuery { | ||
string_query := strings.SplitN(q.String(), "\"", 3)[1] |
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.
No snake_case !!! 😠
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.
😨
service string | ||
keyspace string | ||
paginated string | ||
consistancy string |
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.
consistency
tracer/contrib/gocql/gocqltrace.go
Outdated
|
||
// TraceQuery wraps a gocql.Query into a TracedQuery | ||
func TraceQuery(service string, tracer *tracer.Tracer, q *gocql.Query) *TracedQuery { | ||
string_query := strings.SplitN(q.String(), "\"", 3)[1] |
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.
what is this split doing exactly? seems very opaque. Can you add a test describing the behavior?
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.
q.String() returns for a query: (exemple) [query statement="SELECT * from trace.person" values=[] consistency=QUORUM]
So in between the two first " there's the query
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.
This doesn't work - I'm getting "Non-parsable SQL query" - add a test
tracer/contrib/gocql/gocqltrace.go
Outdated
// TraceQuery wraps a gocql.Query into a TracedQuery | ||
func TraceQuery(service string, tracer *tracer.Tracer, q *gocql.Query) *TracedQuery { | ||
string_query := strings.SplitN(q.String(), "\"", 3)[1] | ||
q.NoSkipMetadata() |
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.
what does NoSkipMetadata do?
We've been using this version prod for a couple of days now. Looking fine. |
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.
LGTM
This contains two PRs: - #73 branch raphael/gocql (Cassandra support) - #78 branch christian/flushchannel (fix partial traces bug) Those 2 branches had conflicts, and the commit 6952e85 fixes the interactions between them. Additionnally, it has been running for 3 complete days on our prod env, so considered stable enough.
Gocql query wrapper
Implementation:
Data we get 100% time:
query, errors, row count, pagestate
Data we only get when the query returns columns:
keyspace
Data we will get once this is merged gocql/gocql#918 :
host, port, clustername