PR Review - Prometheus #18279

📋 PR Summary

Title: Start timestamp synthesis for cumulative metrics PR Link: prometheus/prometheus#18279 Author: Ridwan Sharif Branch: [branch] → main

What This PR Does

This PR introduces a mechanism to synthesize start timestamps for cumulative metrics (Counters, Histograms) when they are missing in the exposition (e.g., OpenMetrics without start timestamps). It uses a "subtract initial" approach (similar to OpenTelemetry) to establish a baseline and then produce delta values or synthesized start times for TSDB ingestion, improving accuracy for rate queries in short-lived environments.

Key Changes

• Added st-synthesis feature flag in cmd/prometheus/main.go.
• Implemented stCache in scrape/st_synthesis.go to track state for counters and histograms.
• Integrated synthesis logic into scrapeLoop in scrape/scrape_append_v2.go.
• Added unit and integration tests in scrape/st_synthesis_test.go and scrape/scrape_test.go.

Key Discussions

I evaluated the code independently based on the fetched diff. No external discussion context was available due to environment limitations.

---

📖 Review Guidelines

Based on the type of changes in this PR, reviewers should focus on:

Area	Priority	What to Check
scrape/st_synthesis.go	🔴 High	Correctness of reset detection and histogram subtraction logic. Precision loss in float conversion.
scrape/scrape_append_v2.go	🔴 High	Integration with scrape loop, correct handling of missing start times, and staleness tracking.
Tests	🟡 Medium	Coverage of edge cases (resets, first scrape behavior).

---

🚨 Issues Found

Issues are sorted by severity (Critical → High → Medium → Low → Nitpick).

🔴 Critical (Must Fix)

None identified.

🟠 High (Should Fix)

None identified.

🟡 Medium (Consider Fixing)

Precision Loss in Integer Histogram Subtraction

• File with exact line number: scrape/st_synthesis.go:L831 (approximate based on diff)
• Problem: The implementation converts integer histograms to float histograms to perform subtraction and then casts back to integers.
• Impact: Potential precision loss or rounding errors in histogram bucket counts.
• Suggestion: Implement native subtraction for integer histograms if possible, as noted in the author's TODO on line 831.

🟢 Low (Nice to Have)

First Sample Skipped for Anchoring

• File with exact line number: scrape/st_synthesis.go:L785 (and similar for histograms)
• Problem: The first sample of a new series is used to anchor the start time and is skipped (not appended to TSDB).
• Impact: New series won't have data points on the first scrape. This is a design choice but should be explicitly documented for users.
• Suggestion: Ensure this behavior is documented in the feature flag description or user guide.

💭 Nitpicks

None identified.

---

🧪 Test Coverage Analysis

Test Changes in This PR

Test File	Type	Coverage
scrape/st_synthesis_test.go	unit	Tests stCache methods with various data (counters, histograms) and resets.
scrape/scrape_test.go	integration	Tests synthesis integration in the scrape loop.

Coverage Assessment

• New code tested: Yes - stCache methods are covered.
• Edge cases covered: Partial - Resets are tested, but complex histogram edge cases might need more coverage.
• Regression tests: N/A - This is a new feature.

Recommended Additional Tests

1. Test with very small increments to check for float precision issues in histogram conversion.

---

❓ Questions & Uncertainties

1. Design choice for skipping first sample: Is there any alternative that allows appending the first sample (e.g., assuming it started at 0 at prevTs if available)?
2. Precision impact: How significant is the precision loss in practice for typical histogram use cases?

---

✅ Review Verdict

Aspect	Status	Notes
Code Quality	🟢	Clean implementation.
Security	🟢	No issues found.
Performance	🟢	Benchmarks added, seems fine.
Test Coverage	🟢	Good unit and integration tests.
Documentation	🟡	Need to ensure the first-sample-skip behavior is clear to users.

Overall: APPROVE with suggestions (Experimental feature, monitoring precision is advised).

---