Skip to content

Add ability for metris to be disabled - #215

Merged
allada merged 1 commit into
masterfrom
disableable-metrics
Jul 24, 2023
Merged

allada merged 1 commit into
masterfrom
disableable-metrics

Conversation

@allada

@allada allada commented Jul 20, 2023 •

Copy link
Copy Markdown
Contributor

Adds the ability to disable metrics through env via TURBO_CACHE_DISABLE_METRICS or through config file.


This change is Reviewable

@allada allada left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

+@chrisstaite-menlo

Reviewable status: 0 of 6 files reviewed, all discussions resolved (waiting on @chrisstaite-menlo)

@allada
allada force-pushed the disableable-metrics branch 2 times, most recently from d3b1258 to 86bcb42 Compare July 21, 2023 03:09
@chrisstaite-menlo

Copy link
Copy Markdown
Collaborator

I wonder, should metrics always be disabled when there's no service and enabled when there's at least one?

@chrisstaite-menlo

Copy link
Copy Markdown
Collaborator

cas/cas_main.rs line 432 at r1 (raw file):

        metrics_enabled = false;
    }
    let runtime = tokio::runtime::Builder::new_multi_thread()

I think if there's no server specified then metrics should be disabled too.

@chrisstaite-menlo chrisstaite-menlo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @allada)

Adds the ability to disable metrics through env via
TURBO_CACHE_DISABLE_METRICS or through config file.
@allada
allada force-pushed the disableable-metrics branch from 86bcb42 to 6cc2b6d Compare July 23, 2023 19:41

@allada allada left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

Reviewable status: 4 of 6 files reviewed, 1 unresolved discussion (waiting on @chrisstaite-menlo)


cas/cas_main.rs line 432 at r1 (raw file):

Previously, chrisstaite-menlo (Chris Staite) wrote…

I think if there's no server specified then metrics should be disabled too.

Done.

@chrisstaite-menlo chrisstaite-menlo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @allada)

@allada
allada merged commit 875b3ca into master Jul 24, 2023
@allada
allada deleted the disableable-metrics branch July 24, 2023 15:59
rejuvenile added a commit to rejuvenile/nativelink that referenced this pull request Aug 19, 2026
Two doc-only edits in `api_worker_scheduler.rs` addressing reviewer
findings on the TraceMachina#214 BIS replay buffer cap:

* `bis_replay_buffer_overflow_drops` doc comment claimed the counter
  is "the operator-visible signal." Reality: `SchedulerMetrics` lacks
  `#[derive(MetricsComponent)]` so the counter is internal-only.
  Operator visibility today is the per-(endpoint, broadcast_id)
  `warn!` log; the counter is reachable via tests / debug only.
  Filed TraceMachina#231 to wire the metrics exporter.

* `BIS_REPLAY_BUFFER_MAX_CHUNKS` doc cited "16 GiB per worker" based
  on wire-encoded chunk size. Decoded heap representation is closer
  to ~38 GiB at the cap (96 B/Digest × 4096 × 100K plus prost
  framing). With 80 GiB MemoryMax, single-misbehaving-worker
  defense holds; two simultaneous misbehavers get tight. Comment
  now points to TraceMachina#215 + TraceMachina#216 for full defense-in-depth.

Pure documentation; no behavior change. HEAD `b9befaf1` is unchanged
in semantics.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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