CASSGO-39 Add exec attempt interceptor (round 3)#1943
CASSGO-39 Add exec attempt interceptor (round 3)#1943abenn135 wants to merge 10 commits intoapache:trunkfrom
Conversation
Removes return statement that bypassed query attempt tracking.
Remove gocql.NewIterWithErr
To facilitate chaining interceptors
Replace with read-only addr fields.
internalRequest is an intentionally package-private type. We cannot expose it directly in public API QueryAttempt, so instead share the statement. Unfortunately, since the statement invoked by internalRequest.execute()/conn.execute() is embedded in the internalRequest itself, it cannot be modified by an interceptor without significant refactoring. Therefore, interceptor exposes only a copy of the statement.
worryg0d
left a comment
There was a problem hiding this comment.
Hey,
I'm unsure if this PR is ready for review, but I just looked at it. I like the overall idea of introducing a generic enough solution that enables us to implement request monitors, rate limiting, etc., but I'm concerned about the ability of in-place mutation of request objects. What are the actual use cases when you need to modify requests in interceptors instead of using the request configuration API?
I didn't dive deep into the implementation details, though, and mostly reviewed the public API.
| } | ||
| } | ||
|
|
||
| type QueryAttemptInterceptorChain struct { |
There was a problem hiding this comment.
I think we can make this a part of gocql api - we already expose a couple of similar types for event listeners: SessionReadyListenersMux , SchemaListenersMux, etc.
There was a problem hiding this comment.
We could... this was originally intended as sample code, and I don't know how many customers will actually need to compose multiple interceptors at a time. Do you want me to move this to query_executor.go?
There was a problem hiding this comment.
I'm actually neutral on this, but we already have two use-cases where we could use interceptors: monitoring and rate-limiting, so having chain out of the box looks reasonable to me. If nobody has objections, you may make it part of gocql public api.
I think query_executor.go is fine.
| switch q := attempt.Statement.Statement().(type) { | ||
| case *gocql.Query: | ||
| // Inspect query | ||
| log.Println(q.Statement()) | ||
| case *gocql.Batch: | ||
| // Inspect batch | ||
|
|
||
| log.Println(q.Entries[0].Stmt) | ||
| } |
There was a problem hiding this comment.
Basically, this allows in-place modification of the submitted Query / Batch, and it violates the immutable nature that the driver follows, so changes made by interceptors might make Query / Batch objects potentially unreusable.
Is there any particular use case when you want to use the ability of interceptors to modify queries over their configuration API?
There was a problem hiding this comment.
I have to apologize here -- the comments in cluster.go and doc.go were out of date. Due to refactors in the v2 API, creation of the internalRequest is upstream of exec invocation, and this is limits our ability to mutate the query/batch in the interceptor without re-exposing that class or more dramatic refactor. The query/batch are not mutable in the interceptor.
The interceptor still provides unique value over an Observer because it is invoked before exec() and can fail, providing an opportunity for e.g. load shedding.
There was a problem hiding this comment.
I've just written a basic test to confirm that requests are immutable, and I'm able to mutate them in an interceptor:
type simpleInterceptor struct {
logger StructuredLogger
}
func (s *simpleInterceptor) Intercept(ctx context.Context, attempt QueryAttempt, handler QueryAttemptHandler) (*Iter, error) {
switch attempt.Statement.Statement().(type) {
case *Query:
query := attempt.Statement.Statement().(*Query)
s.logger.Info("Intercepting query", NewLogFieldString("statement", query.Statement()))
query.Consistency(One)
default:
}
return handler(ctx)
}
func Test_QueryObjectImmutabilityInInterceptor(t *testing.T) {
interceptor := &simpleInterceptor{logger: NewLogger(LogLevelInfo)}
session := createSession(t, func(config *ClusterConfig) {
config.ExecAttemptInterceptor = interceptor
})
defer session.Close()
expectedConsistency := Quorum
query := session.Query("SELECT host_id FROM system.local").Consistency(expectedConsistency)
var hostID string
err := query.Scan(&hostID)
if err != nil {
t.Fatalf("Failed to scan host ID: %v", err)
}
require.Equal(t, expectedConsistency.String(), query.GetConsistency().String(), "Query object should not be mutated by the interceptor")
}Output:
2026/05/05 11:28:35 logger.go:165: INF gocql: Intercepting query statement=DROP KEYSPACE IF EXISTS gocql_test
2026/05/05 11:28:37 logger.go:165: INF gocql: Intercepting query statement=CREATE KEYSPACE gocql_test
WITH replication = {
'class' : 'SimpleStrategy',
'replication_factor' : 1
}
2026/05/05 11:28:39 logger.go:165: INF gocql: Intercepting query statement=SELECT host_id FROM system.local
--- FAIL: Test_QueryObjectImmutabilityInInterceptor (3.73s)
/home/worry/projects/go/github.com/worryg0d/gocql/integration_test.go:1013:
Error Trace: /home/worry/projects/go/github.com/worryg0d/gocql/integration_test.go:1013
Error: Not equal:
expected: "QUORUM"
actual : "ONE"
Diff:
--- Expected
+++ Actual
@@ -1 +1 @@
-QUORUM
+ONE
Test: Test_QueryObjectImmutabilityInInterceptor
Messages: Query object should not be mutated by the interceptor
FAIL
FAIL github.com/apache/cassandra-gocql-driver/v2 3.736s
|
It would be nice if there was a way to determine whether an observed query/attempt was itself triggered by speculative execution vs. retry policy. There's no reliable way to determine that today which makes it hard to observe behavior changes at scale in relation to tweaking one or the other. |
…ments. The interceptor class is called before every exec call, not just queries. The new class name reflects this. Also, notably, the `ExecAttemptInterceptor` cannot mutate the query/batch -- it is downstream of the creation of the `internalRequest`, and that class is package-private and therefore cannot be passed into `Intercept()` directly. This commit clarifies this fact in comments. Making the query mutable would require more invasive refactoring and would violate immutability invariants intentionally built into the v2 interface.
Remove mention of mutating queries.
Sure, we could modify |
This supersedes #1820 rebasing on top of, and taking into account, changes in v2.
Notably, this removes the ability to mutate the Statement in the Interceptor -- it is now embedded in the
internalRequest, making it inaccessible to a public API without invasive refactoring.Additionally, I modified
queryMetricsin a few ways:Fixes #1786.