Upgrade Prometheus and Opentelemetry dependencies#6422
Merged
fab-10 merged 9 commits intobesu-eth:mainfrom Jan 30, 2024
Merged
Conversation
|
365f5e9 to
a818871
Compare
fd2aab2 to
66bdc6b
Compare
pinges
reviewed
Jan 22, 2024
Contributor
pinges
left a comment
There was a problem hiding this comment.
1 Question and minor improvement suggestions
| final String unPrefixedName = | ||
| metricName.startsWith(prefix) ? metricName.substring(prefix.length()) : metricName; | ||
| return totalSuffixedCounters.contains(unPrefixedName + "_total") | ||
| ? unPrefixedName + "_total" |
Contributor
There was a problem hiding this comment.
does this not end up with two "_total" suffixes?
Contributor
Author
There was a problem hiding this comment.
I understand this could be a little misleading, but internally Prometheus stores the counter name without the _total suffix, and if a name already has the suffix, it is removed, so when converting back from Prometheus to Besu names, if we remember that the name already had the suffix, we need to readd it.
e8470b9 to
3af8312
Compare
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
[skip-ci] Co-authored-by: Stefan Pingel <16143240+pinges@users.noreply.github.com> Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
3af8312 to
8936877
Compare
macfarla
reviewed
Jan 29, 2024
| ## 24.1.2-SNAPSHOT | ||
|
|
||
| ### Breaking Changes | ||
| - Following the OpenMetrics convention, the updated Prometheus client adds the `_total` suffix to every metrics of type counter, with the effect that some existing metrics have been renamed to have this suffix. If you are using the official Besu Grafana dashboard [(available here)](https://grafana.com/grafana/dashboards/16455-besu-full/), just update it to the latest revision, that accepts the old and the new name of the affected metrics. If you have a custom dashboards or use the metrics in other ways, then you need to manually update it to support the new naming. |
Contributor
There was a problem hiding this comment.
Suggested change
| - Following the OpenMetrics convention, the updated Prometheus client adds the `_total` suffix to every metrics of type counter, with the effect that some existing metrics have been renamed to have this suffix. If you are using the official Besu Grafana dashboard [(available here)](https://grafana.com/grafana/dashboards/16455-besu-full/), just update it to the latest revision, that accepts the old and the new name of the affected metrics. If you have a custom dashboards or use the metrics in other ways, then you need to manually update it to support the new naming. | |
| - Following the OpenMetrics convention, the updated Prometheus client adds the `_total` suffix to every metrics of type counter, with the effect that some existing metrics have been renamed to have this suffix. If you are using the official Besu Grafana dashboard [(available here)](https://grafana.com/grafana/dashboards/16455-besu-full/), just update it to the latest revision, that accepts the old and the new name of the affected metrics. If you have a custom dashboard or use the metrics in other ways, then you need to manually update it to support the new naming. |
CHANGELOG.md
Outdated
| - `--Xfilter-on-enr-fork-id` has been removed. To disable the feature use `--filter-on-enr-fork-id=false`. | ||
| - The time that can be spent selecting transactions during block creation is not capped at 5 seconds for PoS and PoW networks, and for PoA networks, at 75% of the block period specified in the genesis, this to prevent possible DoS in case a single transaction is taking too long to execute, and to have a stable block production rate, but it could be a breaking change if an existing network used to have transactions that takes more time to executed that the newly introduced limit, if it is mandatory for these network to keep processing these long processing transaction, then the default value of `block-txs-selection-max-time` or `poa-block-txs-selection-max-time` needs to be tuned accordingly. | ||
| - `--Xfilter-on-enr-fork-id` has been removed. To disable the feature use `--filter-on-enr-fork-id=false`. | ||
| - The time that can be spent selecting transactions during block creation is not capped at 5 seconds for PoS and PoW networks, and for PoA networks, at 75% of the block period specified in the genesis. This is to prevent possible DoS attacks in case a single transaction is taking too long to execute, and to have a stable block production rate. This could be a breaking change if an existing network needs to accept transactions that take more time to executed than the newly introduced limit. If it is mandatory for these networks to keep processing these long processing transaction, then the default value of `block-txs-selection-max-time` or `poa-block-txs-selection-max-time` needs to be tuned accordingly. |
Contributor
There was a problem hiding this comment.
Suggested change
| - The time that can be spent selecting transactions during block creation is not capped at 5 seconds for PoS and PoW networks, and for PoA networks, at 75% of the block period specified in the genesis. This is to prevent possible DoS attacks in case a single transaction is taking too long to execute, and to have a stable block production rate. This could be a breaking change if an existing network needs to accept transactions that take more time to executed than the newly introduced limit. If it is mandatory for these networks to keep processing these long processing transaction, then the default value of `block-txs-selection-max-time` or `poa-block-txs-selection-max-time` needs to be tuned accordingly. | |
| - The time that can be spent selecting transactions during block creation is not capped at 5 seconds for PoS and PoW networks, and for PoA networks, at 75% of the block period specified in the genesis. This is to prevent possible DoS attacks in case a single transaction is taking too long to execute, and to have a stable block production rate. This could be a breaking change if an existing network needs to accept transactions that take more time to execute than the newly introduced limit. If it is mandatory for these networks to keep processing these long processing transaction, then the default value of `block-txs-selection-max-time` or `poa-block-txs-selection-max-time` needs to be tuned accordingly. |
Contributor
|
FYI @non-fungible-nelson these debug level logs still occurring even with this upgrade -> |
macfarla
approved these changes
Jan 30, 2024
jflo
pushed a commit
to jflo/besu
that referenced
this pull request
Jan 30, 2024
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net> Co-authored-by: Stefan Pingel <16143240+pinges@users.noreply.github.com> Signed-off-by: Justin Florentine <justin+github@florentine.us>
2 tasks
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
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.
PR description
Upgrade Prometheus and Opentelemetry dependencies, and adjust the code accordingly.
Prometheus simpleclient upgrade introduces some changed, to follow the OpenMetrics specification, like adding the
_totalsuffix to every counters that haven't it, and adding a_createdsuffixed metric to some types, while the latter is not an issue, the_totalsuffix is a breaking change, and requires an updated version of our official Grafana dashboard, that has been already published here, and is compatible with both metrics names.Using the metrics by other means require a change to the tooling, for this a breaking change section has been added.
Fixed Issue(s)