Merged
Conversation
AdamGS
force-pushed
the
adamg/metrics-v2
branch
from
e28c2c5 to
5a80e52
Compare
AdamGS
force-pushed
the
adamg/metrics-v2
branch
from
5a80e52 to
2b519f2
Compare
AdamGS
commented
Jan 28, 2026
| @@ -0,0 +1 @@ | |||
| Cargo.lock No newline at end of file | |||
Contributor
Author
There was a problem hiding this comment.
this is just some housekeeping I ran into
AdamGS
force-pushed
the
adamg/metrics-v2
branch
from
7c00333 to
cccc3d5
Compare
Merging this PR will improve performance by 34.55%
Performance Changes
Comparing 
Footnotes
|
AdamGS
force-pushed
the
adamg/metrics-v2
branch
2 times, most recently
from
215fcde to
ab32105
Compare
AdamGS
added
action/benchmark-sql
github-actions
bot
removed
the
action/benchmark-sql
Contributor
🚨🚨🚨❌❌❌ SQL BENCHMARK FAILED ❌❌❌🚨🚨🚨Benchmark |
Contributor
🚨🚨🚨❌❌❌ SQL BENCHMARK FAILED ❌❌❌🚨🚨🚨Benchmark |
Contributor
Benchmarks: TPC-H SF=1 on NVMESummary
Detailed Results Table
|
Contributor
Benchmarks: FineWeb NVMeSummary
Detailed Results Table
|
Contributor
Benchmarks: TPC-DS SF=1 on NVMESummary
Detailed Results Table
|
Contributor
Benchmarks: FineWeb S3Summary
Detailed Results Table
|
Contributor
Benchmarks: TPC-H SF=10 on NVMESummary
Detailed Results Table
|
Contributor
Benchmarks: TPC-H SF=10 on S3Summary
Detailed Results Table
|
Contributor
Benchmarks: TPC-H SF=1 on S3Summary
Detailed Results Table
|
AdamGS
force-pushed
the
adamg/metrics-v2
branch
from
ab32105 to
32d8be0
Compare
github-actions
bot
removed
the
action/benchmark-sql
Contributor
Benchmarks: Statistical and Population GeneticsSummary
Detailed Results Table
|
Contributor
Benchmarks: Clickbench on NVMESummary
Detailed Results Table
|
AdamGS
force-pushed
the
adamg/metrics-v2
branch
3 times, most recently
from
a56d1ea to
cfff979
Compare
This was referenced Feb 2, 2026
AdamGS
added a commit
that referenced
this pull request
Feb 3, 2026
The tests are failing due to a bug in the `url` crate, I had a fix as part of #6187 but it's taking longer than expected to get it in, so figured I'll just take it out. Signed-off-by: Adam Gutglick <adam@spiraldb.com>
robert3005
reviewed
Feb 3, 2026
| type Item = (MetricId, &'a Metric); | ||
| /// Default metrics registry, stores all state in-memory. | ||
| #[derive(Default, Clone)] | ||
| pub struct DefaultMetricsRegistry { |
Contributor
There was a problem hiding this comment.
Debating whether we want a RwLock or Mutex here
Contributor
Author
There was a problem hiding this comment.
I don't think we ever take a read lock here unless its after the scan is finished and there's no contention
robert3005
reviewed
Feb 3, 2026
Contributor
robert3005
left a comment
robert3005
left a comment
There was a problem hiding this comment.
I made couple small comments. The thing that I am not convinced by is double arcing. But I can see how it ends up being necessary
vortex-metrics/src/timer.rs
Outdated
Comment on lines
4
to
5
| #![allow(clippy::expect_used)] | ||
| #![allow(clippy::unwrap_in_result)] |
Contributor
Author
There was a problem hiding this comment.
moved them to the smallest scope possible
vortex-metrics/src/histogram.rs
Outdated
Comment on lines
4
to
5
| #![allow(clippy::expect_used)] | ||
| #![allow(clippy::unwrap_in_result)] |
|
|
||
| /// Sets the gauge to a specific value. | ||
| pub fn set(&self, value: f64) { | ||
| _ = self.0.swap(value.to_bits(), Ordering::AcqRel); |
Contributor
Author
There was a problem hiding this comment.
my understanding is that using swap here makes the memory ordering such that I can do gauge.get() and then a gague.set(..) and get a consistent ordering.
Contributor
There was a problem hiding this comment.
Can we leave a comment here? On the surface it makes sense to use store but as you point out swap makes the store wait for a get if there’s one ongoing so you get consistent ordering
AdamGS
force-pushed
the
adamg/metrics-v2
branch
from
ff7bbe3 to
5933bad
Compare
robert3005
approved these changes
Feb 6, 2026
Contributor
robert3005
left a comment
robert3005
left a comment
There was a problem hiding this comment.
Feel free to merge it once you add a comment about swap in gauge
Signed-off-by: Adam Gutglick <adam@spiraldb.com>
Signed-off-by: Adam Gutglick <adam@spiraldb.com>
Signed-off-by: Adam Gutglick <adam@spiraldb.com>
AdamGS
force-pushed
the
adamg/metrics-v2
branch
from
051efe9 to
40c54bc
Compare
danking
pushed a commit
that referenced
this pull request
Feb 6, 2026
The tests are failing due to a bug in the `url` crate, I had a fix as part of #6187 but it's taking longer than expected to get it in, so figured I'll just take it out. Signed-off-by: Adam Gutglick <adam@spiraldb.com>
danking
pushed a commit
that referenced
this pull request
Feb 6, 2026
Closes #6078. This PR reworks our metrics crate to simplify it and makes it much harder to make it use unbounded amounts of memory. The implementation is inspired by our current use of `witchcraft-metrics`, the `metrics` crate, DataFusion's metrics and some reading of the OTel spec. It includes several changes: 1. `VortexMetrics` is replaced by the new `MetricsRegistry` trait, which allows user to integrate metrics to whatever metrics backend they have. 1. Removes metrics from the session, making it the responsibility of the user to wire it correctly between various components. 1. No more hierarchical metrics instances, just wire labels if they exist. 1. Removes the `witchcraft-metrics` dependency. It does many things we don't care about or that are opinionated in a way that I don't think is intuitive or useful for most users. It also makes our external API less dependent on other crates we have no control over. 1. Histograms are now f64 and don't use exponential decay, Timers only speak in `Duration`, and counters are u64s. It also introduces a `Gauge` type which I don't think we actually use and I might just delete. I've tested it by running our DF benchmarks with `--show-metrics` and that seems to not change, if anyone else has made use of the metrics, I would love to help test and debug them in another context. --------- Signed-off-by: Adam Gutglick <adam@spiraldb.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #6078.
This PR reworks our metrics crate to simplify it and makes it much harder to make it use unbounded amounts of memory. The implementation is inspired by our current use of
witchcraft-metrics, themetricscrate, DataFusion's metrics and some reading of the OTel spec.It includes several changes:
VortexMetricsis replaced by the newMetricsRegistrytrait, which allows user to integrate metrics to whatever metrics backend they have.witchcraft-metricsdependency. It does many things we don't care about or that are opinionated in a way that I don't think is intuitive or useful for most users. It also makes our external API less dependent on other crates we have no control over.Duration, and counters are u64s. It also introduces aGaugetype which I don't think we actually use and I might just delete.I've tested it by running our DF benchmarks with
--show-metricsand that seems to not change, if anyone else has made use of the metrics, I would love to help test and debug them in another context.