Code review

This page covers the guidelines we use when reviewing changes, whether the reviewer is a human or an AI. To check your own change before opening a PR, read through the checklists below.

How AI review works

A collaborator with write access can comment /dd-review on a PR to request an AI code review. The review uses the codereview_guideline.md files in the repository as its instructions:

• The file at the repository root applies to every PR.
• A file in a subdirectory applies only to PRs that change files under that subdirectory.
• The guidelines are read from main, not from the PR, so changes to them take effect after they merge.

The review action removes AGENTS.md and CLAUDE.md files before the review runs, so each guideline file MUST stay self-contained, rather than pointing to other pages. This makes the guideline files one of the few exceptions to Where documentation lives. To add review guidance for one area of the codebase, add a codereview_guideline.md file in that area's directory and include it on this page.

The sections below render the guideline files as they're written.

General guidelines

From codereview_guideline.md:

You are acting as a reviewer for a proposed code change made by another engineer. Focus on issues that impact correctness, performance, security, maintainability, or developer experience, in roughly that order: correctness comes first, then performance. Flag only actionable issues introduced by the pull request. When you flag an issue, provide a short, direct explanation and cite the affected file and line range. Prioritize severe issues and avoid nit-level comments unless they block understanding of the diff. Do not flag formatting or lint issues that make fmt and make check-all already enforce. After listing findings, produce an overall correctness verdict ("patch is correct" or "patch is incorrect") with a concise justification. Ensure that file citations and line numbers are exactly correct using the tools available; if they are incorrect your comments will be rejected.

Project-specific review checklist

Saluki is a Rust toolkit for building observability data planes. Its primary artifact, bin/agent-data-plane, runs alongside the Datadog Agent.

Generic crates must stay generic

Crates under lib/saluki-* (including lib/saluki-io) are general-purpose infrastructure. Flag code, comments, or documentation in those crates that refer to the Datadog Agent by name, to internal Datadog codenames or project names, or to Datadog-specific deployment topologies. This rule does not apply to bin/agent-data-plane, lib/datadog-agent/, configuration registry annotations, or known-configs documentation.

Generated and vendored files

Flag hand edits to files marked @generated or DO NOT EDIT; the source of truth must be changed and the file regenerated instead. Flag hand edits to the vendored Datadog schema under lib/datadog-agent/config/schema/core/, which must stay a pristine upstream copy.

Hot paths and allocation

Much of Saluki processes high volumes of metrics, logs, and traces. In per-event code paths, flag new heap allocations, clones, or string formatting that could be avoided, unbounded buffering or queues, and logging at any level that scales with event volume. In particular:

• Flag a clone or an owned String where a reference, a &'static str, or a value the code already owns would work, and flag methods that silently clone large data structures.
• Fixed strings stored in events, tags, or contexts should use MetaString::from_static or existing string interning rather than allocating a new String for each event.
• Flag code that collects into a temporary Vec or map only to iterate over it once, when it could iterate directly.
• Flag per-event construction of builders, resolvers, or buffers that could be created once and reused. Regressions that only affect tail latency (p95, p99) still count.

Concurrency and component lifecycle

Components run concurrently on Tokio and have explicit startup and shutdown. Flag blocking calls on async tasks, locks held across .await points, spawned tasks that are never joined or cancelled, and shutdown paths that can drop buffered data or hang. Changes to component lifecycle should have tests that exercise graceful shutdown.

• Flag code that drops messages or updates without telemetry or a log, such as try_send failures that are ignored. Applying backpressure is usually better than losing data without anyone noticing.
• Flag shutdown handling that differs between select! branches, such as one branch that cleans up a resource and another that exits without doing so.
• Flag Arc<T> or Mutex<T> wrappers that nothing shares across tasks. Also flag misuse of Tokio synchronization primitives where their documented semantics differ from what the code assumes. For example, a Notified future from Notify::notified() receives notify_waiters() calls as soon as it is created, but isn't guaranteed to receive a notify_one() call until it is first polled or enable()d.
• Flag synchronization patterns ported from Go, such as serializing every loop iteration through a rendezvous channel, when they don't fit Tokio's scheduling model.

Error handling

Flag new unwrap(), expect(), or panicking indexing in non-test code where the failure is reachable from external input or configuration. Error messages should say what went wrong in plain language and, where relevant, name the configuration field involved.

• Untyped errors use saluki_error::GenericError, with context added through saluki_error::ErrorContext (error_context or with_error_context). Typed errors use snafu. In non-test code, flag new uses of anyhow types directly, Box<dyn Error>, or String as an error type.
• When a recoverable condition can be handled by logging a warning and skipping the bad input, prefer that over panicking. Conditions that need someone's attention should log at warn or higher, not info.
• Flag errors that are swallowed in a way that silently loses data.

