Migrate to Prometheus lib 1.x #7880

fab-10 · 2024-11-13T18:49:01Z

PR description

Prometheus Java Metrics library development now focus on version 1.x, while we are still using the simple client version 0.16.
Since version 1.x introduced many changes and it is not compatible with 0.16, the upgrade is not straightforward and some refactoring is needed to migrate to the latest version, so I took the opportunity to also make the Prometheus code more OO.
The Prometheus HTTP server is not used instead of our custom implementation, so there is less effort to keep it updated, and we get by default improvement, like new export formats like protobuf.

See the CHANGELOG for the breaking changes introduced by this upgrade.

Some JVM metrics have changed name to adhere to the OTEL standard (see the table below), Besu Full Grafana dashboard is updated to support both names

Fixed Issue(s)

Thanks for sending a pull request! Have you done the following?

Checked out our contribution guidelines?
Considered documentation and added the doc-change-required label to this PR if updates are required.
Considered the changelog and included an update if required.
For database changes (e.g. KeyValueSegmentIdentifier) considered compatibility and performed forwards and backwards compatibility tests

Locally, you can run these tests to catch failures early:

unit tests: ./gradlew build
acceptance tests: ./gradlew acceptanceTest
integration tests: ./gradlew integrationTest
reference tests: ./gradlew ethereum:referenceTests:referenceTests

Signed-off-by: Fabio Di Fabio <[email protected]>

…rsion Signed-off-by: Fabio Di Fabio <[email protected]>

Signed-off-by: Fabio Di Fabio <[email protected]> # Conflicts: # metrics/core/build.gradle # metrics/core/src/main/java/org/hyperledger/besu/metrics/noop/NoOpMetricsSystem.java # metrics/core/src/main/java/org/hyperledger/besu/metrics/opentelemetry/OpenTelemetrySystem.java # metrics/core/src/main/java/org/hyperledger/besu/metrics/prometheus/PrometheusMetricsSystem.java # metrics/rocksdb/src/main/java/org/hyperledger/besu/metrics/rocksdb/RocksDBStats.java # plugin-api/build.gradle # plugin-api/src/main/java/org/hyperledger/besu/plugin/services/MetricsSystem.java

# Conflicts: # ethereum/core/src/main/java/org/hyperledger/besu/ethereum/chain/DefaultBlockchain.java # metrics/core/build.gradle # metrics/core/src/main/java/org/hyperledger/besu/metrics/prometheus/PrometheusMetricsSystem.java

Signed-off-by: Fabio Di Fabio <[email protected]>

# Conflicts: # CHANGELOG.md # metrics/core/src/main/java/org/hyperledger/besu/metrics/prometheus/PrometheusMetricsSystem.java # metrics/core/src/main/java/org/hyperledger/besu/metrics/prometheus/PrometheusSuppliedValueCollector.java

# Conflicts: # CHANGELOG.md

Signed-off-by: Fabio Di Fabio <[email protected]>

Matilda-Clerke

LGTM, but leaving unapproved as I'm not an expert on Promethius. Please consider my review comments

besu/src/main/java/org/hyperledger/besu/RunnerBuilder.java

...cs/core/src/main/java/org/hyperledger/besu/metrics/prometheus/AbstractPrometheusSummary.java

...n/java/org/hyperledger/besu/metrics/prometheus/AbstractPrometheusSuppliedValueCollector.java

metrics/core/src/main/java/org/hyperledger/besu/metrics/prometheus/MetricsHttpService.java

...cs/core/src/main/java/org/hyperledger/besu/metrics/prometheus/PrometheusSuppliedCounter.java

metrics/core/src/main/java/org/hyperledger/besu/metrics/prometheus/PrometheusSuppliedGauge.java

...cs/core/src/main/java/org/hyperledger/besu/metrics/prometheus/PrometheusSuppliedSummary.java

Matilda-Clerke · 2024-11-25T00:59:07Z

...-api/src/main/java/org/hyperledger/besu/plugin/services/metrics/LabelledSuppliedSummary.java

+
+import java.util.function.Supplier;
+
+/** The interface Labelled supplied summary. */


Not a fan of this kind of javadoc comment... it doesn't tell us anything the class name doesn't already tell us

Matilda-Clerke · 2024-11-25T00:59:53Z

...-api/src/main/java/org/hyperledger/besu/plugin/services/metrics/LabelledSuppliedSummary.java

