Add parser diagnostic observability infrastructure (#304) - #305
Conversation
Introduce typed diagnostic categories and freeze the eight relation diagnostic identifiers as an ADR-001 compatibility surface. Centralize relation message text so scanner output and typed codes share one contract. Keep the full gate clean under the current Whitaker policy by replacing test-helper `expect` calls with explicit cardinality assertions.
Expose an opt-in `ParseObserver` entry point for deterministic parser, scanner, and diagnostic events while preserving the existing `parse` API through a no-op observer. Add an optional adapter for the `tracing` and `metrics` facades. Keep message text and spans out of metric labels, and leave recorder, subscriber, and exporter installation to host applications.
Record parser callbacks in integration tests and assert all relation diagnostic codes, source spans, parser categories, event ordering, and repeated-run determinism. Verify the observer path returns the same syntax tree and errors as the no-op entry point. Document custom and facade-backed host integration, stable metrics and logging fields, exporter ownership, and rate-based alerting guidance.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary
WalkthroughAdd backend-neutral parser observability with stable diagnostic codes, observer callbacks, optional tracing/metrics integration, public re-exports, documentation, and deterministic integration tests. ChangesParser observability
Sequence Diagram(s)sequenceDiagram
participant Host
participant Parser
participant Observer
participant TelemetryObserver
Host->>Parser: parse_with_observer(source, observer)
Parser->>Observer: report attempt lifecycle
Parser->>Observer: emit diagnostic context
Observer->>TelemetryObserver: forward callbacks
TelemetryObserver-->>Host: tracing events and metrics counters
Parser-->>Host: return Parsed
Possibly related PRs
Suggested labels: Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 4 warnings)
✅ Passed checks (15 passed)
📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. src/parser/observability/mod.rs Comment on lines +57 to +71 pub const fn new(
code: Option<DiagnosticCode>,
category: DiagnosticCategory,
span: Span,
severity: DiagnosticSeverity,
message: &'a str,
) -> Self {
Self {
code,
category,
span,
severity,
message,
}
}❌ New issue: Excess Number of Function Arguments |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Remove the `DiagnosticContext::new` constructor to clear the CodeScene "Excess Number of Function Arguments" finding. The sole call site in `report_diagnostics` now builds the struct with a field literal carrying identical values, preserving field visibility, accessors, lifetimes, and observer callback behaviour. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@docs/parser-observability.md`:
- Around line 80-91: Add descriptive captions immediately before both tables in
docs/parser-observability.md, including the table describing TelemetryObserver
counters and the additional table around the referenced section. Also add the
required Triage paragraph for this documentation-style change, preserving the
existing table content.
In `@src/parser/diagnostics/code.rs`:
- Around line 76-94: The DiagnosticCode enum in src/parser/diagnostics/code.rs
(lines 76-94) must be marked non-exhaustive by adding the appropriate attribute
so future variants remain additive for downstream consumers. No direct changes
are required in docs/parser-conformance-register.md (lines 198-203) or
docs/parser-observability.md (lines 119-122); they are affected sites covered by
the enum-level fix.
In `@src/parser/observability/mod.rs`:
- Around line 21-42: Mark the public DiagnosticSeverity enum as
#[non_exhaustive] so downstream matches must remain forward-compatible with
future severity variants. Leave its existing Error variant and as_str/Display
implementations unchanged.
In `@src/parser/observability/telemetry.rs`:
- Around line 1-56: Add focused tests for TelemetryObserver that install a
metrics_util::debugging::DebuggingRecorder, construct TelemetryObserver::new(),
invoke its ParseObserver methods directly, and assert the emitted counter names,
label keys, and category/code/severity values. Include the relevant
completion/diagnostic paths exposed by TelemetryObserver, and ensure recorder
setup is isolated so the test does not conflict with global metrics state.
- Around line 57-83: Change the diagnostic logging call in diagnostic_emitted
from tracing::error! to tracing::warn!, preserving its existing target, fields,
and message. Keep the metric emission unchanged so alerting can continue to use
sustained diagnostic rates or ratios rather than individual error-level logs.
In `@src/parser/span_scanner.rs`:
- Around line 108-122: Update the Ok arm of the span_result match to call
complete_attempt with DiagnosticCategory::SpanBuilder and an empty span-error
collection, matching the existing Err arm’s completion flow. Remove the direct
ParseAttemptContext construction and ensure the successful spans are still
returned unchanged.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7e1c6cba-a0c4-4d05-a32f-65e812559306
📒 Files selected for processing (23)
Cargo.tomldocs/contents.mddocs/parser-conformance-register.mddocs/parser-implementation-notes.mddocs/parser-observability.mdsrc/lib.rssrc/parser/cst_builder/tree.rssrc/parser/diagnostics/code.rssrc/parser/diagnostics/mod.rssrc/parser/error_messages.rssrc/parser/mod.rssrc/parser/observability/mod.rssrc/parser/observability/telemetry.rssrc/parser/span_scanner.rssrc/parser/span_scanners/relations.rssrc/parser/span_scanners/relations/cursor.rssrc/parser/span_scanners/relations/preamble.rssrc/parser/span_scanners/tests/mod.rssrc/parser/tests/helpers.rssrc/parser/tests/rules/aggregations.rssrc/parser/tests/rules/body_terms.rstests/parser_observability.rstests/support/observability.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/shared-actions(auto-detected)
| `TelemetryObserver` emits the following counters: | ||
|
|
||
| | Metric | Labels | Meaning | | ||
| | --------------------------------- | ------------------------------ | ---------------------------------- | | ||
| | `ddlint_parser_attempts_total` | `category` | Parser or scanner attempts started | | ||
| | `ddlint_parser_diagnostics_total` | `code`, `category`, `severity` | Diagnostics emitted | | ||
|
|
||
| The `code` label is the stable diagnostic code or `uncoded`. Categories and | ||
| severity are bounded enums. The adapter emits attempt start/completion events at | ||
| `DEBUG` and diagnostics at `ERROR` under the `ddlint::parser` tracing target. | ||
| Diagnostic events include `code`, `category`, `severity`, `span_start`, | ||
| `span_end`, and `message` fields. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Caption both tables.
Add a descriptive caption immediately before each table.
Triage: [type:docstyle]
As per coding guidelines, “Caption every table.” As per path instructions, documentation-style findings require a Triage paragraph.
Also applies to: 124-138
🤖 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 `@docs/parser-observability.md` around lines 80 - 91, Add descriptive captions
immediately before both tables in docs/parser-observability.md, including the
table describing TelemetryObserver counters and the additional table around the
referenced section. Also add the required Triage paragraph for this
documentation-style change, preserving the existing table content.
Sources: Coding guidelines, Path instructions
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] | ||
| pub enum DiagnosticCode { | ||
| /// A relation kind appears before its role keyword. | ||
| RelationKindBeforeRole, | ||
| /// A relation declaration contains more than one role keyword. | ||
| RelationDuplicateRole, | ||
| /// A relation declaration contains more than one kind keyword. | ||
| RelationDuplicateKind, | ||
| /// A bracket-form relation declares a primary-key clause. | ||
| RelationBracketPrimaryKey, | ||
| /// A bracket-form relation does not contain exactly one element type. | ||
| RelationInvalidBracketElementType, | ||
| /// A non-input relation declares a primary-key clause. | ||
| RelationPrimaryKeyOnNonInput, | ||
| /// A relation contains an unexpected or malformed primary-key clause. | ||
| RelationMalformedPrimaryKey, | ||
| /// A relation uses an unsupported bracket-wrapped primary-key clause. | ||
| RelationBracketWrappedPrimaryKey, | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== code.rs ==\n'
sed -n '1,180p' src/parser/diagnostics/code.rs
printf '\n== conformance register ==\n'
sed -n '185,210p' docs/parser-conformance-register.md
printf '\n== observability docs ==\n'
sed -n '110,130p' docs/parser-observability.mdRepository: leynos/ddlint
Length of output: 8944
Mark DiagnosticCode as non-exhaustive. Add #[non_exhaustive]; otherwise every future variant is a source-breaking change for downstream exhaustive matches, which undermines the additive compatibility contract.
📍 Affects 3 files
src/parser/diagnostics/code.rs#L76-L94(this comment)docs/parser-conformance-register.md#L198-L203docs/parser-observability.md#L119-L122
🤖 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 `@src/parser/diagnostics/code.rs` around lines 76 - 94, The DiagnosticCode enum
in src/parser/diagnostics/code.rs (lines 76-94) must be marked non-exhaustive by
adding the appropriate attribute so future variants remain additive for
downstream consumers. No direct changes are required in
docs/parser-conformance-register.md (lines 198-203) or
docs/parser-observability.md (lines 119-122); they are affected sites covered by
the enum-level fix.
| /// Severity of a parser diagnostic. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] | ||
| pub enum DiagnosticSeverity { | ||
| /// The parser could not accept or validate part of the source. | ||
| Error, | ||
| } | ||
|
|
||
| impl DiagnosticSeverity { | ||
| /// Return the stable, low-cardinality severity label. | ||
| #[must_use] | ||
| pub const fn as_str(self) -> &'static str { | ||
| match self { | ||
| Self::Error => "error", | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl fmt::Display for DiagnosticSeverity { | ||
| fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| formatter.write_str(self.as_str()) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Mark DiagnosticSeverity #[non_exhaustive].
The doc comment frames this as "severity of a parser diagnostic" with a single variant today, implying growth (Warning, perhaps). Downstream ParseObserver implementations that match exhaustively on this enum will break the moment a second variant lands. Mark it #[non_exhaustive] now, whilst the API is new, to make that future addition non-breaking.
🔧 Proposed fix
/// Severity of a parser diagnostic.
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
+#[non_exhaustive]
pub enum DiagnosticSeverity {
/// The parser could not accept or validate part of the source.
Error,
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Severity of a parser diagnostic. | |
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] | |
| pub enum DiagnosticSeverity { | |
| /// The parser could not accept or validate part of the source. | |
| Error, | |
| } | |
| impl DiagnosticSeverity { | |
| /// Return the stable, low-cardinality severity label. | |
| #[must_use] | |
| pub const fn as_str(self) -> &'static str { | |
| match self { | |
| Self::Error => "error", | |
| } | |
| } | |
| } | |
| impl fmt::Display for DiagnosticSeverity { | |
| fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { | |
| formatter.write_str(self.as_str()) | |
| } | |
| } | |
| /// Severity of a parser diagnostic. | |
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] | |
| #[non_exhaustive] | |
| pub enum DiagnosticSeverity { | |
| /// The parser could not accept or validate part of the source. | |
| Error, | |
| } | |
| impl DiagnosticSeverity { | |
| /// Return the stable, low-cardinality severity label. | |
| #[must_use] | |
| pub const fn as_str(self) -> &'static str { | |
| match self { | |
| Self::Error => "error", | |
| } | |
| } | |
| } | |
| impl fmt::Display for DiagnosticSeverity { | |
| fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { | |
| formatter.write_str(self.as_str()) | |
| } | |
| } |
🤖 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 `@src/parser/observability/mod.rs` around lines 21 - 42, Mark the public
DiagnosticSeverity enum as #[non_exhaustive] so downstream matches must remain
forward-compatible with future severity variants. Leave its existing Error
variant and as_str/Display implementations unchanged.
| //! Optional `tracing` and `metrics` observer adapter. | ||
|
|
||
| use metrics::{counter, describe_counter}; | ||
|
|
||
| use super::{DiagnosticContext, ParseAttemptContext, ParseObserver}; | ||
| use crate::parser::diagnostics::DiagnosticCategory; | ||
|
|
||
| const ATTEMPT_METRIC: &str = "ddlint_parser_attempts_total"; | ||
| const DIAGNOSTIC_METRIC: &str = "ddlint_parser_diagnostics_total"; | ||
|
|
||
| /// Observer that forwards parser events to the `tracing` and `metrics` facades. | ||
| /// | ||
| /// Constructing this adapter describes its counters but does not install a | ||
| /// global tracing subscriber, metrics recorder, or exporter. | ||
| #[derive(Debug, Clone, Copy)] | ||
| #[non_exhaustive] | ||
| pub struct TelemetryObserver; | ||
|
|
||
| impl TelemetryObserver { | ||
| /// Construct the optional telemetry adapter. | ||
| #[must_use] | ||
| pub fn new() -> Self { | ||
| describe_counter!(ATTEMPT_METRIC, "Parser and scanner attempts"); | ||
| describe_counter!(DIAGNOSTIC_METRIC, "Parser diagnostics emitted"); | ||
| Self | ||
| } | ||
| } | ||
|
|
||
| impl Default for TelemetryObserver { | ||
| fn default() -> Self { | ||
| Self::new() | ||
| } | ||
| } | ||
|
|
||
| impl ParseObserver for TelemetryObserver { | ||
| fn parse_attempt_started(&self, category: DiagnosticCategory) { | ||
| let category = category.as_str(); | ||
| counter!(ATTEMPT_METRIC, "category" => category).increment(1); | ||
| tracing::debug!( | ||
| target: "ddlint::parser", | ||
| category, | ||
| "parser attempt started" | ||
| ); | ||
| } | ||
|
|
||
| fn parse_attempt_completed(&self, context: ParseAttemptContext) { | ||
| let category = context.category().as_str(); | ||
| let diagnostic_count = context.diagnostic_count(); | ||
| tracing::debug!( | ||
| target: "ddlint::parser", | ||
| category, | ||
| diagnostic_count, | ||
| "parser attempt completed" | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a test asserting TelemetryObserver's emitted counters/fields.
This adapter's label-mapping logic (category/code/severity strings, counter names) is untouched by any provided test — RecordingParseObserver covers the generic observer contract, but nothing exercises TelemetryObserver itself. A metrics_util::debugging::DebuggingRecorder snapshot (or a captured tracing subscriber) run against TelemetryObserver::new() plus direct trait-method calls would catch label typos or cardinality regressions before they ship silently in an optional, hard-to-manually-verify path.
🤖 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 `@src/parser/observability/telemetry.rs` around lines 1 - 56, Add focused tests
for TelemetryObserver that install a metrics_util::debugging::DebuggingRecorder,
construct TelemetryObserver::new(), invoke its ParseObserver methods directly,
and assert the emitted counter names, label keys, and category/code/severity
values. Include the relevant completion/diagnostic paths exposed by
TelemetryObserver, and ensure recorder setup is isolated so the test does not
conflict with global metrics state.
Source: Coding guidelines
| fn diagnostic_emitted(&self, context: &DiagnosticContext<'_>) { | ||
| let code = context.code().map_or("uncoded", |code| code.as_str()); | ||
| let category = context.category().as_str(); | ||
| let severity = context.severity().as_str(); | ||
| let span_start = context.span().start; | ||
| let span_end = context.span().end; | ||
| let message = context.message(); | ||
|
|
||
| counter!( | ||
| DIAGNOSTIC_METRIC, | ||
| "code" => code, | ||
| "category" => category, | ||
| "severity" => severity | ||
| ) | ||
| .increment(1); | ||
| tracing::error!( | ||
| target: "ddlint::parser", | ||
| code, | ||
| category, | ||
| severity, | ||
| span_start, | ||
| span_end, | ||
| message, | ||
| "parser diagnostic" | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Reconsider hard-coding tracing::error! for every diagnostic.
A parser diagnostic represents expected, potentially very frequent, user-input mistakes (a syntax typo whilst editing), not a system fault. Hosts that treat ERROR-level logs as actionable will get paged or alert-fatigued on ordinary typos. The retrieved guidance for this repo is explicit that diagnostics shouldn't trigger alerts on existence alone, only on sustained rate/ratio — emitting at error! unconditionally works against that by default. Emit at tracing::warn! (or select the level from context.severity() once more variants exist) so hosts aren't nudged toward per-occurrence alerting.
Based on learnings, "Configure alerts on sustained diagnostic-to-parser-attempt rates or ratios after a minimum traffic threshold; do not page solely because a diagnostic exists, since invalid user input may be expected."
🤖 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 `@src/parser/observability/telemetry.rs` around lines 57 - 83, Change the
diagnostic logging call in diagnostic_emitted from tracing::error! to
tracing::warn!, preserving its existing target, fields, and message. Keep the
metric emission unchanged so alerting can continue to use sustained diagnostic
rates or ratios rather than individual error-level logs.
Source: Learnings
| let spans = match span_result { | ||
| Ok(spans) => spans, | ||
| Ok(spans) => { | ||
| observer.parse_attempt_completed(super::observability::ParseAttemptContext::new( | ||
| DiagnosticCategory::SpanBuilder, | ||
| 0, | ||
| )); | ||
| spans | ||
| } | ||
| Err(err) => { | ||
| all_errors.push(chumsky::error::Simple::custom(0..0, err.to_string())); | ||
| let span_errors = [chumsky::error::Simple::custom(0..0, err.to_string())]; | ||
| complete_attempt(observer, DiagnosticCategory::SpanBuilder, &span_errors); | ||
| all_errors.extend(span_errors); | ||
| ParsedSpans::default() | ||
| } | ||
| }; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Route the SpanBuilder success path through complete_attempt for consistency.
Every other attempt in this file (including the Err arm directly below) reports completion via complete_attempt, which also drives report_diagnostics. The Ok arm instead hand-constructs ParseAttemptContext via a fully-qualified path, since ParseAttemptContext isn't imported here. This is harmless today (zero errors on the happy path) but diverges silently if complete_attempt's behaviour ever changes, and the fully-qualified path is avoidable.
♻️ Proposed fix
-use super::observability::{ParseObserver, complete_attempt};
+use super::observability::{ParseAttemptContext, ParseObserver, complete_attempt}; let spans = match span_result {
Ok(spans) => {
- observer.parse_attempt_completed(super::observability::ParseAttemptContext::new(
- DiagnosticCategory::SpanBuilder,
- 0,
- ));
+ complete_attempt(observer, DiagnosticCategory::SpanBuilder, &[]);
spans
}🤖 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 `@src/parser/span_scanner.rs` around lines 108 - 122, Update the Ok arm of the
span_result match to call complete_attempt with DiagnosticCategory::SpanBuilder
and an empty span-error collection, matching the existing Err arm’s completion
flow. Remove the direct ParseAttemptContext construction and ensure the
successful spans are still returned unchanged.
1 participant
Summary
This branch establishes repository-wide, backend-neutral parser observability
before scanner-specific telemetry expands. It adds a stable diagnostic
taxonomy, an opt-in observer API, an optional
tracingandmetricsadapter,deterministic event tests, and downstream export and alerting guidance.
Existing
parse()behaviour remains unchanged through the no-op observer, andthe parser library does not install or require a telemetry runtime.
Closes #304.
Review walkthrough
for the stable categories and
D-REL-001throughD-REL-008compatibilitysurface.
and parser orchestration
for the no-op path and structured callback boundary.
to see category provenance preserved before diagnostic vectors merge.
for bounded metric labels and host-owned subscriber and recorder setup.
and host integration guide
for deterministic code/span coverage and operational guidance.
Validation
cargo check --no-default-features: passed.cargo check --all-features: passed.cargo test --no-default-features --test parser_observability: 10 passed.make check-fmt: passed.make lint: passed, including Clippy, Whitaker, and spelling.make test: passed for all targets and all features.make markdownlint: passed.make nixie: passed.mbake validate Makefile: passed.coderabbit review --agent: zero findings after each of the threeimplementation milestones.
Notes
observabilityfeature is opt-in and is absent from default features.code,category, andseveritylabels. Messagetext and spans remain structured logging fields.
those diagnostics are observable through their stable categories.
References