Skip to content
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

Make metrics collection optional/faster #1147

Conversation

QuerthDP
Copy link
Contributor

@QuerthDP QuerthDP commented Dec 10, 2024

This patch contains:

  • Implementation of lock-free histogram with hot and cold bucket pools
  • Introduction of crate feature "metrics" to make using them optional
  • Functionality to gather latency statistics via histogram snapshots
  • Implementation of rates of queries per second based on cpp-driver implementation.
  • Initial implementation of connection metrics (changes required after review)

Fixes: #330

Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • I added relevant tests for new features and bug fixes.
  • All commits compile, pass static checks and pass test.
  • PR description sums up the changes and reasons why they should be introduced.
  • I have provided docstrings for the public items that I want to introduce.
  • I have adjusted the documentation in ./docs/source/.
  • I added appropriate Fixes: annotations to PR description.

Copy link

github-actions bot commented Dec 10, 2024

cargo semver-checks detected some API incompatibilities in this PR.
Checked commit: 6c749d7

See the following report for details:

cargo semver-checks output
./scripts/semver-checks.sh --baseline-rev e808345d7ab1e80970a8bd8371e367aec0e5cdbf
+ cargo semver-checks -p scylla -p scylla-cql --baseline-rev e808345d7ab1e80970a8bd8371e367aec0e5cdbf
     Cloning e808345d7ab1e80970a8bd8371e367aec0e5cdbf
    Building scylla v0.15.0 (current)
       Built [  34.933s] (current)
     Parsing scylla v0.15.0 (current)
      Parsed [   0.051s] (current)
    Building scylla v0.15.0 (baseline)
       Built [  33.554s] (baseline)
     Parsing scylla v0.15.0 (baseline)
      Parsed [   0.049s] (baseline)
    Checking scylla v0.15.0 -> v0.15.0 (no change)
     Checked [   0.119s] 127 checks: 124 pass, 3 fail, 0 warn, 0 skip

--- failure auto_trait_impl_removed: auto trait no longer implemented ---

Description:
A public type has stopped implementing one or more auto traits. This can break downstream code that depends on the traits being implemented.
        ref: https://doc.rust-lang.org/reference/special-types-and-traits.html#auto-traits
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.39.0/src/lints/auto_trait_impl_removed.ron

Failed in:
  type MetricsError is no longer UnwindSafe, in /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/observability/metrics.rs:11
  type MetricsError is no longer RefUnwindSafe, in /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/observability/metrics.rs:11

--- failure inherent_method_missing: pub method removed or renamed ---

Description:
A publicly-visible method or associated fn is no longer available under its prior name. It may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.39.0/src/lints/inherent_method_missing.ron

Failed in:
  Metrics::new, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-e808345d7ab1e80970a8bd8371e367aec0e5cdbf/b0de710f68985dca38fbe4c6cffd9ea9bfd6e808/scylla/src/observability/metrics.rs:35

--- failure struct_with_no_pub_fields_changed_type: public API struct with no public fields is no longer a struct ---

Description:
A struct without pub fields became an enum or union, breaking pattern matching.
        ref: https://internals.rust-lang.org/t/rest-patterns-foo-should-match-non-struct-types/21607
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.39.0/src/lints/struct_with_no_pub_fields_changed_type.ron

Failed in:
  struct scylla::observability::metrics::MetricsError became enum in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/observability/metrics.rs:11

     Summary semver requires new major version: 3 major and 0 minor checks failed
    Finished [  70.251s] scylla
    Building scylla-cql v0.4.0 (current)
       Built [  10.837s] (current)
     Parsing scylla-cql v0.4.0 (current)
      Parsed [   0.029s] (current)
    Building scylla-cql v0.4.0 (baseline)
       Built [  11.033s] (baseline)
     Parsing scylla-cql v0.4.0 (baseline)
      Parsed [   0.029s] (baseline)
    Checking scylla-cql v0.4.0 -> v0.4.0 (no change)
     Checked [   0.115s] 127 checks: 127 pass, 0 skip
     Summary no semver update required
    Finished [  22.790s] scylla-cql
make: *** [Makefile:64: semver-rev] Error 1