+   * Labels.
+   *
+   * @param summarySupplier the summary supplier
+   * @param labelValues the label values


This javadoc also fails to convey any useful information that the method signature doesn't already convey

I understand that, and I also found these comments just redundant, but since the build process requires us to put comments regardless the need of it, also in cases like this, where the code is self documenting, idk what it is better to do, wdyt?

I'd suggest writing javadoc comments such that someone completely new to the code could read it and understand what the code is aiming to do

Rewritten the javadoc to be more useful and removed it where the code is self-explanatory.
Classes in this package are not public and using it requires at least some base knowledge of metrics and Prometheus, so the doc should take that for granted in this case.

In general the javadoc requirement feels like an unnecessary barrier to me. While it is good to write something more meaningful rather than generated, I don't believe the forced comments are providing any actual value. Developers already add adhoc javadoc if it has value.

# Conflicts: # plugin-api/build.gradle

Signed-off-by: Fabio Di Fabio <[email protected]>

# Conflicts: # gradle/verification-metadata.xml

fab-10 · 2024-11-25T17:54:15Z

@Matilda-Clerke I update the PR to improve the javadoc, please check if it is clearer now

This reverts commit 66eaa69. Signed-off-by: Fabio Di Fabio <[email protected]>

Signed-off-by: Fabio Di Fabio <[email protected]>

macfarla

I think it looks ok, I'm assuming you have run up nodes etc and tested the metrics work as expected

CHANGELOG.md

macfarla · 2024-11-26T22:25:26Z

CHANGELOG.md

