docs: fix new-developer onboarding gaps across overview, quickstart, and configuration pages - #326
Conversation
…and configuration pages
janiussyafiq
added
documentation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Free Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR updates documentation: restructures bootstrap configuration sections into tables with defaults and optional flags, refines core concept fields and observability exporter text, and adds explicit quickstart prerequisites plus an API-key secrecy warning. ChangesDocumentation Clarity Improvements
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Note 🎁 Summarized by CodeRabbit FreeYour organization has reached its limit of developer seats under the Pro Plan. For new users, CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please add seats to your subscription by visiting https://app.coderabbit.ai/login.If you believe this is a mistake and have available seats, please assign one to the pull request author through the subscription management page using the link above. Comment |
There was a problem hiding this comment.
Pull request overview
This doc-only PR improves new-developer onboarding by clarifying prerequisites, model naming, credential handling, and bootstrap configuration defaults across the quickstart and overview/configuration docs.
Changes:
- Adds Rust/Node/provider-key prerequisite guidance and build-time expectations.
- Clarifies model alias vs upstream model ID and credential visibility.
- Expands bootstrap configuration tables, especially observability and cache fields.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
docs/quickstart/self-hosted.md |
Adds Rust/cargo prerequisite and first-build duration note. |
docs/quickstart/openai-sdk.md |
Adds gateway and Node.js/npm prerequisites. |
docs/quickstart/first-model-first-key-first-request.md |
Clarifies provider API key prerequisite and plaintext provider-key handling. |
docs/overview/core-concepts.md |
Clarifies model fields, API-key plaintext handling, and observability exporter description. |
docs/configuration/bootstrap-config.md |
Adds defaults and expanded observability/cache bootstrap field tables. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| Current data-plane behavior is based on `key_hash`, not plaintext storage. The proxy hashes the incoming bearer token and resolves it against the stored `key_hash`. | ||
|
|
||
| This means you cannot retrieve the plaintext bearer after creation — capture the value returned at create time, and if you lose it, use `POST /admin/v1/apikeys/:id/rotate` to issue a new one. |
There was a problem hiding this comment.
Applied. Rewrote the appended paragraph to reflect actual behavior: the plaintext bearer is chosen (or generated) by the caller and SHA-256-hashed locally before submission, the gateway never sees or returns the plaintext at create time, and POST /admin/v1/apikeys/:id/rotate is the only endpoint that emits a server-generated plaintext (verified against apikeys_handlers.rs:97-104 for create and :155-178 for rotate). Pushed in commit 5401ff1.
| | `metrics.otlp.enabled` | push-style OTLP metrics exporter | `false` | | ||
| | `metrics.otlp.endpoint` | OTLP/gRPC collector endpoint, e.g. `"http://otel-collector:4317"` | none | | ||
| | `tracing.otlp.enabled` | push-style OTLP traces exporter | `false` | | ||
| | `tracing.otlp.endpoint` | OTLP/gRPC collector endpoint for traces | none | |
There was a problem hiding this comment.
Applied. Confirmed against code: install_otlp_tracer (crates/aisix-obs/src/otlp.rs:26-58) validates the endpoint and emits a startup log line but defers the real exporter pipeline ("The real exporter wires up in a follow-up PR" per the inline code comment); no install_otlp_metrics function exists in the workspace. The bootstrap-config observability table now carries a Status column marking metrics.otlp.* as reserved (not yet wired) and tracing.otlp.* as partial (validation only). Pushed in commit 5401ff1.
| An `Observability Exporter` is a resource that configures external telemetry export from the gateway. | ||
|
|
||
| Use this concept when documenting external metrics, traces, or event forwarding behavior. | ||
| An `Observability Exporter` ships telemetry (metrics, traces, request logs) from the gateway to an external backend over an OTLP-compatible endpoint (Grafana Tempo / Loki, Honeycomb, Langfuse via OTLP, and so on). Configure one when you want gateway request and response data forwarded to your existing observability stack. |
There was a problem hiding this comment.
Applied. Verified against otlp_http_sink.rs:184/:249 (the exporter builds OTLP traces payloads from UsageEvent records — per-request spans, not general metrics or logs). Rewrote the section to: "ships per-request span telemetry — derived from gateway UsageEvent records — to an OTLP/HTTP-compatible backend". Pushed in commit 5401ff1.
| | `metrics.prometheus.enabled` | scrape-style Prometheus exporter | `true` | | ||
| | `metrics.prometheus.path` | HTTP path the Prometheus exporter is served on | `"/metrics"` | |
There was a problem hiding this comment.
Applied. Verified against crates/aisix-admin/src/lib.rs:68 (/metrics route is hardcoded with no conditional gating). The bootstrap-config observability table now marks metrics.prometheus.enabled and metrics.prometheus.path as reserved (not yet consulted), with a footnote that the Prometheus exporter is currently mounted unconditionally at the hardcoded admin /metrics path. Pushed in commit 5401ff1.
| | --- | --- | --- | | ||
| | `service_name` | service-name attribute attached to every metric, log, and span emitted by the process | `"aisix"` | | ||
| | `log_level` | minimum log level (`error` / `warn` / `info` / `debug` / `trace`) | `"info"` | | ||
| | `access_log` | emit a structured access-log line for every proxy request | `true` | |
There was a problem hiding this comment.
Applied. Verified against crates/aisix-obs/src/access_log.rs:32-50 (AccessLog::emit() always calls tracing::info! with no config gate) and the unconditional emit_access_log call sites in every proxy handler. The bootstrap-config observability table now marks access_log as reserved (not yet consulted), with a footnote that access logs are emitted unconditionally by every proxy handler today. Pushed in commit 5401ff1.
…lity-config claims to match runtime (Copilot review)
…s fields PR #331 (commit e6125b6) wired the `metrics.prometheus.enabled` and `metrics.prometheus.path` consumers in `crates/aisix-admin/src/lib.rs:154-159` plus the `normalized_prometheus_path` helper at `:164-174`, and exercised both paths end-to-end in three new unit tests (`metrics_endpoint_uses_configured_path`, `metrics_endpoint_normalizes_configured_path`, `metrics_endpoint_can_be_disabled`). The two rows in this PR's observability field table that called those fields "reserved (not yet consulted)" are now factually stale. Update both rows to `wired` and align the row descriptions with #331's runtime behaviour. Also adjust the introductory paragraph above the table so it no longer asserts that only `service_name` and `log_level` are consulted at runtime. This is the consumer-trace discipline locked in during round 2 of this PR, applied prospectively to a moving target: PR #331 changed the consumer site after round 2's verification window, so the doc text needs to update.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
docs/configuration/bootstrap-config.md:164
- The tracing endpoint is described as validated at boot, but the implementation only checks that
tracing.otlp.enabled=truehas a non-empty endpoint; it does not parse or validate the endpoint URL/protocol before returning a handle. Calling this "validated" overstates the current behavior.
| `tracing.otlp.enabled` | enabling this validates the endpoint at boot and emits a startup log line; the OTLP traces pipeline itself is deferred to a future release | `false` | partial (validation only) |
| `tracing.otlp.endpoint` | OTLP/gRPC collector endpoint for traces; validated at boot when `tracing.otlp.enabled` is `true` | none | partial (validation only) |
| | `metrics.prometheus.enabled` | controls whether the admin listener mounts the Prometheus scrape endpoint; when `false`, no `/metrics` route is registered | `true` | wired | | ||
| | `metrics.prometheus.path` | mount path for the Prometheus scrape endpoint when `metrics.prometheus.enabled` is `true`; values without a leading slash are normalised by prepending one, and an empty value falls back to `/metrics` | `"/metrics"` | wired | |
| ## `observability` | ||
|
|
||
| Use `observability` to configure: | ||
| Use `observability` to set process-wide telemetry knobs: service name, log level, Prometheus exporter control, and (in future releases) access-log gating and OTLP exporters. Today `service_name`, `log_level`, and the `metrics.prometheus.*` block are consulted at runtime; the remaining keys are recognized in the schema and reserved for upcoming releases — setting them is harmless but currently has no effect. |
Two related items folded into a single revision commit per operator authorization (msg `a5c9dec7`): 1. Merge conflict resolution against `main` post-PR-#326 PR #326 (merge commit `e12155b`, landed 2026-05-19 02:30:04Z) updated the `## Prerequisites` block and the `## Step 3: Start the gateway` section of `docs/quickstart/self-hosted.md` on `main`. This branch's prior edits (etcd glossary link, `config.yaml` location anchor, `YOUR_ADMIN_KEY` placeholder format note, "Keep the gateway running" framing) overlapped the Prerequisites bullet on the same physical line. Resolution per Umar's approved Option 2: single new commit on top of `a109d2d` that re-applies this branch's Bucket-B edits on top of the post-#326 canonical lines: - Add the post-#326 Rust 1.93 prerequisite bullet. - Capitalize "A reachable etcd instance" (per #326) while preserving this branch's `[etcd](../overview/glossary.md#etcd)` glossary link. - Add the post-#326 "first time you run this" cargo-build duration paragraph between the `cargo run` code block and this branch's "Keep the gateway running. Open a new terminal..." framing. The two paragraphs now sequence as: (a) explain first-run build time, (b) instruct reader to keep the gateway running and switch terminals. Other 4 overlapping files (`bootstrap-config.md`, `core-concepts.md`, `first-model-first-key-first-request.md`, `openai-sdk.md`) auto-merge cleanly against post-#326 main and are not touched by this commit. 2. Copilot review address — comment id `3260189475` The reviewer flagged that the "Capture the id (jq path)" snippet on `docs/quickstart/first-model-first-key-first-request.md` re-ran the same `POST /admin/v1/provider_keys` as Step 1, which would either trigger a duplicate-`display_name` rejection from the admin API or leave the reader with two ProviderKey rows whose ids differ from the response example shown after the first POST. Verified empirically by reading the two POST blocks (lines 37-46 and the captured-id snippet) against each other. Restructure the section so the jq path is presented as a variant of the original create, not a second POST: - Rename the code block title to "Create and capture the id in one shot" to make the variant relationship explicit. - Wrap the two paths as bulleted Option A / Option B blocks. Option A is the jq variant of the create above ("instead of the curl shown earlier"). Option B is the jq-free path ("if you already ran the curl above, copy the id field by eye"). - Prefix the section with an explicit "pick one, don't run both" callout naming the duplicate-display_name rejection as the failure mode if the reader runs both forms.
Integrate origin/main (commit 2c1d485 = post-PR-#326 / #348 plus #330 / #341 / #343 / #345 / #346) into this branch via `git merge --squash` to clear PR #344's lingering `mergeable: dirty` state. Conflict on `docs/quickstart/self-hosted.md` was a 3-way-merge-base artifact: base (3596c0a) read `- a reachable etcd instance`, main changed `a` → `A` (via #326), this branch additionally inserted the glossary link. Both changes are wanted; resolution per Umar's approved plan was `git checkout --ours`, which preserves the branch's self-hosted.md state (already integrates capital A + glossary link + first-time-build paragraph + keep-running framing). Other 4 overlapping doc files auto-merged cleanly (`bootstrap-config.md`, `core-concepts.md`, `first-model-first-key-first-request.md`, `openai-sdk.md`). Code files all auto-merged cleanly. Additional Copilot review (post-`167196a` cycle) addressed: - `docs/index.md:7` — change link display text from `[data-plane]` to `[data plane]` to match the canonical glossary term. The URL anchor `#data-plane` stays kebab-case (matches the glossary heading's auto-anchor); only the display text changes. Comment id 3271145422. - `docs/quickstart/openai-sdk.md:43` — change `All three steps below` to `All commands below`. The Install-the-SDK section has two command blocks (mkdir+cd, npm install), not three; the prior wording originated from a mental model (mkdir, cd, install) that doesn't match the typographic count of code blocks under the heading. Comment id 3271145458. Copilot's third comment on `docs/overview/core-concepts.md` Observability Exporter wording (id 3271145444) auto-resolves via this merge — main's #326 rewrite supersedes the branch's pre-#326 wording at that location ("ships per-request span telemetry… OTLP/HTTP-compatible backend…" replaces "Use this concept when documenting…"). No separate edit needed; the merge IS the fix.
…ickstart-polish Resolve PR #344's lingering mergeable: dirty state by linking the branch history to origin/main (2c1d485 = post-#326 / #348 / #330 / #341 / #343 / #345 / #346). The squash-merge commit landed earlier (e2af197) integrated main's content into the branch tree but did not link the histories, so GitHub's mergeable computation still saw the 3-way-merge-base artifact conflict on docs/quickstart/self-hosted.md (a vs A + the glossary link / "In another terminal" vs "Keep the gateway running" framing). This explicit merge commit ties the branch to main's history. Self-hosted.md conflict resolved by taking OUR side — the branch's edits already contain main's substantive changes (capital A, first-time-build paragraph) plus this PR's additions (glossary link, keep-running framing, YOUR_ADMIN_KEY note, config.yaml location anchor). The auto-merge of first-model-first-key-first-request.md duplicated the :::warning callout that was already integrated via the squash commit; removed the duplicate.
Summary
Fixes six new-developer onboarding gaps surfaced by an end-to-end walkthrough of the 7-page onboarding flow (
docs/index.md→docs/configuration/bootstrap-config.md) againstmainat1a744ee. Three of the fixes (#1, #2, #3) close hard blockers on a clean machine (missingcargo/ npm / provider-API-key prerequisites;model_not_foundfromdisplay_name/model_nameambiguity; an Observability Exporter section whose second sentence is style-guide advice rather than user content). The remaining three (#4, #5, #6) close friction points along the same path (bootstrap-config.mdfield tables missing aDefaultcolumn and an observability subsection; two credential-handling operational callouts; a cold-build duration note).Doc-only diff. No code, schemas, configs, or test fixtures touched.
Closes #325.
Changes
docs/quickstart/self-hosted.mdcargo(viarustup) as the leading prerequisite. Aftercargo run …: add a one-sentence note that the first-time build typically takes 3–5 minutes and that subsequent runs are incremental.docs/quickstart/first-model-first-key-first-request.mdsk-...format hint, placeholder pointer). After theProviderKeycreate-time response example: add a:::warningcallout that thesecretfield is plaintext and thatGET /admin/v1/provider_keys/:idalso returns the plaintext.docs/quickstart/openai-sdk.md## Prerequisitessection placed between the "If you have not done that yet…" intro and the first## What Changes In The SDKsection. Lists a running gateway with provider key / model / API key already created, and Node.js 18+ withnpm(the OpenAI SDK floor).docs/overview/core-concepts.mdModelfield-list bullet: inline-annotatedisplay_nameas the alias callers send in the API request'smodelfield andmodel_nameas the upstream model ID forwarded to the provider.API Keysection: append a sentence after thekey_hashdescription noting the plaintext is unrecoverable post-create and pointing atPOST /admin/v1/apikeys/:id/rotate.Observability Exportersection: replace the documentation-author-facing second sentence with a user-facing description of what the resource does (OTLP-compatible export to external backends) and when to configure one.docs/configuration/bootstrap-config.mdDefaultcolumn to the field tables onetcd,proxy,admin. Build out theobservabilitysection with a new field table coveringservice_name,log_level,access_log,metrics.prometheus.*,metrics.otlp.*, andtracing.otlp.*, plus a pointer to the dynamic Observability Exporters page. Build out thecachesection with a small field table coveringbackendandredis.Net diff: 5 files,
+58 / -35. Source:git diff --stat.Test plan
Doc-only diff — no
.rsfiles, schemas, configs, or test fixtures touched. The cargo trio was still run end-to-end as the canonical pre-merge gate.cargo fmt --check— PASS (exit 0).cargo clippy --workspace --all-targets -- -D warnings— PASS (exit 0; full tail captured below).cargo test --workspace— PASS (exit 0; full crate-suite results captured below).grep -rln <each affected page> tests/e2e/for all 5 pages returns empty.pnpm testundertests/e2e/is not applicable for this diff.cargo clippytailcargo testsummaryAffected pages
docs/quickstart/self-hosted.mddocs/quickstart/first-model-first-key-first-request.mddocs/quickstart/openai-sdk.mddocs/overview/core-concepts.mddocs/configuration/bootstrap-config.mdPre-merge-check verification log
The three pre-merge checks called out in #325 were resolved as follows:
rust-toolchain.toml(channel = "1.93.1") andCargo.toml(rust-version = "1.93"). The issue body's tentative "Rust 1.79+" floor was bumped to Rust 1.93 or newer in the doc, and a note added thatrustupwill pull the right channel automatically becauserust-toolchain.tomlpins it. For Node.js, neithertests/e2e/package.jsonnor a top-levelpackage.jsondeclares anenginesfloor, so the doc cites Node.js 18 or newer (the OpenAI SDK's published floor at version4.xshipped intests/e2e/package.json).Defaultcolumn. Pulled all default values fromcrates/aisix-core/src/config.rs(this repo's config crate lives inaisix-core, not in a separateaisix-configcrate as the issue body tentatively listed). Values pulled:EtcdConfig::default_prefix → "/aisix",default_dial_timeout / default_request_timeout → 5000ms,ProxyConfig::default_body_limit → 10 * 1024 * 1024,AdminConfig::default → addr "127.0.0.1:0" / admin_keys [] / tls None,ObservabilityConfig::default_service_name → "aisix",default_log_level → "info",default_access_log → true,PrometheusConfig::default → { enabled: true, path: "/metrics" },OtlpConfig::default → { enabled: false, endpoint: None },OtlpTracingConfig::default → { enabled: false, endpoint: None, sample_ratio: 1.0 },CacheConfig::default → { backend: Memory, redis: None }. For fields without an explicit default and withConfig::validateenforcement (etcd.endpoints,proxy.addr), the column readsrequired. For fields whoseDefaultisNone, the column readsnone.GETresponse shape.crates/aisix-core/src/models/provider_key.rs:39definespub secret: Stringwithout#[serde(skip_serializing)], andcrates/aisix-admin/src/provider_keys_handlers.rs:29-38(get_provider_key) returnsJson<ResourceEntry<ProviderKey>>— soGET /admin/v1/provider_keys/:iddoes include the plaintextsecretin the response body, every time. The original draft callout in docs: fix new-developer onboarding gaps across overview, quickstart, and configuration pages #325 read "subsequent reads viaGETdo not include it", which was empirically incorrect. The callout in this PR was adjusted to reflect actual behavior: the warning now states that the plaintext is in both the create response and any subsequentGET, and that the same handling applies any time you read the resource.References
docs: rebuild customer-facing docs across overview, config, cloud, ops, reference, and tutorials(the docs rebuild whose pages this PR amends)Summary by CodeRabbit