@QuerthDP QuerthDP force-pushed the 330-make-metrics-collection-optional/faster branch from 74b7fa3 to d989a59 Compare December 11, 2024 10:50
@QuerthDP QuerthDP force-pushed the 330-make-metrics-collection-optional/faster branch from d7b32d0 to f9a7153 Compare January 7, 2025 22:28
@wprzytula wprzytula added this to the 0.16.0 milestone Jan 9, 2025
@QuerthDP QuerthDP force-pushed the 330-make-metrics-collection-optional/faster branch from bff846c to 88de7ff Compare January 12, 2025 18:46
@QuerthDP QuerthDP marked this pull request as ready for review January 12, 2025 19:01
@QuerthDP QuerthDP force-pushed the 330-make-metrics-collection-optional/faster branch from 88de7ff to 185ff57 Compare January 13, 2025 12:42
@QuerthDP
Copy link
Contributor Author

Changelog:

  • rebased on main

@github-actions github-actions bot added the semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes label Jan 13, 2025
@NikodemGapski NikodemGapski force-pushed the 330-make-metrics-collection-optional/faster branch from 185ff57 to 7be0e38 Compare January 14, 2025 19:03
Copy link
Contributor

@muzarski muzarski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still need to review commits that introduce Meter and connection metrics. Posting this review early, because there are some matters to discuss.

Mutex,
}

