[Bugfix]: Fix Promethus spec decode counter sum-of-sums #15415
Conversation
|
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels. Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can either: Add 🚀 |
The Prometheus spec decode counters (draft/accepted/emitted token counts) are incremented by the values in spec_decode_metrics. However, those values are aggregates since startup. Therefore, the Prometheus counters are effectively a sum-of-sums instead of just a sum. If a high-traffic vLLM is left on for a few hours those counters start to suggest absurdly high values like a TPS in the tens of millions. Signed-off-by: Adam Lugowski <adam.lugowski@parasail.io>
| @@ -80,6 +80,7 @@ def __init__(self, local_interval: float, vllm_config: VllmConfig) -> None: | |||
| self.last_local_log = time.time() | |||
| self.local_interval = local_interval | |||
| self.spec_decode_metrics: Optional[SpecDecodeWorkerMetrics] = None | |||
| self.last_spec_decode_metrics: Optional[SpecDecodeWorkerMetrics] = None | |||
There was a problem hiding this comment.
This state could be on PrometheusStatLogger since it's not used by LoggingStatLogger
| @@ -669,20 +670,37 @@ def log(self, stats: Stats): | |||
| if local_interval_elapsed(stats.now, self.last_local_log, | |||
| self.local_interval): | |||
| if self.spec_decode_metrics is not None: | |||
| # The counters in self.spec_decode_metrics are aggregates. | |||
| # The Prometheus Counters must be incremented with deltas. | |||
| # Keep track of the previous value so we can compute deltas. | |||
There was a problem hiding this comment.
I'd be inclined to do something like this:
def log_counter_from_cumulative(metric, cumulative, previous):
self._log_counter(metric, cumulative - previous)
The method name helps the code to be self-documenting, requiring less comments
and then:
self._log_from_cumulative(
self.metrics.counter_spec_decode_num_accepted_tokens,
self.spec_decode_metrics.accepted_tokens,
self.spec_decode_prev_num_accepted)
self.spec_decode_prev_num_accepted = self.spec_decode_metrics.accepted_tokens
since we don't need all of SpecDecodeWorkerMetrics
The Prometheus spec decode counters (draft/accepted/emitted token counts) are incremented by the values in
spec_decode_metrics. However, those values are aggregates since startup. Therefore, the Prometheus counters are effectively a sum-of-sums instead of just a sum.If a high-traffic vLLM is left on for a few hours those counters start to suggest absurdly high values like a TPS in the tens of millions.
The values in
spec_decode_metricsare used by other stat reporters such as the command-line logger, so those cannot be converted to only store deltas. Instead this PR modifiesPrometheusStatLoggerto compute the deltas itself and properly increment the PrometheusCountermetrics.