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

report mempool fetched and missing counts to prometheus #134

Merged

Conversation

philippem
Copy link
Collaborator

We are currently reporting the mempool tx count to prometheus.
This MR adds the missing and fetched counts to the same prometheus gauge, using different labels.

@philippem philippem requested a review from shesek December 5, 2024 18:48
src/new_index/mempool.rs Outdated Show resolved Hide resolved
src/new_index/mempool.rs Outdated Show resolved Hide resolved
@philippem philippem force-pushed the add-mempool-prometheus-metrics branch from a5e41db to 355d262 Compare December 7, 2024 00:55
@shesek
Copy link
Collaborator

shesek commented Dec 10, 2024

There's an IntGaugeVec we could use:

The integer version of GaugeVec. Provides better performance if metric values are all integers.

However, this will likely make it impossible to compare historical values recorded as floats with new integer values, so I'm not sure if its worth it. What do you think?

@philippem
Copy link
Collaborator Author

philippem commented Dec 10, 2024

There's an IntGaugeVec we could use:

The integer version of GaugeVec. Provides better performance if metric values are all integers.

However, this will likely make it impossible to compare historical values recorded as floats with new integer values, so I'm not sure if its worth it. What do you think?

Prometheus stores all metrics as float64. https://prometheus.io/docs/concepts/data_model/
I wonder where the better performance will be on... maybe on the exporter side?
Changing to IntGaugeVec would impact metrics.rs which we received from upstream, I don't think it's worth changing it given that the metrics end up as floats in Prometheus.

@philippem philippem force-pushed the add-mempool-prometheus-metrics branch from 355d262 to 3fabe0f Compare December 10, 2024 16:54
@shesek
Copy link
Collaborator

shesek commented Dec 10, 2024

I wonder where the better performance will be on... maybe on the exporter side?

It appears so, it uses integer date types on the rust side which is more performant.

The PR that introduced this to rust-prometheus includes some benchmarks:

$ ENABLE_FEATURES="nightly" make bench
...
test counter::bench_counter_no_labels                   ... bench:          13 ns/iter (+/- 4)
test counter::bench_int_counter_no_labels               ... bench:           6 ns/iter (+/- 0)
test gauge::bench_gauge_no_labels                       ... bench:          15 ns/iter (+/- 1)
test gauge::bench_int_gauge_no_labels                   ... bench:           7 ns/iter (+/- 1)
...

IntCounter and IntGauge can improve 50% performance.


Changing to IntGaugeVec would impact metrics.rs which we received from upstream, I don't think it's worth changing it given that the metrics end up as floats in Prometheus.

I don't feel strongly about this, and it can be done in a future PR if we do decide to do it. But keeping the codebase similar to upstream is long a lost cause, it was changed significantly in our fork and basically rewritten in romanz/electrs.

@shesek
Copy link
Collaborator

shesek commented Dec 10, 2024

Prometheus stores all metrics as float64

Also if that's the case (I was not aware of it), my earlier "impossible to compare historical values recorded as floats with new integer values" comment is probably incorrect

@philippem
Copy link
Collaborator Author

Prometheus stores all metrics as float64

Also if that's the case (I was not aware of it), my earlier "impossible to compare historical values recorded as floats with new integer values" comment is probably incorrect

The electrs prometheus exporter (gauges) are snapshots. The prometheus server will scrape and record values, and we can run queries on the data in prometheus, and compare historical values. The histograms generated by electrs are less useful, because they are an aggregation since the last electrs restart.

@philippem
Copy link
Collaborator Author

I wonder where the better performance will be on... maybe on the exporter side?

It appears so, it uses integer date types on the rust side which is more performant.

The PR that introduced this to rust-prometheus includes some benchmarks:

$ ENABLE_FEATURES="nightly" make bench
...
test counter::bench_counter_no_labels                   ... bench:          13 ns/iter (+/- 4)
test counter::bench_int_counter_no_labels               ... bench:           6 ns/iter (+/- 0)
test gauge::bench_gauge_no_labels                       ... bench:          15 ns/iter (+/- 1)
test gauge::bench_int_gauge_no_labels                   ... bench:           7 ns/iter (+/- 1)

where is that time being spent? How often are we manipulating the gauges and counters?

IntCounter and IntGauge can improve 50% performance.

From what?

I don't feel strongly about this, and it can be done in a future PR if we do decide to do it. But keeping the codebase similar to upstream is long a lost cause, it was changed significantly in our fork and basically rewritten in romanz/electrs.
OK

@philippem philippem force-pushed the add-mempool-prometheus-metrics branch from 3fabe0f to f6bbac6 Compare December 10, 2024 21:52
@philippem
Copy link
Collaborator Author

amended

@shesek
Copy link
Collaborator

shesek commented Dec 12, 2024

mempool.rs.copy got accidentally committed. All LGTM other than that.

@philippem philippem force-pushed the add-mempool-prometheus-metrics branch from f6bbac6 to bb78705 Compare December 12, 2024 15:12
@philippem philippem force-pushed the add-mempool-prometheus-metrics branch from bb78705 to 5e02898 Compare December 12, 2024 15:14
@philippem
Copy link
Collaborator Author

  • removed file that was committed by accident
  • rebased onto new-index

@philippem philippem merged commit ebc9312 into Blockstream:new-index Dec 12, 2024
2 checks passed
@philippem philippem deleted the add-mempool-prometheus-metrics branch December 12, 2024 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants