-
Notifications
You must be signed in to change notification settings - Fork 439
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
contrib/gocql/gocql: implement observer api based tracing #2805
Conversation
BenchmarksBenchmark execution time: 2024-08-12 10:12:32 Comparing candidate commit c6c0385 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 58 metrics, 1 unstable metrics. |
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 is great IMO, much better than the previous API. Left a few comments in the review.
contrib/gocql/gocql/example_test.go
Outdated
cluster.Keyspace = "my-keyspace" | ||
|
||
// Create a new traced session using any number of options | ||
session, err := gocqltrace.NewTracedSession(cluster, gocqltrace.WithServiceName("ServiceName")) |
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.
^ my understanding is that this specific call will NOT trace anything.
[EDIT] me wrong, indeed by default this is true, forget about this. Maybe only comment somewhere that the default is "it's traced".
session, err := gocqltrace.NewTracedSession(cluster, gocqltrace.WithServiceName("ServiceName")) | |
session, err := gocqltrace.NewTracedSession(cluster, gocqltrace.WithServiceName("ServiceName"), gocqltrace.WithTraceQuery(true), gocqltrace.WithTraceBatch(true), gocqltrace.WithTraceConnect(true)) |
as we have to explicitly pass WithTrace<Something>
to activate tracing (but I may be wrong, just asking)
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.
Thanks for addressing all my remarks, looks good!
What does this PR do?
CreateTracedSession
, andNewObserver
functions that make use of the gocql observer apis: (it only implements the query, batch and connect observers for now).WithTraceQuery
,WithTraceBatch
andWithTraceConnect
to enable/disable these observers individually (all are enabled by default).Differences with the existing tracing method:
cassandra.paginated
andcassandra.consistency_level
are not set, since the required information is not available in the observer api.out.host
to the actual host name instead of the cassandra host ID. The host ID is set to a new tag calleddb.cassandra.host.id
.DD_TRACE_GOCQL_COMPAT
env variable introduced in [contrib/gocql] reporting both cluster and datacenter #2577 and instead directly set the correct values for thecassandra.cluster
andcassandra.datacenter
tags.cassandra.row_count
is a number value instead of a string.Motivation
Using the observer apis makes this instrumentation easier to use and maintain. It also allows to use the native gocql types instead of our custom wrapper types.
Reviewer's Checklist
Unsure? Have a question? Request a review!