@@ -4,6 +4,21 @@

 ### Breaking Changes
 - Removed Retesteth rpc service and commands [#7833](https://github.com/hyperledger/besu/pull/7783)
+- With the upgrade of the Prometheus Java Metrics library, there are the following changes:


there are some projects (eg teku) that use metrics core modules - need to communicate breaking changes to them

yes, will support them for the upgrade

siladu · 2024-11-27T02:10:35Z

CHANGELOG.md

+  - Gauge names are not allowed to end with `total`, therefore the metric `besu_blockchain_difficulty_total` is losing the `_total` suffix
+  - The `_created` timestamps are not returned by default, you can set the env var `BESU_OPTS="-Dio.prometheus.exporter.includeCreatedTimestamps=true"` to enabled them


Are either of these metrics used on the Besu full dashboard?

No, they are not used by our official dashboard

siladu · 2024-11-27T02:12:08Z

CHANGELOG.md

+- With the upgrade of the Prometheus Java Metrics library, there are the following changes:
+  - Gauge names are not allowed to end with `total`, therefore the metric `besu_blockchain_difficulty_total` is losing the `_total` suffix
+  - The `_created` timestamps are not returned by default, you can set the env var `BESU_OPTS="-Dio.prometheus.exporter.includeCreatedTimestamps=true"` to enabled them
+  - Some JVM metrics have changed name to adhere to the OTEL standard (see the table below), [Besu Full Grafana dashboard](https://grafana.com/grafana/dashboards/16455-besu-full/) is updated to support both names


That's great there's a dashboard update to go along with this 👍 . For completeness and extra visibility can you also mention this in the PR desc please?
I think we should also call it out in the release notes and when we announce on socials.

updated the description

siladu · 2024-11-27T02:20:11Z

...-api/src/main/java/org/hyperledger/besu/plugin/services/metrics/LabelledSuppliedSummary.java

+   * Labels.
+   *
+   * @param summarySupplier the summary supplier
+   * @param labelValues the label values


In general the javadoc requirement feels like an unnecessary barrier to me. While it is good to write something more meaningful rather than generated, I don't believe the forced comments are providing any actual value. Developers already add adhoc javadoc if it has value.

siladu

Thanks for your efforts on this one 👍

It would be good to call out in the PR desc what testing has been done - I assume it has been tested with the updated dashboard at least?

Has the push gateway functionality also been tested?

It seems there are quite extensive changes to the Timer code and I think the sync duration metrics were relying on some esoteric besu-specific code - so might be worth @pinges taking a look unless you're confident this is all still working as expected @fab-10?

Does these (or any other) docs need updating as well? https://besu.hyperledger.org/public-networks/how-to/monitor/metrics

Co-authored-by: Sally MacFarlane <[email protected]> Signed-off-by: Fabio Di Fabio <[email protected]>

fab-10 · 2024-11-27T09:37:11Z

@macfarla @siladu I have tested that our official dashboard works fine after this update, and timers are working fine, I have also verified that OpenTelemetry is working, but haven't yet tried the push gateway functionality, so I will test it before merging.

For the final user, apart the changes described in the changelog, there is no impact, but will review the doc in case it is referring to some of the breaking change.

# Conflicts: # plugin-api/build.gradle

fab-10 · 2024-11-27T10:53:57Z

@siladu verified that also the Prometheus push gateway works

siladu

Thanks @fab-10

fab-10 force-pushed the upgrade-prometheus-1.x branch 2 times, most recently from 52258c5 to f5c4ebf Compare November 13, 2024 19:41

fab-10 self-assigned this Nov 13, 2024

fab-10 force-pushed the upgrade-prometheus-1.x branch 4 times, most recently from 8c3c5ba to 3f13acf Compare November 19, 2024 14:49

fab-10 added 6 commits November 19, 2024 16:00

Consolidate all metric collectors into the metrics system

0e7db5f

Signed-off-by: Fabio Di Fabio <[email protected]>

Fixes

695a53a

Signed-off-by: Fabio Di Fabio <[email protected]>

Do not recreate the Prometheus metric system

5095e68

Signed-off-by: Fabio Di Fabio <[email protected]>

Bump opentelemetry dependencies

a4d09ae

Signed-off-by: Fabio Di Fabio <[email protected]>

Upgrade to Promethus java client 1.x and adapt the code to the new ve…

a7d8131

…rsion Signed-off-by: Fabio Di Fabio <[email protected]>

fab-10 force-pushed the upgrade-prometheus-1.x branch from 3f13acf to 43770b4 Compare November 19, 2024 15:04

fab-10 added 3 commits November 19, 2024 17:25

Merge branch 'main' into upgrade-prometheus-1.x

ea3b2e3

# Conflicts: # ethereum/core/src/main/java/org/hyperledger/besu/ethereum/chain/DefaultBlockchain.java # metrics/core/build.gradle # metrics/core/src/main/java/org/hyperledger/besu/metrics/prometheus/PrometheusMetricsSystem.java

Add support for counter metric with external supplied value

d8ffff4

Signed-off-by: Fabio Di Fabio <[email protected]>

fab-10 force-pushed the upgrade-prometheus-1.x branch 2 times, most recently from 591a123 to 553b8ee Compare November 20, 2024 18:19

Merge branch 'main' into upgrade-prometheus-1.x

1610025

# Conflicts: # CHANGELOG.md # metrics/core/src/main/java/org/hyperledger/besu/metrics/prometheus/PrometheusMetricsSystem.java # metrics/core/src/main/java/org/hyperledger/besu/metrics/prometheus/PrometheusSuppliedValueCollector.java

fab-10 force-pushed the upgrade-prometheus-1.x branch from 553b8ee to 1610025 Compare November 20, 2024 18:25

fab-10 added 3 commits November 21, 2024 11:24

Merge branch 'main' into upgrade-prometheus-1.x

7281076

# Conflicts: # CHANGELOG.md

Update CHANGELOG

0405e2c

Signed-off-by: Fabio Di Fabio <[email protected]>

Clean verification metadata

539dc5d

Signed-off-by: Fabio Di Fabio <[email protected]>

fab-10 force-pushed the upgrade-prometheus-1.x branch from 15e6e77 to 539dc5d Compare November 21, 2024 11:07

fab-10 added 2 commits November 22, 2024 14:54

Merge branch 'main' into upgrade-prometheus-1.x

5a577d2

Create a unique key for cached metric maps

f6c79cc

Signed-off-by: Fabio Di Fabio <[email protected]>

fab-10 force-pushed the upgrade-prometheus-1.x branch from 3bcfd84 to f6c79cc Compare November 22, 2024 14:59

fab-10 marked this pull request as ready for review November 22, 2024 15:00

Correct other counters that were created as gauges

f343ce5

Signed-off-by: Fabio Di Fabio <[email protected]>

fab-10 force-pushed the upgrade-prometheus-1.x branch from 4c16b53 to 4370075 Compare November 22, 2024 17:52

Rename ExtranalSummary to SuppliedSummary for consistency

306a140

Signed-off-by: Fabio Di Fabio <[email protected]>

fab-10 force-pushed the upgrade-prometheus-1.x branch from 4370075 to 306a140 Compare November 22, 2024 18:09

Matilda-Clerke reviewed Nov 25, 2024

View reviewed changes

fab-10 added 3 commits November 25, 2024 12:32

Merge branch 'main' into upgrade-prometheus-1.x

b76bdee

# Conflicts: # plugin-api/build.gradle

Push up some common collectors methods

66eaa69

Signed-off-by: Fabio Di Fabio <[email protected]>

Merge branch 'main' into upgrade-prometheus-1.x

187ff9b

# Conflicts: # gradle/verification-metadata.xml

fab-10 added 3 commits November 25, 2024 20:16

Revert "Push up some common collectors methods"

fd9f323

This reverts commit 66eaa69. Signed-off-by: Fabio Di Fabio <[email protected]>

Fix tests

a58c2cc

Signed-off-by: Fabio Di Fabio <[email protected]>

Improve javadoc

ffc7907

Signed-off-by: Fabio Di Fabio <[email protected]>

fab-10 force-pushed the upgrade-prometheus-1.x branch 2 times, most recently from 5fc91c4 to 74ae047 Compare November 25, 2024 21:36

Fixes

1c2da80

Signed-off-by: Fabio Di Fabio <[email protected]>

fab-10 force-pushed the upgrade-prometheus-1.x branch from 74ae047 to 1c2da80 Compare November 26, 2024 10:47

Merge branch 'main' into upgrade-prometheus-1.x

4a74167

fab-10 force-pushed the upgrade-prometheus-1.x branch 2 times, most recently from 8144419 to c8b8d1f Compare November 26, 2024 17:10

o.prometheus:prometheus-metrics-bom:1.3.4

2d75538

Signed-off-by: Fabio Di Fabio <[email protected]>

fab-10 force-pushed the upgrade-prometheus-1.x branch from c8b8d1f to 2d75538 Compare November 26, 2024 17:13

macfarla approved these changes Nov 26, 2024

View reviewed changes

siladu reviewed Nov 27, 2024

View reviewed changes

Update CHANGELOG.md

9d7011a

Co-authored-by: Sally MacFarlane <[email protected]> Signed-off-by: Fabio Di Fabio <[email protected]>

Merge branch 'main' into upgrade-prometheus-1.x

6ccc049

# Conflicts: # plugin-api/build.gradle

fab-10 enabled auto-merge (squash) November 27, 2024 10:55

siladu approved these changes Nov 27, 2024

View reviewed changes

fab-10 merged commit 5b2da5a into hyperledger:main Nov 27, 2024
43 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to Prometheus lib 1.x #7880

Migrate to Prometheus lib 1.x #7880

fab-10 commented Nov 13, 2024 •

edited

Loading

Matilda-Clerke left a comment

Matilda-Clerke Nov 25, 2024

Matilda-Clerke Nov 25, 2024

fab-10 Nov 25, 2024

Matilda-Clerke Nov 25, 2024

fab-10 Nov 26, 2024

siladu Nov 27, 2024

fab-10 commented Nov 25, 2024

macfarla left a comment

macfarla Nov 26, 2024

fab-10 Nov 27, 2024

siladu Nov 27, 2024

fab-10 Nov 27, 2024

siladu Nov 27, 2024 •

edited

Loading

fab-10 Nov 27, 2024

siladu Nov 27, 2024

siladu left a comment

fab-10 commented Nov 27, 2024

fab-10 commented Nov 27, 2024

siladu left a comment


		import java.util.function.Supplier;

		/** The interface Labelled supplied summary. */

		- Gauge names are not allowed to end with `total`, therefore the metric `besu_blockchain_difficulty_total` is losing the `_total` suffix
		- The `_created` timestamps are not returned by default, you can set the env var `BESU_OPTS="-Dio.prometheus.exporter.includeCreatedTimestamps=true"` to enabled them

Migrate to Prometheus lib 1.x #7880

Migrate to Prometheus lib 1.x #7880

Conversation

fab-10 commented Nov 13, 2024 • edited Loading

PR description

Fixed Issue(s)

Thanks for sending a pull request! Have you done the following?

Locally, you can run these tests to catch failures early:

Matilda-Clerke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fab-10 commented Nov 25, 2024

macfarla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siladu Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siladu left a comment

Choose a reason for hiding this comment

fab-10 commented Nov 27, 2024

fab-10 commented Nov 27, 2024

siladu left a comment

Choose a reason for hiding this comment

fab-10 commented Nov 13, 2024 •

edited

Loading

siladu Nov 27, 2024 •

edited

Loading