You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "joao-r-reis (via GitHub)" <gi...@apache.org> on 2025/09/24 21:12:32 UTC
[PR] CASSGO-90 Make QueryMetrics (Latency and Attempts) opt-in [cassandra-gocql-driver]
joao-r-reis opened a new pull request, #1912:
URL: https://github.com/apache/cassandra-gocql-driver/pull/1912
In 2.0.0-rc1 we got rid of the query object pooling and now the extra allocs associated with the code that is collecting QueryMetrics is 1 of the factors that are leading to a small performance regression.
Since the majority of the users don't use Query Metrics, it makes more sense to make these opt-in.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSGO-90 Don't collect host metrics if a query/batch observer is not provided [cassandra-gocql-driver]
Posted by "joao-r-reis (via GitHub)" <gi...@apache.org>.
joao-r-reis merged PR #1912:
URL: https://github.com/apache/cassandra-gocql-driver/pull/1912
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSGO-90 Make QueryMetrics (Latency) and HostMetrics opt-in [cassandra-gocql-driver]
Posted by "joao-r-reis (via GitHub)" <gi...@apache.org>.
joao-r-reis commented on code in PR #1912:
URL: https://github.com/apache/cassandra-gocql-driver/pull/1912#discussion_r2378964049
##########
session.go:
##########
@@ -944,14 +949,26 @@ func (qm *queryMetrics) attempt(addAttempts int, addLatency time.Duration,
updateHostMetrics.Attempts += addAttempts
updateHostMetrics.TotalLatency += addLatency.Nanoseconds()
- var hostMetricsCopy *hostMetrics
- if needsHostMetrics {
- hostMetricsCopy = new(hostMetrics)
- *hostMetricsCopy = *updateHostMetrics
- }
-
qm.l.Unlock()
- return totalAttempts, hostMetricsCopy
+ return totalAttempts, updateHostMetrics
+}
+
+var nilHostMetrics = &hostMetrics{}
Review Comment:
Yeah my first implementation of this was a truly nil object but I realized later that `attempts` is used for retry policies so can't get rid of that one. Also I called it `nil` because of the [Null Object Pattern](https://en.wikipedia.org/wiki/Null_object_pattern) but I'm not even sure if using this name in the context of Go is a good idea so I'll just rename it to `empty` anyway.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSGO-90 Make QueryMetrics (Latency) and HostMetrics opt-in [cassandra-gocql-driver]
Posted by "joao-r-reis (via GitHub)" <gi...@apache.org>.
joao-r-reis commented on code in PR #1912:
URL: https://github.com/apache/cassandra-gocql-driver/pull/1912#discussion_r2378971158
##########
session.go:
##########
@@ -1500,11 +1530,17 @@ func (iter *Iter) Columns() []ColumnInfo {
}
// Attempts returns the number of times the statement was executed.
+//
+// Set Query.WithQueryMetrics to true
+// so that this Attempts metric is available, otherwise this method will always return 0.
Review Comment:
Good catch
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSGO-90 Don't collect host metrics if a query/batch observer is not provided [cassandra-gocql-driver]
Posted by "joao-r-reis (via GitHub)" <gi...@apache.org>.
joao-r-reis commented on PR #1912:
URL: https://github.com/apache/cassandra-gocql-driver/pull/1912#issuecomment-3357223059
After discussing with @jameshartig we decided to go with another approach. Instead of doing a breaking change and adding a new API we'll just stop collecting host metrics (therefore removing the map allocation) when an observer is not provided.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSGO-90 Make QueryMetrics (Latency) and HostMetrics opt-in [cassandra-gocql-driver]
Posted by "worryg0d (via GitHub)" <gi...@apache.org>.
worryg0d commented on code in PR #1912:
URL: https://github.com/apache/cassandra-gocql-driver/pull/1912#discussion_r2377948952
##########
session.go:
##########
@@ -944,14 +949,26 @@ func (qm *queryMetrics) attempt(addAttempts int, addLatency time.Duration,
updateHostMetrics.Attempts += addAttempts
updateHostMetrics.TotalLatency += addLatency.Nanoseconds()
- var hostMetricsCopy *hostMetrics
- if needsHostMetrics {
- hostMetricsCopy = new(hostMetrics)
- *hostMetricsCopy = *updateHostMetrics
- }
-
qm.l.Unlock()
- return totalAttempts, hostMetricsCopy
+ return totalAttempts, updateHostMetrics
+}
+
+var nilHostMetrics = &hostMetrics{}
Review Comment:
I think the `nil` prefix is misleading, because value of this var is not actually nil. Probably the `empty` prefix is better?
The same applies to `nilQueryMetricsImpl`
##########
session.go:
##########
@@ -1500,11 +1530,17 @@ func (iter *Iter) Columns() []ColumnInfo {
}
// Attempts returns the number of times the statement was executed.
+//
+// Set Query.WithQueryMetrics to true
+// so that this Attempts metric is available, otherwise this method will always return 0.
Review Comment:
From what I understand, attempts are always available, so this comment is not true
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSGO-90 Make QueryMetrics (Latency) and HostMetrics opt-in [cassandra-gocql-driver]
Posted by "joao-r-reis (via GitHub)" <gi...@apache.org>.
joao-r-reis commented on code in PR #1912:
URL: https://github.com/apache/cassandra-gocql-driver/pull/1912#discussion_r2378964049
##########
session.go:
##########
@@ -944,14 +949,26 @@ func (qm *queryMetrics) attempt(addAttempts int, addLatency time.Duration,
updateHostMetrics.Attempts += addAttempts
updateHostMetrics.TotalLatency += addLatency.Nanoseconds()
- var hostMetricsCopy *hostMetrics
- if needsHostMetrics {
- hostMetricsCopy = new(hostMetrics)
- *hostMetricsCopy = *updateHostMetrics
- }
-
qm.l.Unlock()
- return totalAttempts, hostMetricsCopy
+ return totalAttempts, updateHostMetrics
+}
+
+var nilHostMetrics = &hostMetrics{}
Review Comment:
Yeah my first implementation of this was a truly nil object but I realized later that `attempts` is used for retry policies so can't get rid of that one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org