Skip to content

feat: document metrics, disable most libp2p metrics by default#5457

Merged
lemmih merged 11 commits intomainfrom
lemmih/metrics-docs
Mar 26, 2025
Merged

feat: document metrics, disable most libp2p metrics by default#5457
lemmih merged 11 commits intomainfrom
lemmih/metrics-docs

Conversation

@lemmih
Copy link
Contributor

@lemmih lemmih commented Mar 25, 2025

Summary of changes

Changes introduced in this pull request:

  • Put libp2p metrics behind a FOREST_LIBP2P_METRICS env variable.
  • Libp2p bandwidth is still reported in the metrics. I feel this is the most useful of the libp2p metrics.
  • Add reference documentation for all foreign-native metrics (and a single libp2p native bandwidth metric).
  • Omitting the libp2p metrics drastically reduces the volume of metrics, solving High-cardinality Prometheus metrics #3509.

Reference issue to close (if applicable)

Closes #5196 #3509

Other information and links

Change checklist

  • I have performed a self-review of my own code,
  • I have made corresponding changes to the documentation. All new code adheres to the team's documentation standards,
  • I have added tests that prove my fix is effective or that my feature works (if possible),
  • I have made sure the CHANGELOG is up-to-date. All user-facing changes should be reflected in this document.

@lemmih lemmih marked this pull request as ready for review March 26, 2025 08:40
@lemmih lemmih requested a review from a team as a code owner March 26, 2025 08:40
@lemmih lemmih requested review from LesnyRumcajs and elmattic and removed request for a team March 26, 2025 08:40
@lemmih
Copy link
Contributor Author

lemmih commented Mar 26, 2025

AFAIK, the removed metrics are not used by the infra team.

| `FOREST_TEST_RNG_FIXED_SEED` | non-negative integer | empty | 0 | Override RNG with a reproducible one seeded by the value. This should never be used out of test context for security. |
| `RUST_LOG` | string | empty | `debug,forest_libp2p::service=info` | Allows for log level customization. |
| `FOREST_IGNORE_DRAND` | 1 or true | empty | 1 | Ignore Drand validation. |
| `FOREST_LIBP2P_METRICS` | 1 or true | empty | 1 | Include `libp2p` metrics in Forest's Prometheus output. |
Copy link
Member

Choose a reason for hiding this comment

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

Looking at other variables (at least newer ones), it seems the convention is to have a verb to make it more obvious,e.g., _OPT_OUT (here rather opt-in), _ENABLED etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added _ENABLED

}

const LIBP2P_METRICS: &str = "FOREST_LIBP2P_METRICS";
fn enable_libp2p_metrics() -> bool {
Copy link
Member

Choose a reason for hiding this comment

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

The name suggests the metrics are going to get enabled.

Suggested change
fn enable_libp2p_metrics() -> bool {
fn libp2p_metrics_enabled() -> bool {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

LesnyRumcajs
LesnyRumcajs previously approved these changes Mar 26, 2025
@LesnyRumcajs LesnyRumcajs dismissed their stale review March 26, 2025 10:00

no green checkmark!

@lemmih lemmih enabled auto-merge March 26, 2025 10:30
@lemmih lemmih added this pull request to the merge queue Mar 26, 2025
Merged via the queue into main with commit fcb8c00 Mar 26, 2025
42 checks passed
@lemmih lemmih deleted the lemmih/metrics-docs branch March 26, 2025 10:53
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.

Document Prometheus metrics

3 participants