-
Notifications
You must be signed in to change notification settings - Fork 642
CASSGO-90 Don't collect host metrics if a query/batch observer is not provided #1912
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
Conversation
e860946 to
aa3829a
Compare
caa8d88 to
eb0375d
Compare
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.
It's a good idea to make query metrics collectors optional. If someone doesn't care about them or has their own mechanisms to collect such metrics, the driver should allow disabling them. Also, it reduces the amount of mem allocations when we don't really need them.
I left some comments regarding naming and godoc.
Now it makes more sense to me than a copy call on a zero-length source to impact performance that much. Ofc it has some place, but believing that this causes 10% performance degradation is really hard
8bac773 to
41efd6d
Compare
bdb525b to
adeb204
Compare
|
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. |
Patch by João Reis; reviewed by Bohdan Siryk and James Hartig for CASSGO-90
adeb204 to
5fd7672
Compare
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
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
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.Benchmarks here (this branch vs v1.7.0). With this change, the results are much more acceptable. I still think the v5 implementation has room for improvement but this is the first version of our v5 implementation and we can iterate on it in follow up releases. The most important part is that now v4 (in 1.7.0) vs v4 (in 2.0 - this branch) looks ok.Don't collect host metrics if a query/batch observer is not provided, this is a small optimization.