feat: add helpers for creating feature usage counter#94
Conversation
Signed-off-by: Buqian Zheng <zhengbuqian@gmail.com>
Signed-off-by: Buqian Zheng <zhengbuqian@gmail.com>
| class alignas(64) FeatureReporter { | ||
| public: | ||
| // Prefer the predeclared static reporters below on hot paths. | ||
| explicit FeatureReporter(std::string_view name); |
There was a problem hiding this comment.
FeatureReporter exposes a public constructor and prometheus-cpp's Family::Add coalesces identical label sets, so constructing a second reporter with an existing feature name (e.g. FeatureReporter{kHybridSearch} alongside the HybridSearch() singleton) hands back the same shared Counter while giving the new instance its own next_allowed_nanos_ throttle. The ~once-per-hour invariant then holds per instance rather than per counter, so a feature can be recorded up to once per hour per duplicate reporter. Funnel construction through the singletons (e.g. make the constructor private) so each counter has exactly one throttle.
| bool | ||
| MetricsContain(const std::string& metrics, const std::string& feature, const std::string& value) { | ||
| const auto expected = "milvus_feature_report_total{feature=\"" + feature + "\",source=\"cpp\"} " + value; | ||
| return metrics.find(expected) != std::string::npos; |
There was a problem hiding this comment.
MetricsContain matches with an unanchored find(), and expected ends right after the counter value with no trailing newline. The feature name is actually bounded by the surrounding ",source="cpp"}, so the reviewer's name-substring example does not apply here; the unbounded part is the value, so asserting a value of "2" would also pass against an actual value of "20" or "25". Append "\n" to expected (or match the whole line) so the count is checked exactly.
Signed-off-by: Buqian Zheng <zhengbuqian@gmail.com>
| bool | ||
| RecordAtForTest(std::chrono::steady_clock::time_point now); | ||
|
|
||
| void |
There was a problem hiding this comment.
RecordAtForTest() and ResetForTest() are declared under public: (lines 43-47), exposing test-only seams — clearing the throttle gate and injecting arbitrary timestamps — on the production reporter's API. Since the class already friends its factory functions, a friend-based test seam would keep these off the public surface. No in-repo production caller exists today, so this is an encapsulation concern rather than a live bug.
| FeatureReportFamily() { | ||
| static auto& family = prometheus::BuildCounter() | ||
| .Name("milvus_feature_report_total") | ||
| .Help("Count of throttled feature reports.") |
There was a problem hiding this comment.
The counter increments only on the path that passes the throttle, so it measures emitted reports, not suppressed ones. Help("Count of throttled feature reports.") reads naturally as a count of dropped/rate-limited reports — the inverse of what is measured. Reword to something like "Count of feature reports emitted."
| ASSERT_FALSE(reporter.RecordAtForTest(now + std::chrono::minutes(1))); | ||
| ASSERT_TRUE(reporter.RecordAtForTest(now + std::chrono::hours(1))); | ||
|
|
||
| ASSERT_TRUE(MetricsContain(getPrometheusClient().GetMetrics(), std::string(reporter.Name()), "2")); |
There was a problem hiding this comment.
The assertion hard-codes the absolute cumulative counter value — MetricsContain(..., reporter.Name(), "2") at line 41 (and "1" at line 65) — but the Prometheus counter is process-global and monotonic and ResetForTest() clears only the throttle gate, not the counter. Under --gtest_repeat=2, shuffled re-runs, or any later test that also records HybridSearch/PartitionKey, the value climbs to 4/2 and the assertion fails on otherwise-correct code. Read the counter before and after and assert the delta (+2/+1) instead of an absolute string; the unanchored find in MetricsContain coupling to integer formatting is a secondary concern.
Signed-off-by: Buqian Zheng <zhengbuqian@gmail.com>
2 participants
No description provided.