pub struct LockFreeHistogram {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a docstring as well. Not only this is a public type (does it need to be public?), but its methods contain some very complex logic, which could be briefly explained here.

Also, it would be nice to mention the motivation behind this struct. I understand the logic (after reviewing the methods), but I still don't quite get WHY we need it. Why can't we just use the AtomicHistogram from histogram crate? Are there any races that can occur without this additional layer of synchronization?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, so there are a couple of things to mention here, but I'll start from the beginning.

In the issue description here it is noted that no suitable solution for the problem was found on crates.io (I believe AtomicHistogram already existed by the time of writing of that issue), so I assumed it must have had a flaw. And, as it turned out, it did.

I highly recommend reading through the issue I opened up on the crate histogram repo, where I explain all of the motivation in detail, but in short: the .load() method has no synchronisation with increments, which causes a logical race (the state of the loaded histogram is dependent on the speed at which it is loaded).

The idea of some sort of a lock-free algorithm was also proposed in the "Make metrics optional" issue, along with sharding, which I considered potentially harder to implement, thus I went with a lock-free algorithm.

However (!), the LockFreeHistogram's implementation comes with potential drawbacks in terms of performance (in comparison to AtomicHistogram, not a global mutex) due to global atomic counters accessed upon each bucket increment.

I haven't managed to run any benchmarks in this regard, nor do I have concrete examples of cases when AtomicHistogram's implementation yields a very significant error in results (though I did come up with some ideas and calculations; they can be found on my linked issue on crate histogram's repo). Therefore, the decision of which histogram implementation to incorporate into this driver is up to you. I've just provided a safe alternative and done some research.

Also, should you choose to go with AtomicHistogram, the change will be rather effortless, as I maintained the API schema used in crate histogram for my implementation.

Copy link
Contributor

@muzarski muzarski Jan 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the explanation! I think part of this deserves to be put in the docstring.

Also, should you choose to go with AtomicHistogram, the change will be rather effortless, as I maintained the API schema used in crate histogram for my implementation.

This is great. Also, please unpub (pub(crate)) the LockFreeHistogram and its methods. Only then will we be 100% sure that if we ever decide to drop/modify LockFreeHistogram, such change will not be API breaking.

Which histogram implementation we want to incorporate to the driver? Well, this needs to be discussed. I'd wait until @Lorak-mmk and @wprzytula review the code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I unpubbed LockFreeHistogram and its methods, but now I can see it might be difficult to make such a change completely non-API-breaking. That's because LockFreeHistogramError is propagated as MetricsError's cause and thus needs to be pub. Upon change to AtomicHistogram this error struct would be removed entirely.

I'm not yet sure how we could go around this issue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good catch. There are some workarounds for this, however. For example, we could always hide underlying LockFreeHistogramError under Arc<dyn Error>. I wouldn't worry about this now. Let's wait for others to join the discussion.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For what it's worth, while .load() is not atomic wrt concurrent increments into the histogram. I'd still consider using the AtomicHistogram which is used in metrics for both rpc-perf and Pelikan.

As I said in the issue opened in rustcommon, I'm not sure that the potential skew here would be meaningful. But I do welcome some concrete details if such skew does prove to be meaningful.

The TLDR is I'm not sure how much it matters whether it's on one side or the other of loading a histogram. If you envision periodically snapshotting the histogram, it seems you have to accept that the latency is already being recorded at the tail end of the event. Imagine a request that takes a long time, that latency gets incremented after the fact, when the service might be back to responding quickly.

My feeling is that ultimately this is all an approximation and I've found the AtomicHistogram as in the histogram crate to be satisfactory for projects I work on.

Comment on lines 122 to 156
impl Default for LockFreeHistogram {
fn default() -> Self {
// Config: 64ms error, values in range [0ms, ~262_000ms].
// Size: (2^13 + 5 * 2^12) * 8B * 2 ~= 450kB.
let grouping_power = 12;
let max_value_power = 18;
LockFreeHistogram::new(grouping_power, max_value_power)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where are these defaults taken from?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the histogram crate no longer provides defaults, I had to come up with some choice here. It was my best guess at what might be needed, though that is obviously to be discussed and modified if needed.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may find our calculator useful while evaluating what parameters are appropriate: https://observablehq.com/@iopsystems/h2-histogram

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NikodemGapski Have you consulted the above calculator about the defaults?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, I had made my guess according to the mentioned calculator, but I can't know if it meets the needs of the users of this driver (the range and absolute error I noted in the comment for reference). It is your decision whether or not to change it.

@NikodemGapski NikodemGapski force-pushed the 330-make-metrics-collection-optional/faster branch from 7be0e38 to 2f6ae32 Compare January 15, 2025 18:31
@QuerthDP QuerthDP force-pushed the 330-make-metrics-collection-optional/faster branch from 2f6ae32 to b36ef1f Compare January 16, 2025 11:46
@NikodemGapski NikodemGapski force-pushed the 330-make-metrics-collection-optional/faster branch from 0bcde8e to cf43ed2 Compare January 16, 2025 17:20
@QuerthDP QuerthDP force-pushed the 330-make-metrics-collection-optional/faster branch from cf43ed2 to 4392cb8 Compare January 16, 2025 19:57
Comment on lines 19 to 34
/// Snapshot is a structure that contains histogram statistics such as
/// min, max, mean, standard deviation, median, and most common percentiles
/// collected in a certain moment.
#[derive(Debug)]
pub struct Snapshot {
pub min: u64,
pub max: u64,
pub mean: u64,
pub stddev: u64,
pub median: u64,
pub percentile_75: u64,
pub percentile_95: u64,
pub percentile_98: u64,
pub percentile_99: u64,
pub percentile_99_9: u64,
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔧 For future compatibility, let's either:

  • make this struct #[non_exhaustive] to allow adding more fields in the future without breaking the API;
  • or make all those fields private and expose a getter for each field.

Which one do you find a more suitable solution? @Lorak-mmk @muzarski

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So far I've used the #[non_exhaustive] macro, but this case is still open for discussion.

@QuerthDP QuerthDP force-pushed the 330-make-metrics-collection-optional/faster branch from 4392cb8 to 6dab731 Compare January 22, 2025 11:52
@QuerthDP QuerthDP requested a review from wprzytula January 22, 2025 12:14
@wprzytula wprzytula requested a review from muzarski January 22, 2025 13:36
@QuerthDP QuerthDP force-pushed the 330-make-metrics-collection-optional/faster branch from 6dab731 to 32537f9 Compare January 26, 2025 16:13
@QuerthDP
Copy link
Contributor Author

Changelog:

  • rebased on main

@QuerthDP QuerthDP force-pushed the 330-make-metrics-collection-optional/faster branch from bcb3b77 to f297b75 Compare January 27, 2025 18:33
@QuerthDP
Copy link
Contributor Author

Changelog:

  • rebased @wprzytula's refactor on the branch and squashed his contribution into b3ae501
  • adjusted EWMA docstrings
  • fixed invalid unwraps in examplary code
  • added more clarity to the request timeout
  • resolved nitpicks

@QuerthDP QuerthDP requested a review from wprzytula January 27, 2025 18:35
@QuerthDP QuerthDP force-pushed the 330-make-metrics-collection-optional/faster branch from f297b75 to b31d836 Compare January 27, 2025 19:13
@Lorak-mmk
Copy link
Collaborator

Hi, thanks for the PR!

I did not read all the code yet, but I did read the linked histogram issue.

I think that the best course for now is to:

  1. Make sure that implementation details are not exposed, so that we can easily swap underlying histogram implementation in the future. I understand that the only think preventing this is the error type, right? In such case it makes sense to put it in Arc<dyn Error>.
  2. For now use the AtomicHistogram, but put LockFreeHistogram somewhere it won't get lost (maybe a new repo?)

Why do I think we should use AtomicHistogram (at least for now)?

  • The original motivation for meddling with metrics is performance. Afaiu AtomicHistogram has better performance (but we don't have the benchmarks that could tell how much better), in exchange for being a small bit imprecise. Imo our metrics use case is not something that requires the absolute precision provided by LockFreeHistogram, so it should be ok to sacrifice it in the name of performance. If it ever turns out that we need the precision, we can change the implementation.
  • Histograms author provided examples of other crates successfully using AtomicHistogram for similar purposes to ours, which inspires confidence in the implementation.
  • LockfreeHistogram is more code to be maintained by us, so we should have a good rationale to use it.

wdyt @wprzytula @muzarski ?

@muzarski
Copy link
Contributor

wdyt @wprzytula @muzarski ?

Yep, sounds good to me.

@Lorak-mmk Lorak-mmk modified the milestones: 0.16.0, 1.0.0 Feb 5, 2025
@roydahan
Copy link
Collaborator

roydahan commented Feb 6, 2025

@QuerthDP can you please fix the conflicts and address any comment left? (if any)

@QuerthDP QuerthDP force-pushed the 330-make-metrics-collection-optional/faster branch from 27e4e86 to 8a0ea57 Compare February 6, 2025 20:11
@QuerthDP
Copy link
Contributor Author

QuerthDP commented Feb 6, 2025

Changelog:

  • rebased on main
  • added safety comments to the tick functions

TODO:

@NikodemGapski

  • replace LockFreeHistogram with AtomicHistogram (leaving a possible way to revert this change)

@NikodemGapski NikodemGapski force-pushed the 330-make-metrics-collection-optional/faster branch from 8a0ea57 to 66b63c9 Compare February 8, 2025 16:36
@NikodemGapski
Copy link
Contributor

Changelog:

  • rebased on main
  • replaced LockFreeHistogram with AtomicHistogram (the lock-free version can still be found on this branch of our fork)
  • hid the potential synchronisation metrics errors under Arc<dyn Error> (that's where the lock-free errors may go)

@QuerthDP QuerthDP force-pushed the 330-make-metrics-collection-optional/faster branch from 66b63c9 to 6f5026a Compare February 17, 2025 12:58
@QuerthDP
Copy link
Contributor Author

Changelog:

  • rebased on main
  • added requested #[non-exhaustive]

Comment on lines 455 to 457
host_filter: host_filter.clone(),
initial_known_nodes,
control_connection_repair_requester,
#[cfg(feature = "metrics")]
metrics,
})
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@muzarski @wprzytula I'm looking at the commit that puts metrics behind crate future, and I'm wondering:
maybe it is better to restrict those cfg's to the metrics module, by providing alternative implementation which will be active when cfg(not(feature = "metrics")). This implementation would have no fields, but all the same methods, which would just be empty.

Why? The reasons to put metrics behind future are, afaik:

  • Performance
  • Making histogram dependency optional

Doing what I proposed still achieves those goals, but also remove a lot of conditional compilation flags from other files in the crate, which I think is a great plus for code readability.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method on Session that retrieves the metrics would still be under #[cfg(feature = "metrics")] of course.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@muzarski @wprzytula I'm looking at the commit that puts metrics behind crate future, and I'm wondering: maybe it is better to restrict those cfg's to the metrics module, by providing alternative implementation which will be active when cfg(not(feature = "metrics")). This implementation would have no fields, but all the same methods, which would just be empty.

I understand your idea. The only thing I don't get: "but all the same methods, which would just be empty". How can a method be empty if it returns something other than ()? Apart from that, your suggestion sounds reasonable.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This artifical Metrics type only needs inc_*, dec_*, and log_* methods because those are used to record data.
All such methods return (), apart from log_query_latency, which returns Result, but keeping the MetricsError (maybe as pub(crate) if feature is disabled) is not a problem.
Alternatively it could ignore the error - both call site of this method already do that by using let _ = ....

Copy link
Collaborator

@wprzytula wprzytula Mar 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? The reasons to put metrics behind future are, afaik:

* Performance

* Making histogram dependency optional

Doing what I proposed still achieves those goals, but also remove a lot of conditional compilation flags from other files in the crate, which I think is a great plus for code readability.

Not exactly. Keep in mind that Metrics are passed behind an Arc to connections. Then, by encapsulating the feature flag inside Metrics, we unconditionally pay for Arc allocations and its atomic reference counting.
This is why I'm in favour of leaving the current way of doing this.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a much more reasonable objection!
How I see it:

  • I believe the runtime impact will not be meaningful, so we should focus on code quality.
  • The code that interacts with metric will be in the source anyway. In fact there will be less of it if we remove the cfg lines.
  • Connection / Session does not need to care what Metrics will do with the data provided to them. I see at as sort of a compile-time traits. You would not raise the same objections for the code having Arc<dyn MetricsCollector> and calling its methods, right?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • You would not raise the same objections for the code having Arc<dyn MetricsCollector> and calling its methods, right?

In fact, I would. The difference is that we have Option<Arc<dyn Policy>> instead of Arc<dyn Policy> for run-time traits. If you agree to have metrics behind Option, then I would not raise this particular objection.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't see the difference in this case. As far as I know we never had any reports about Arc's being a performance problem in the driver. Let's not do micro optimizations unless we have data showing they are useful.
Right now we also always clonde the metrics Arc - as we do with ClusterState Arc.
Please also note, that my proposed change can be reverted if it turns out that performance is a problem here - it will not be a breaking change.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct observation, both the change and reverting it are not API-breaking. Therefore, I suggest leaving this as-is and possibly addressing your suggestion after 1.0. WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can postpone this change, but please open an issue about this after merging the PR.

NikodemGapski and others added 8 commits March 2, 2025 01:02
Add atomic histogram from the histogram crate to metrics instead of a plain histogram placed under a mutex.

This commit also updates crate histogram dependency from 0.6.9 to 0.11.1 for atomic functionalities and cleaner error handling.

The updated histogram crate removes the Histogram::mean() method, which is why our own implementation is introduced in this commit.
Implement metrics making use of the histogram
to measure query latencies.

Added metrics are provided by the Snapshot structure
containing metrics such as: min, max, mean, median,
standard deviation and various percentiles.

Co-authored-by: NikodemGapski <[email protected]>
Implement gathering of connection metrics like total number
of active connections, connection timeouts and request timeouts.

Co-authored-by: Wojciech Przytuła <[email protected]>
Add metrics crate feature which enables usage
and gathering of metrics.

Therefore everyone willing to use metrics in their code
is required to add metrics feature in their Cargo.toml
file or compile otherwise with --features metrics flag.

Additionally, add a CI step with cargo checks for this
feature.
Inform that metrics may now only be used under crate feature 'metrics'.
Mention new metrics in documentation and show an example
how to collect them.
Adjust examples to include new metrics.
As the user should not be able to create metrics instance otherwise
than by `get_metrics()` function, the `Metrics::new()` method shall
be set to pub(crate) visibility to support only internal usage.
@QuerthDP QuerthDP force-pushed the 330-make-metrics-collection-optional/faster branch from 6f5026a to 6c749d7 Compare March 2, 2025 00:17
@mykaul
Copy link
Contributor

mykaul commented Mar 3, 2025

My >100 comments PR alarm just popped here. Are we on the right track here?

@wprzytula
Copy link
Collaborator

My >100 comments PR alarm just popped here. Are we on the right track here?

We're just finishing this up.

@dkropachev
Copy link
Collaborator

Closed in favor of #1263

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make metrics collection optional/faster
9 participants