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. In every string value of your JSON output, including comment bodies and suggestion blocks, keep { and } balanced: each string must contain as many { as }. If any string has unbalanced braces, the review action can't parse your output and posts nothing. When you need to quote code that opens a block, such as impl Foo for Bar {, either quote the complete block or describe it in prose instead.

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.