#620 Monitoring – OTEL, Grafana, graphs and alerting - #753
Open
alisku wants to merge 4 commits into
Open
Conversation
- Introduced metrics for tracking cluster leader presence, node leadership status, and partition counts. - Added metrics for job lifetime, process instance duration, and message correlation failures. - Enhanced the monitoring stack with Prometheus configuration for alerting.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughZenBPM adds OpenTelemetry tracing and metrics, cluster readiness endpoints, Prometheus and Alertmanager configuration, Grafana dashboards, and observability reference documentation. ChangesZenBPM observability
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/bpmn/jobs_api.go (1)
89-97: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
recordJobLifetime(..., "failed")also fires when the job was caught by an error boundary/subprocess, not genuinely failed.This defer only checks
retErr == nil, butJobFailByKeyreturnsnilearly both when the job is redirected to an error-boundary event (line ~111) or an error event-subprocess (line ~117), and when it's actually failed viafailJobWithIncident(line ~127). Since the same defer fires on everynil-returning path,engine.metrics.JobsFailedand the newengine.recordJobLifetime(ctx, job, "failed")both misclassify caught/handled jobs as "failed", skewing the outcome-labeled telemetry this PR introduces.Consider tracking whether the job was actually handled by a catch target and skipping the "failed" metrics in that case, or moving the metric calls to sit alongside the genuine
failJobWithIncidentsuccess path instead of the blanket defer.🐛 Sketch of a fix
+ handled := false defer func() { - if retErr == nil { + if retErr == nil && !handled { engine.metrics.JobsFailed.Add(ctx, 1, metric.WithAttributes( attribute.String("type", job.Type), attribute.Bool("internal", false), )) engine.recordJobLifetime(ctx, job, "failed") } }() if errorCode != nil { target, err := engine.findErrorCatchTarget(ctx, &batch, instance, job.Token, errorCode) if err != nil { return err } if target != nil { switch { case target.boundary != nil: if handledBoundary, err := engine.processBoundaryErrorEvent(ctx, &batch, job, instance, target.boundary, variables); err != nil { return err } else if handledBoundary { + handled = true return nil } case target.eventSubprocess != nil: if handledSub, err := engine.processErrorEventSubprocessForJob(ctx, &batch, job, target.eventSubprocess, variables); err != nil { return err } else if handledSub { + handled = true return nil } } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/bpmn/jobs_api.go` around lines 89 - 97, Update JobFailByKey so caught jobs are not classified as failed: distinguish error-boundary and error-event-subprocess handling from the genuine failJobWithIncident path, and emit engine.metrics.JobsFailed and engine.recordJobLifetime(ctx, job, "failed") only for actual failures. Remove or adjust the blanket retErr-based defer while preserving existing handling behavior.
🧹 Nitpick comments (3)
pkg/bpmn/engine_batch.go (1)
148-165: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated save+metric-scheduling pattern across three methods.
WriteTokenIncident,WriteMessageIncident, andSaveIncidenteach independently implement "save incident, then on success append apostFlushActionscallback callingrecordIncidentMetric";SaveTimerrepeats the analogous shape forrecordTimerMetric. Consider extracting a small shared helper (e.g.saveIncidentWithMetric(ctx, incident) error) to reduce duplication and keep the persistence+metric contract in one place.Also applies to: 166-201, 219-227, 257-265
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/bpmn/engine_batch.go` around lines 148 - 165, Extract the repeated incident persistence and metric scheduling logic from WriteTokenIncident, WriteMessageIncident, and SaveIncident into a shared EngineBatch helper such as saveIncidentWithMetric(ctx, incident) error. Have the helper save the incident, log and return persistence errors, and append the postFlushActions callback that invokes recordIncidentMetric only after a successful save; update each caller to use it while preserving existing behavior. Apply the same deduplication principle to SaveTimer for recordTimerMetric only if a corresponding shared timer helper is already supported by the surrounding code.pkg/otel/traces.go (1)
4-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse Go initialisms and add the missing doc comment.
AttributeProcessId,AttributeElementId,AttributeDecisionId, andAttributeDrdIdshould useID, andAttributeProcessInstanceKeyneeds its own doc comment on this exported API.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/otel/traces.go` around lines 4 - 27, Update the exported constants AttributeProcessId, AttributeElementId, AttributeDecisionId, and AttributeDrdId to use Go initialism naming with ID, preserving their existing attribute values. Add a dedicated doc comment immediately before AttributeProcessInstanceKey describing its purpose.Source: Linters/SAST tools
scripts/prometheus.yml (1)
1-11: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAlert latency:
evaluation_intervalunset defaults to 1m, underminingfor: 30salerts.Several critical alerts (
NoClusterLeader,NoPartitionLeader) usefor: 30s, but Prometheus rule evaluation defaults toevaluation_interval: 1mwhen not set (it doesn't inheritscrape_interval). Detection latency for these leader-loss alerts will effectively be governed by the 1m evaluation cadence rather than 30s.⏱️ Suggested fix
global: scrape_interval: 5s + evaluation_interval: 5s🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/prometheus.yml` around lines 1 - 11, Set the Prometheus global evaluation_interval alongside scrape_interval in the configuration, using a cadence no longer than the 30s alert duration so NoClusterLeader and NoPartitionLeader rules are evaluated promptly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/prometheus-rules.yml`:
- Around line 24-31: Update the dashboard panel in
scripts/grafana_provisioning/dashboards/zenbpm/cluster.json at lines 37-43 to
use max by(partition) (partition_has_leader), matching the NoPartitionLeader
alert expression. Leave scripts/prometheus-rules.yml lines 24-31 unchanged
because its NoPartitionLeader expression already uses the required aggregation.
---
Outside diff comments:
In `@pkg/bpmn/jobs_api.go`:
- Around line 89-97: Update JobFailByKey so caught jobs are not classified as
failed: distinguish error-boundary and error-event-subprocess handling from the
genuine failJobWithIncident path, and emit engine.metrics.JobsFailed and
engine.recordJobLifetime(ctx, job, "failed") only for actual failures. Remove or
adjust the blanket retErr-based defer while preserving existing handling
behavior.
---
Nitpick comments:
In `@pkg/bpmn/engine_batch.go`:
- Around line 148-165: Extract the repeated incident persistence and metric
scheduling logic from WriteTokenIncident, WriteMessageIncident, and SaveIncident
into a shared EngineBatch helper such as saveIncidentWithMetric(ctx, incident)
error. Have the helper save the incident, log and return persistence errors, and
append the postFlushActions callback that invokes recordIncidentMetric only
after a successful save; update each caller to use it while preserving existing
behavior. Apply the same deduplication principle to SaveTimer for
recordTimerMetric only if a corresponding shared timer helper is already
supported by the surrounding code.
In `@pkg/otel/traces.go`:
- Around line 4-27: Update the exported constants AttributeProcessId,
AttributeElementId, AttributeDecisionId, and AttributeDrdId to use Go initialism
naming with ID, preserving their existing attribute values. Add a dedicated doc
comment immediately before AttributeProcessInstanceKey describing its purpose.
In `@scripts/prometheus.yml`:
- Around line 1-11: Set the Prometheus global evaluation_interval alongside
scrape_interval in the configuration, using a cadence no longer than the 30s
alert duration so NoClusterLeader and NoPartitionLeader rules are evaluated
promptly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0b9a7b5a-1a23-4ec1-b08e-35bb1f4e7e0b
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (36)
Makefiledocs/reference/observability.mdgo.modinternal/cluster/jobmanager/otel.gointernal/cluster/jobmanager/server.gointernal/cluster/node.gointernal/cluster/partition/partition.gointernal/cluster/partition/partition_persistence.gointernal/cluster/store/metrics.gointernal/cluster/store/store.gointernal/cluster/store/store_helper.gointernal/config/config.gointernal/otel/metrics.gointernal/otel/tracer.gointernal/otel/tracer_test.gointernal/rest/health_test.gointernal/rest/server.gopkg/bpmn/engine.gopkg/bpmn/engine_api.gopkg/bpmn/engine_batch.gopkg/bpmn/engine_metrics_test.gopkg/bpmn/events_api.gopkg/bpmn/jobs_api.gopkg/bpmn/start_event_instance_creation_handler.gopkg/dmn/dmn_engine.gopkg/dmn/dmn_multiline_test.gopkg/otel/metrics.gopkg/otel/traces.goscripts/alertmanager.ymlscripts/grafana_provisioning/dashboards/zenbpm/cluster.jsonscripts/grafana_provisioning/dashboards/zenbpm/host.jsonscripts/grafana_provisioning/dashboards/zenbpm/incidents.jsonscripts/grafana_provisioning/dashboards/zenbpm/latency.jsonscripts/grafana_provisioning/dashboards/zenbpm/storage.jsonscripts/prometheus-rules.ymlscripts/prometheus.yml
- fixes for CI auto AI findings: update OpenTelemetry attribute names for consistency and improve Grafana query for partition leaders
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/grafana_provisioning/dashboards/zenbpm/cluster.json (2)
50-51: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not sum the per-node leader-change counter.
partition_leader_changesis recorded as an event count observed by each node.sum by(partition)counts the same event once per node. Select one authoritative series, or usemax by(partition)when every node observes the same events.Proposed query change
- "expr": "sum by(partition) (increase(partition_leader_changes_total[5m]))" + "expr": "max by(partition) (increase(partition_leader_changes_total[5m]))"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/grafana_provisioning/dashboards/zenbpm/cluster.json` around lines 50 - 51, Update the dashboard target query for partition leader changes to avoid summing duplicated per-node observations; replace sum by(partition) with an authoritative single series or max by(partition), while preserving the existing increase window, partition grouping, legendFormat, and refId.
21-22: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse current Prometheus samples for status panels.
If metric collection stops,
lastNotNullcan display an oldLEADER OKvalue across the one-hour range. Set"instant": trueon both stat targets and configure a distinct no-data display.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/grafana_provisioning/dashboards/zenbpm/cluster.json` around lines 21 - 22, Update both status panel targets in the dashboard JSON to use current Prometheus samples by setting instant querying on each target, and configure a distinct no-data display for the stat panels. Preserve the existing expressions, legends, and references while applying the same behavior consistently to both targets.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/grafana_provisioning/dashboards/zenbpm/cluster.json`:
- Line 40: Update the dashboard panel using the partition_has_leader target so
expected partitions absent from cluster state remain visible and display NO
LEADER. Compare cluster_partitions with cluster_desired_partitions, or otherwise
add zero-valued series for missing expected partitions, while preserving the
existing leader status display for present partitions.
---
Outside diff comments:
In `@scripts/grafana_provisioning/dashboards/zenbpm/cluster.json`:
- Around line 50-51: Update the dashboard target query for partition leader
changes to avoid summing duplicated per-node observations; replace sum
by(partition) with an authoritative single series or max by(partition), while
preserving the existing increase window, partition grouping, legendFormat, and
refId.
- Around line 21-22: Update both status panel targets in the dashboard JSON to
use current Prometheus samples by setting instant querying on each target, and
configure a distinct no-data display for the stat panels. Preserve the existing
expressions, legends, and references while applying the same behavior
consistently to both targets.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 46da2e47-8920-4a4d-9918-914db1e87154
📒 Files selected for processing (11)
internal/cluster/node.gointernal/cluster/partition/partition_persistence.gointernal/cluster/store/metrics.gointernal/rest/server.gopkg/bpmn/engine.gopkg/bpmn/engine_api.gopkg/bpmn/multi_instance.gopkg/bpmn/sub_process.gopkg/dmn/dmn_engine.gopkg/otel/traces.goscripts/grafana_provisioning/dashboards/zenbpm/cluster.json
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/cluster/node.go
- pkg/bpmn/engine.go
- internal/cluster/partition/partition_persistence.go
- pkg/dmn/dmn_engine.go
- internal/cluster/store/metrics.go
- pkg/bpmn/engine_api.go
- feat(monitoring): add partition deficit metric to Grafana dashboard
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a complete local-first observability stack for ZenBPM (Prometheus + Alertmanager + Grafana + OTEL tracing/metrics), alongside new health probes and a documented metrics/alerting catalog.
Changes:
- Introduces Prometheus alert rules + Alertmanager config and multiple Grafana dashboards provisioned for ZenBPM metrics.
- Expands OTEL instrumentation across BPMN/DMN engines, cluster/partition/raft, and rqlite persistence (plus runtime metrics).
- Adds liveness/readiness health endpoints and corresponding tests + observability reference documentation.
Reviewed changes
Copilot reviewed 38 out of 39 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/prometheus.yml | Enables rule loading and Alertmanager integration for the local Prometheus container. |
| scripts/prometheus-rules.yml | Adds technical + business Prometheus alerting rules for cluster health and engine signals. |
| scripts/alertmanager.yml | Provides a default “blackhole” Alertmanager config for local use. |
| scripts/grafana_provisioning/dashboards/zenbpm/storage.json | Grafana dashboard for rqlite size/growth, latency, and disk free. |
| scripts/grafana_provisioning/dashboards/zenbpm/latency.json | Grafana dashboard for latency percentiles and throughput. |
| scripts/grafana_provisioning/dashboards/zenbpm/incidents.json | Grafana dashboard for incidents, errors, timers, message correlation, DMN. |
| scripts/grafana_provisioning/dashboards/zenbpm/host.json | Grafana dashboard for node_exporter host resource metrics. |
| scripts/grafana_provisioning/dashboards/zenbpm/cluster.json | Grafana dashboard for raft/cluster leadership and partition health. |
| pkg/otel/traces.go | Renames ZenBPM tracing attribute keys into a zenbpm.* namespace. |
| pkg/otel/metrics.go | Adds shared latency buckets + new engine metrics instruments (incidents, timers, durations, message correlation). |
| pkg/dmn/dmn_multiline_test.go | Updates DMN tests to pass context.Context. |
| pkg/dmn/dmn_engine.go | Adds DMN tracing + evaluation counters/histograms around decision evaluation. |
| pkg/bpmn/sub_process.go | Updates span attributes to the renamed OTEL attribute constants. |
| pkg/bpmn/start_event_instance_creation_handler.go | Records timer-fired metrics for definition-level timer start events post-flush. |
| pkg/bpmn/multi_instance.go | Updates span attributes to the renamed OTEL attribute constants. |
| pkg/bpmn/jobs_api.go | Records job lifetime histogram on completion/failure. |
| pkg/bpmn/events_api.go | Records message correlation success/failure counters with bounded label values. |
| pkg/bpmn/engine.go | Records definition-level timer lifecycle metrics when bypassing EngineBatch.SaveTimer. |
| pkg/bpmn/engine_metrics_test.go | Adds tests for the new engine metric recorders. |
| pkg/bpmn/engine_batch.go | Adds post-flush incident/timer metric recording and implements incident/timer recorder helpers. |
| pkg/bpmn/engine_api.go | Records process instance duration histogram on instance completion/failure. |
| Makefile | Extends start-monitoring to run Alertmanager and mount rule files. |
| internal/rest/server.go | Adds /system/health/live + /system/health/ready and improves /system/status error handling. |
| internal/rest/health_test.go | Adds unit tests for the health response helper. |
| internal/otel/tracer.go | Adds sampler ratio validation and configures parent-based ratio sampling. |
| internal/otel/tracer_test.go | Tests sampler ratio validation and fail-fast behavior. |
| internal/otel/metrics.go | Adds global propagators and runtime metrics instrumentation; validates sampler ratio early. |
| internal/config/config.go | Adds tracing sampler ratio configuration (env + yaml/json). |
| internal/cluster/store/store.go | Stores OTEL callback registration handle for later cleanup. |
| internal/cluster/store/store_helper.go | Unregisters OTEL callbacks on store close to avoid leaks. |
| internal/cluster/store/metrics.go | Adds observable gauges for main cluster raft/partition health. |
| internal/cluster/partition/partition.go | Adds partition-level gauges/counters (leadership, db size, leader changes). |
| internal/cluster/partition/partition_persistence.go | Records rqlite exec/query histograms alongside trace spans. |
| internal/cluster/node.go | Registers store metrics and adds a readiness Health() evaluation. |
| internal/cluster/jobmanager/server.go | Records job activation latency histogram on successful distribution. |
| internal/cluster/jobmanager/otel.go | Registers job_activation_latency histogram and fixes joined error wrapping. |
| go.mod | Adds OTEL runtime instrumentation dependency. |
| go.sum | Adds checksums for OTEL runtime instrumentation dependency. |
| docs/reference/observability.md | Adds comprehensive observability documentation: endpoints, tracing, metrics catalog, alerts, dashboards. |
Suppressed comments (1)
pkg/bpmn/engine_batch.go:298
recordTimerMetriccan panic ifotel.NewMetricsreturned an error but the engine kept the partially-initialized metrics struct (i.e. one of the timer counters is nil). Guard each counter before callingAdd.
engine.metrics.TimersScheduled.Add(ctx, 1)
case bpmnruntime.TimerStateTriggered:
engine.metrics.TimersFired.Add(ctx, 1)
case bpmnruntime.TimerStateCancelled:
engine.metrics.TimersCancelled.Add(ctx, 1)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
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 CodeRabbit
New Features
Documentation