Skip to content

fix(obs): disable profiling export by default and fix Helm env name#2719

Merged
houseme merged 4 commits intomainfrom
fix/fix-issues-2715
Apr 28, 2026
Merged

fix(obs): disable profiling export by default and fix Helm env name#2719
houseme merged 4 commits intomainfrom
fix/fix-issues-2715

Conversation

@houseme
Copy link
Copy Markdown
Contributor

@houseme houseme commented Apr 28, 2026

Type of Change

  • New Feature
  • Bug Fix
  • Documentation
  • Performance Improvement
  • Test/CI
  • Refactor
  • Other:

Related Issues

#2715

Summary of Changes

Problem

When RUSTFS_OBS_ENDPOINT is set but the Pyroscope/OTel collector endpoint is
unreachable, the profiling export background task accumulates data indefinitely.
After 2-3 days of idle (no bucket traffic) this eventually triggers an extremely
large allocation request (memory allocation of 2267059503403440 bytes failed)
and causes the process to abort.

Root cause

DEFAULT_OBS_PROFILING_EXPORT_ENABLED was true, meaning any deployment that
set an OTLP root endpoint automatically activated the Pyroscope profiling export
path on Linux/macOS — even when no Pyroscope endpoint was reachable or intended.
Additionally, the Helm chart emitted RUSTFS_OBS_PROFILING_ENABLED (wrong name)
instead of RUSTFS_OBS_PROFILING_EXPORT_ENABLED, so the intended false value
in the chart was silently ignored and the runtime default (true) took effect.

Changes

  1. crates/config/src/constants/app.rs

    • DEFAULT_OBS_PROFILING_EXPORT_ENABLED: truefalse.
    • Updated constant doc comment to match.
  2. crates/obs/src/config.rs

    • Added backward-compatible read_profiling_export_enabled() that recognises
      the legacy alias RUSTFS_OBS_PROFILING_ENABLED (as emitted by older Helm
      charts) so existing deployments with RUSTFS_OBS_PROFILING_ENABLED=true
      continue to work without a config change.
    • Priority: RUSTFS_OBS_PROFILING_EXPORT_ENABLED > RUSTFS_OBS_PROFILING_ENABLED

      hardcoded default (false).

    • Added 3 targeted tests covering default, legacy alias, and precedence.
  3. helm/rustfs/templates/configmap.yaml

    • Fixed env var name: RUSTFS_OBS_PROFILING_ENABLEDRUSTFS_OBS_PROFILING_EXPORT_ENABLED.
  4. crates/obs/README.md

    • Updated RUSTFS_OBS_PROFILING_EXPORT_ENABLED default column from truefalse.

Migration notes

  • Deployments that relied on profiling being enabled by default must now
    explicitly set RUSTFS_OBS_PROFILING_EXPORT_ENABLED=true (or the legacy
    alias) and ensure a reachable RUSTFS_OBS_PROFILING_ENDPOINT.
  • Deployments using the Helm chart: this fix makes the chart's existing
    profiling.enabled: false default actually take effect (previously it was
    silently ignored due to the wrong env var name).

Checklist

  • I have read and followed the CONTRIBUTING.md guidelines
  • Passed make pre-commit
  • Added/updated necessary tests
  • Documentation updated (if needed)
  • CI/CD passed (if applicable)

Impact

  • Breaking change (compatibility)
    • Backward-compatible: the old alias RUSTFS_OBS_PROFILING_ENABLED is still
      accepted. Only the default changes (off instead of on).
  • Requires doc/config/deployment update
    • Operators who want profiling must now opt in explicitly.
  • Other impact:

Additional Notes

Verification steps used during development:

cargo test -p rustfs-obs profiling_export_
cargo test -p rustfs-obs canonical_profiling_toggle_has_priority_over_legacy_alias

All 3 tests pass.

Copilot AI review requested due to automatic review settings April 28, 2026 10:50
@github-actions
Copy link
Copy Markdown
Contributor

CLA requirements are satisfied for this pull request.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Disables Pyroscope profiling export by default to prevent unbounded profile buffering/allocations when the collector endpoint is unreachable, and aligns Helm/config/docs with the corrected env-var behavior.

Changes:

  • Flip DEFAULT_OBS_PROFILING_EXPORT_ENABLED from true to false.
  • Add backward-compatible env handling for the legacy RUSTFS_OBS_PROFILING_ENABLED toggle (with tests) and update docs.
  • Fix Helm chart to emit the correct env var name for profiling export enablement.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
crates/config/src/constants/app.rs Changes the default profiling export toggle to disabled (false).
crates/obs/src/config.rs Adds legacy env alias support + tests; wires new helper into config extraction.
helm/rustfs/templates/configmap.yaml Renames Helm-emitted env var to RUSTFS_OBS_PROFILING_EXPORT_ENABLED.
crates/obs/README.md Updates documented default for profiling export to false.

Comment thread crates/obs/src/config.rs
Comment thread helm/rustfs/templates/configmap.yaml
Comment thread crates/obs/src/config.rs Outdated
@houseme houseme requested a review from loverustfs April 28, 2026 11:16
Copilot AI review requested due to automatic review settings April 28, 2026 11:29
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

Comment thread crates/obs/README.md
Comment on lines 117 to +120
| `RUSTFS_OBS_TRACES_EXPORT_ENABLED` | `true` | Toggle trace export |
| `RUSTFS_OBS_METRICS_EXPORT_ENABLED` | `true` | Toggle metrics export |
| `RUSTFS_OBS_LOGS_EXPORT_ENABLED` | `true` | Toggle OTLP log export |
| `RUSTFS_OBS_PROFILING_EXPORT_ENABLED` | `true` | Toggle profiling export |
| `RUSTFS_OBS_PROFILING_EXPORT_ENABLED` | `false` | Toggle profiling export |
Copy link

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

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

The README’s “Routing Logic” section earlier states that any OTLP endpoint implies a “Full OTLP/HTTP pipeline (… + profiling)”. With RUSTFS_OBS_PROFILING_EXPORT_ENABLED now defaulting to false, that statement is no longer accurate—profiling export requires an explicit opt-in. Please update the routing logic text to reflect the new default/opt-in behavior to avoid operator confusion.

Copilot uses AI. Check for mistakes.
Comment thread crates/obs/src/config.rs
Comment on lines 289 to +294
logs_export_enabled: Some(get_env_bool(ENV_OBS_LOGS_EXPORT_ENABLED, DEFAULT_OBS_LOGS_EXPORT_ENABLED)),
profiling_export_enabled: Some(get_env_bool(ENV_OBS_PROFILING_EXPORT_ENABLED, DEFAULT_OBS_PROFILING_EXPORT_ENABLED)),
profiling_export_enabled: Some(get_env_bool_with_aliases(
ENV_OBS_PROFILING_EXPORT_ENABLED,
&[LEGACY_ENV_OBS_PROFILING_ENABLED],
DEFAULT_OBS_PROFILING_EXPORT_ENABLED,
)),
Copy link

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

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

The PR description mentions adding a read_profiling_export_enabled() helper, but the implementation here directly calls get_env_bool_with_aliases(...) inline. Either update the PR description to match the actual implementation or add the helper as described (if that was the intent) to keep the change log accurate.

Copilot uses AI. Check for mistakes.
@houseme houseme added this pull request to the merge queue Apr 28, 2026
Merged via the queue into main with commit c4d5c5c Apr 28, 2026
8 checks passed
@houseme houseme deleted the fix/fix-issues-2715 branch April 28, 2026 12:11
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.

4 participants