Decouple flag eval metrics from CoreMetricCollector - #12666
sarahchen6 wants to merge 5 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
PerfectSlayer
left a comment
There was a problem hiding this comment.
💭 thought: There might be a dependency issue. The FFE bootstrap module now has dependency to the metrics api. (Which might relocated it into the bootstrap? 🤔 )
Same with the telemetry module which gains a dependency to a product (FFE).
I would stick to the original design having a collector in internal-api module (not great but that's the current design pattern for debugger, iast, llmobs, WAF, etc...) to avoid adding such dependencies. WDYT?
Makes sense! I think this should follow other product collector formats as well as minimize shared module dependencies on products... I'll refactor |
236d499 to
4a854c3
Compare
What Does This Do
This PR moves flag evaluation recording out of
CoreMetricCollectorand into product-ownedFlagEvaluationMetricsusing:products:metrics:metrics-api’sAccumulatorfor fixed counters by removingCoreMetricCollector.count()and instead adding a periodic telemetry adapter, preserving metric names, tags, namespace, and type.Motivation
#11639 has the flag evaluation writer push metrics directly into
CoreMetricCollector; however, this breaks the design ofCoreMetricCollectorwhich is to drain metrics for delivery (not capture / push metrics). It also coupled product-specific metric recording to a shared internal telemetry component - which we want to avoid.The changes in this PR restore the ownership pattern for flag evaluation based on existing usages such as by
SpanMetricsImplandBaggageMetrics. Now feature flagging owns its counters and telemetry drains them for delivery.Additional Notes
With these changes, truncation counts are collectible without waiting for an EVP writer flush, and collection timestamps follow telemetry’s schedule. Queue saturation keeps pending snapshots for later collection, which is similar to
StatsDCountReporter.Besides this telemetry collection timing and retention, flag evaluation results and event payloads are unchanged.Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issueJira ticket: [PROJ-IDENT]