Design and API conventions

• Matching the Datadog Agent's observable behavior is often required, but reproducing its internal design is not. Flag code that copies an Agent implementation detail which is a poor fit for Rust or for Saluki's architecture, unless the pull request explains why that exact behavior is required.
• Conversion methods should follow the Rust API Guidelines naming conventions. as_ is a cheap borrow, to_ is an expensive or owned conversion, and into_ consumes self. Use an into_parts(self)-style method when callers need owned access to the internals.
• Flag hand-rolled logic that duplicates a method in the standard library or an existing workspace crate, such as a manual ASCII check instead of char::is_ascii_alphanumeric.
• Flag constants or definitions duplicated across crates. Define them once and reference them from each place.
• Flag new wrappers, traits, or layers of indirection that add nothing over the type they wrap.

The following are minor. Report them as nits, ranked below correctness and performance findings:

• Type and function names that repeat the enclosing module or type name, such as eventd::EventPriority instead of eventd::Priority.
• Small, field-less enums that don't derive Copy, or that allocate a String where a method returning &'static str would do.
• TODO comments and "not yet implemented" notes that don't say what is missing.

Internal telemetry

• agent-data-plane applies the adp. prefix to all internal metrics at bootstrap. Flag new metric names that include the prefix themselves.
• A metric must measure what its name says. For example, flag a counter named for spans that is incremented once per payload. Also check that the metric type (counter, gauge, or histogram) fits what is being measured.

Scope

Large refactors that a pull request merely makes possible belong in follow-up pull requests. Don't flag the lack of such a refactor as a defect. If the change introduces a hazard that it leaves unfixed, suggest filing an issue.

Tests

• Every property-based test function MUST be named with the property_test_ prefix. The Make targets select tests by that prefix, so a misnamed property test silently runs in the wrong job.
• Flag tests that cannot fail when the behavior under test regresses: no assertions, assertions only on values the test itself constructed, or a property test with no prop_assert!.
• Flag tests that sleep to wait for background work instead of using deterministic synchronization or Tokio's paused time.
• Test functions should be named as bare, descriptive sentences, without a test_ prefix.
• New behavior, including new input forms such as short-form aliases, should have unit tests that exercise it.
• Flag tests that share global state, such as environment variables or process-wide singletons, without real mutual exclusion. Two separate Mutex instances don't serialize anything.
• Flag test code that copies setup or assertions from neighboring tests when they don't apply to the behavior under test.

Documentation and release notes

• All public items MUST have rustdoc. Configuration fields MUST document what they control, their default value, and their edge cases.
• If a pull request changes behavior but does not update the corresponding docs, comments, or doc strings, flag it.
• Reference documentation under docs/reference/ describes how the code works today. Flag aspirational or inaccurate descriptions there.
• User-visible changes to agent-data-plane need a release note fragment under releasenotes/notes/, unless the pull request carries the changelog/no-changelog label.

Scoped guidelines

From lib/datadog-agent/config/codereview_guideline.md, which applies to PRs that change the configuration system:

Configuration system review checklist

Apply these when the pull request touches the Datadog configuration source model.

• Support for Datadog schema keys is classified in schema/schema_overlay.yaml. Flag changes that alter which keys are supported without updating the overlay, and flag code generated from the schema and overlay that was edited by hand instead of regenerated with make build-schema-overlay.
• Files under schema/core/ are a vendored upstream copy of the Datadog schema. Flag any hand edit to them.
• A modeled key arrives in the Datadog Agent's canonical shape, and the source model must read that shape. Flag new #[serde(rename)] or #[serde(alias)] attributes on source model fields.
• Whether a key is a Datadog key or a Saluki-only key depends on whether it appears in the schema, not on its name. Flag reasoning or code that infers the class of a key from its prefix, such as data_plane..

From lib/saluki-core/codereview_guideline.md, which applies to PRs that change the core crate and data model:

Core crate review checklist

Apply these when the pull request touches lib/saluki-core, which holds the data model, the component and topology traits, and the runtime that every other crate builds on.

• The event data model under src/data_model/event/ is Saluki's own representation of events. Flag fields or methods on event types that take or return protocol-specific types, such as OTLP or wire-format protobuf types. Translation between wire formats and the data model belongs in sources, decoders, and encoders. Recording where an event came from, for example an Otlp variant in an origin enum, is fine. Payload types under src/data_model/payload/ are transport-level by design and may carry protocol types such as http::Request.
• Flag data model accessors that clone heavy values when a reference would do. Callers that need ownership should get an into_parts(self)-style method or a mutable accessor instead.
• Changes to public traits and types here affect every component. Flag breaking changes that the pull request doesn't call out, and flag new public API that has only one caller and could live with that caller instead.