mirror of
https://github.com/openobserve/openobserve.git
synced 2026-11-05 08:09:22 +00:00
fac24d59bb15fd3932b6bc1fd19b54b0e8a9f22e
3374
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Hengfei Yang
|
e28de6c6c3 |
fix(infra): serialize every remaining sqlite write path (#13905)
## Problem In local mode with the default sqlite meta store, concurrent metadata writes fail with: ``` Execution Error: error returned from database: (code: 517) database is locked ``` 517 is `SQLITE_BUSY_SNAPSHOT`: a deferred sqlite transaction pins its read snapshot at the first SELECT, and when it later performs its first write the upgrade fails immediately if any other connection committed in between — `busy_timeout` never retries it. The codebase convention is to serialize sqlite writes with `infra::table::get_lock()`, but a full audit found ~100 write functions that never take it, concentrated in the newer subsystems. ## What changed (31 files) - **Table layer, previously zero locks**: `alert_states`, `alert_incidents`, `incident_events`, `dashboards`, `folders`, `ratelimit`, `sessions`, `system_settings`, `alert_composites`, `slo` / `slos` / `slo_budget` / `slo_backfill_jobs`, `synthetics_jobs` / `synthetics_runs`. - **Partially covered modules**: the specific unlocked writers in `users`, `org_users`, `organizations`, `org_ingestion_tokens`, `synthetics_agents`, `synthetics_checks`, `compactor_manual_jobs`, `trial_quota_usage`, `alert_eval_intervals`, `search_job/search_jobs`. - **Service layer**: `db/pipeline_errors` (self-reporting flush), alert deduplication state, the composite-alert scheduler transaction in `core/alerts/scheduler/handlers.rs`, raw anomaly-detection config updates, and two direct writes in `core/alerts/composite/service.rs`. - **Bonus fixes**: `search_job_partitions::submit_partitions` ran its `FOR UPDATE` probe on the pool instead of its open transaction (on Postgres that row lock was not tied to the transaction at all); a ratelimit unit test raced the global `ORM_CLIENT` OnceCell and failed depending on suite ordering. ## Lock-placement rules (why some functions deliberately do NOT lock) The mutex is not reentrant, so the diff enforces **exactly one acquisition per call chain**: - The function that owns the transaction locks before `begin()`; helpers that receive an open transaction (`persist_update_in_transaction`, `apply_status_in_txn`, `record_evaluation_with`, the `alert_states` public wrappers, …) never lock. - Guards are dropped before coordinator emits, `db::scheduler` calls, and helpers that lock internally (e.g. `dashboards::delete_all` locks only after `distinct_values::batch_remove`, which takes the lock itself). ## Verification - Full workspace `cargo check` + clippy clean; 928 infra table tests pass (run twice for ordering stability). - Combined build with the enterprise crate passes; **companion enterprise PR uses the same branch name** `fix/sqlite-lock-remaining` per paired-CI convention. - Live smoke on a sqlite local-mode instance: 40 concurrent folder creations (previously an unlocked path) plus mixed ingestion/metadata load — all requests succeed, zero locked errors in the log. ## Note for reviewers This is the per-call-site fix consistent with the existing convention. A structural alternative (single-connection sqlite RW pool, which makes 517 impossible and would let the ~330 manual lock sites retire) was prototyped separately and can follow as its own change. |
||
 Huaijin
|
08d31371cc |
refactor(metrics): prepare indexed metrics groundwork (#13907)
## Summary - rename the existing TSID-major config, modules, types, merge modes, and persisted file prefixes to indexed-metrics terminology - add the promql-service dependency declarations needed by the follow-up metrics-index work - carry the exact Cargo.lock content from #13899, including h2 0.4.16 and the promql-service dependency entries ## Scope This is no-new-feature groundwork split from #13899. It retains the existing metrics layout mechanics under the new names and does not add .midx sidecar writing, index lookup/query pruning, or index lifecycle handling. The old environment variable and file prefixes are intentionally not kept as compatibility aliases because this experimental path has not been used in a deployment. ## Validation - cargo fmt --all -- --check - cargo metadata --locked --no-deps --format-version 1 - cargo check -p promql-service - cargo test -p config --lib -- layout (5 passed) - cargo test -p search --lib -- datafusion::merge (8 passed) - cargo test -p compaction --lib -- merge (32 passed) - cargo check -p openobserve-jobs |
||
|
Huaijin
|
8ef1fd4cbb |
refactor(compaction): split merge/mod.rs by function into job/stream/file modules (#13900)
## Summary Pure code-move refactor: the 1795-line `src/compaction/src/merge/mod.rs` is split by responsibility into three modules under `merge/`: - **`job.rs`** — job generation: `generate_job_by_stream`, `generate_old_data_job_by_stream`, `generate_downsampling_job_by_stream_and_rule`, plus the shared `job_range_end` helper (now `pub(super)`) and its test - **`stream.rs`** — per-stream merge orchestration: `merge_by_stream` with its private helpers `write_file_list` and `sort_by_time_range`, plus all `sort_by_time_range` tests - **`file.rs`** — file-level merge execution (called by the worker): `merge_files` with its private helpers `cache_remote_files` and `generate_inverted_index` - **`mod.rs`** — now only module declarations and `pub use` re-exports; `tsid_major.rs` untouched No logic changes — only `use` blocks and relative paths were adjusted (`super::is_past_hour` → `crate::is_past_hour`, etc.). The public API paths are unchanged, so the call sites in `worker.rs` and `lib.rs` need no edits. ## Testing - `cargo check -p compaction` and `cargo clippy -p compaction` clean, zero warnings - All 30 merge-module tests pass (`cargo test -p compaction merge::`) |
||
|
Huaijin
|
1dbf32baa2 |
fix(metrics): preserve numeric JSON series hash (#13893)
## Summary - exclude the generated `__hash__` field from JSON metric label stringification - normalize metrics `__hash__` schemas inferred as `Int64` or `Float64` to the canonical `UInt64` type - keep the existing hash calculation order and the rest of the JSON ingestion flow unchanged ## Why The JSON metrics path inserted the numeric series hash and then stringified every label, including `__hash__`. This left JSON-ingested streams with an `Utf8` hash and prevented the UInt64-only TSID-major compaction path from being selected. For hashes above `i64::MAX`, Arrow text JSON schema inference reports the field as `Float64`. The source `serde_json::Number` still retains the exact `u64`, so normalizing only the metrics system hash schema back to `UInt64` preserves the full hash range without changing ordinary floating-point fields. ## Validation - `cargo fmt --all -- --check` - `cargo test -p config --lib utils::schema::tests -- --nocapture` (19 passed) - `cargo test -p openobserve-core --lib metrics::json::tests -- --nocapture` (41 passed) - `cargo clippy -p config -p openobserve-core --lib --tests -- -D warnings` ### E2E Built the PR with `cargo build --features mimalloc` and started OpenObserve with an isolated local data directory. A JSON gauge was ingested through `/api/default/ingest/metrics/_json`, then verified through the stream schema and SQL search APIs: - schema: `__hash__ = UInt64` - schema: `value = Float64` - queried value: `1.25` - queried hash: `14531083140834660755` (greater than `i64::MAX`, returned exactly) - repository metrics API suite: `tests/ingest/test_metrics.py` — 4 passed |
||
|
Huaijin
|
789eee463f |
feat(metrics): TSID-major Parquet layout behind ZO_METRICS_TSID_MAJOR_ENABLED (#13882)
The storage-layout half of #13839, without the `.sidx` series index and its PromQL query path (those stay in #13839, which will rebase on this). ## What Behind `ZO_METRICS_TSID_MAJOR_ENABLED` (default off), Parquet metrics files of a stream whose `__hash__` column is `UInt64` are written ordered by `(__hash__, _timestamp)` so every series is contiguous: - **`config::meta::promql::tsid_layout`** — `metrics_tsid_major_enabled(stream_type)` (flag + configured Parquet), `metrics_tsid_major_stream(stream_type, schema)` (also `__hash__: UInt64`; JSON-ingested metrics store `__hash__` as a string and keep the classic layout), and `MetricsFileLayout { Legacy, HashSorted, TsidMajor }` encoded in the file-name prefix (`tsid-sorted-*`, `tsid-major-v3-*`) so readers and later merges know the physical order without opening the file. - **`MergeMode::HashSorted`** (open hour): the ingester movers and incremental compactor merges write one hash-ordered file named `tsid-sorted-*`. In pack mode the single-segment upload-as-is fast path is skipped for such streams (segments are in arrival order). - **`MergeMode::TsidMajor`** (closed hour): `merges_whole_batch()` — every file of the hour, regardless of size — into size-split `tsid-major-v3-*` files written at RecordBatch boundaries using `ZO_COMPACT_MAX_FILE_SIZE` as logical original size (`search/datafusion/merge/tsid_major.rs`). `MergeMode::for_compactor / for_ingester` take the schema and pick the mode via `metrics_tsid_major_stream`; downsampling keeps precedence. The compactor skips a closed hour that is already all `tsid-major-v3-*`. - **`MergedFile.layout`** decides the output file name (`layout.file_name` / `mark_file_key`) in the compactor and the ingester movers. - **Merging pre-sorted inputs**: `MergeMode::input_sort_order(&files)` declares `FileSortOrder::HashTimestampAsc` when every input is hash-ordered (`tsid-sorted-*` / `tsid-major-v3-*`). Overlapping hash ranges make DataFusion drop the order for its size-based groups; the listing adapter regroups one file per partition (single-file groups are trivially ordered) and keeps the declared `output_ordering`, so `EnforceSorting` turns the `SortExec` into a `SortPreservingMergeExec` over N pre-sorted streams (memory N × batch instead of the whole hour). A batch that mixes layouts declares no order and falls back to the full sort. Two file states only: `tsid-sorted-*` (ordered, not finalized) and `tsid-major-v3-*` (finalized). Nothing changes for streams without the flag, non-Parquet metrics, or a non-UInt64 `__hash__`. ## Known limitation The SQL query path still derives `sorted_by_time` from the SQL shape only, so with the flag on a `SELECT … FROM <metrics> ORDER BY _timestamp DESC LIMIT n` may assume a timestamp order that hash-sorted files do not have. PromQL is unaffected (no ordering declared). To be addressed with the query-side work. ## Validation - `cargo fmt --check`; `cargo check` / `cargo clippy` on config, search, compaction, jobs, promql-service, search_service (`--tests`) clean - `cargo test -p config --lib -- tsid_layout` 2 passed; `cargo test -p search --lib -- merge:: mode:: tsid_major listing_adapter` 9 passed (mode selection, per-layout input order, hash `ORDER BY`, size-split rotation, SPM plan shape); `cargo test -p compaction --lib -- merge` 31 passed - End-to-end on #13839 (debug build, local disk, OTLP gauge 300 series): ingester uploads `tsid-sorted-*` verified `(__hash__, _timestamp)` ordered; hour-end `merge [tsid_major] input sort order: hash_timestamp_asc` → plan `SortPreservingMergeExec` ← `DataSourceExec{N single-file groups}` (no `SortExec`) → `tsid-major-v3-*`; incremental `merge [hash_sorted]` 2 → 1 `tsid-sorted-*`; JSON-ingested stream (Utf8 `__hash__`) keeps the classic layout with no compactor errors. ## TODO (follow-up, not in this PR) **Sort TSID-major streams by `(__hash__, _timestamp)` already at WAL dump time.** `Partition::persist` full-sorts every dumped chunk anyway (today by `_timestamp DESC` in `merge_record_batches`); switching the sort key for TSID-major streams is near-free at dump time and removes the biggest ingester-side cost of this layout — the full `SortExec` over the merge input: 1. **persist**: lexsort the chunk by `(__hash__ ASC, _timestamp ASC)` (same gate as the writers, `metrics_tsid_major_stream`) and stamp a `sort_order: hash_timestamp_asc` key-value into the Parquet footer. A file-name marker is not an option for WAL files — `parse_time_range_from_filename` splits on `.` and a prefix would break WAL time pruning. 2. **ingester mover**: it already reads every input's footer for the schema; declare `HashTimestampAsc` on the table only when *all* inputs carry the footer marker (else `FileSortOrder::None`, full sort — handles mixed files during rolling upgrades / flag flips, same role `MetricsFileLayout::of` plays for the compactor). The merge then becomes a `SortPreservingMergeExec` over pre-sorted files instead of a full sort. 3. **pack, single segment**: the as-is fast path (skipped for TSID-major streams in this PR because segments were time-sorted) can come back — the segment is already hash-sorted, upload it unchanged and mark it `tsid-sorted-*`. 4. **pack, multiple segments**: one `MemTable` partition per segment with a declared sort order, so the in-memory merge is also an SPM instead of a full sort. Query-side is already safe for this change: metrics SQL never declares `sorted_by_time` and PromQL declares no order, so hash-sorted WAL files cannot mislead a reader. The cost is slightly coarser time clustering inside WAL files, negligible at WAL file sizes. Steps 1–2 carry most of the win; 3–4 are incremental. |
||
 udev
|
a6af96289f |
fix(ofga): migrate annotation queue and dataset permissions (#13888)
## What Adds the OpenFGA migration for the `annotation_queues` and `datasets` resources, gated on model version `0.0.39`, following the pattern used for every other resource in `init()`: - a `need_annotation_queues_datasets_migration` flag - a `v0_0_39` version constant - an `existing_model_version < v0_0_39` gate - `get_ownership_all_org_tuple(org_name, "annotation_queues", …)` and `…"datasets"…` in the per-org loop Those two strings are the `OFGA_MODELS` map keys added in o2-enterprise, which resolve to the `annotation_queue` and `dataset` object types. ## Why `annotation_queue` and `dataset` were added to the OpenFGA model without a migration, so existing deployments never receive the `_all_<org>` ownership tuples for them. ## Depends on o2-enterprise#2477, which adds the two types to `model.json` and bumps `o2_version` to `0.0.39`. Without it this migration never fires, since the version gate is never crossed. ## Verification Verified on a local enterprise instance running both changes: ``` [OFGA:Local] model version changed: 0.0.38 -> 0.0.39 [OFGA:Local] annotation queues and datasets permissions migration needed [OFGA:Local] Data migrated to openfga ``` - `org:<org> owningOrg annotation_queue:_all_<org>` and `… dataset:_all_<org>` are written, with no errors in the batch. - Creating a role and saving `annotation_queue` / `dataset` permissions returns 200 and reads back correctly. - OpenFGA `check`, with the `org_context` contextual tuple the app sends, resolves correctly: `AllowAll` on `annotation_queue` permits GET/POST/DELETE; `AllowList` only on `dataset` permits LIST and denies DELETE. - `cargo check -p openobserve-core --features enterprise` is clean. No frontend change is needed — the roles edit page builds its resource list from the `/resources` endpoint, and the entity fetchers for both resources already shipped in #13719. |
||
  
|
0f4b606e8e |
feat(synthetics): super-cluster support, then move synthetics to OSS (#13795)
> ### ⚠️ This PR carries four changes — review alongside openobserve/o2-enterprise#2420 > > 1. **Super-cluster support (consumer half)** — `8ceb041c43 … 5885c57b3a`. > 2. **Synthetics OSS migration** — `3a380087a3 … 2596300dbf`, nine commits — one per build-order step, plus a ninth for fixes found by running the binary. Moves synthetics out of `o2_enterprise` into `src/synthetics` (`openobserve-synthetics`), leaving only the private-VPC-agent path enterprise: `job_api::lease`, `agent/*`, private locations and the agent-setup install flow. Paired with openobserve/o2-enterprise#2420. > > 3. **Last-check columns read from the results stream** — `6af411e808`, `e7a3f2c759`. Outside the region whose scheduler runs a check, the list showed a stale status and a blank Last check / response time: `last_check_status` replicates only on change, `last_triggered_at` is a runtime column that does not replicate, and `last_response_ms` was never populated anywhere. The `synthetics_results` stream already carries all three and is federated, so the list API reads it instead of replicating the columns — no queue traffic, no schema change. Best-effort: any miss keeps the stored DB values, so search being unavailable cannot take the list down. OSS-only; no enterprise counterpart. > > 4. **Check secrets are readable in every region** — `d47a6b2345` (+ o2-enterprise `da71d7ec`). The per-org DEK is minted by each region for itself, so a check created outside the scheduler region replicated fine and then silently never ran — `resolve` could not decrypt its credentials (o2-enterprise#2451). Secrets now cross the queue **in clear** and the applier re-encrypts them under its own DEK on arrival, fixing the class at the replication boundary with no key to distribute or keep in sync. > > ⚠️ **This puts customer secrets in plaintext on the inter-region bus** — a deliberate, recorded trade-off (`o2oi_` ingest tokens and probe tokens already travel it in clear). Securing the bus itself (TLS on the gateway links, short retention on secret-carrying subjects) is the real fix and is **not** in this PR. > > ⚠️ **Deploy consumers before producers.** The payload's bytes did not change, only the meaning of the secret strings, so an un-upgraded consumer handed the new payload would write customer passwords to disk in clear. The plaintext-carrying messages therefore use new variant names (old ones retired as `CreateEncrypted`/`UpdateEncrypted`), so an old region fails serde and redelivers until upgraded instead of silently persisting plaintext. > > An OSS build can now create, schedule, dispatch to Lambda, take `resolve`/`ack` and report a check. 7 393 lines moved into the new crate; 679 lines stay enterprise. > > **🛑 Deployment prerequisite (2 of 2).** `ZO_SYNTHETICS_ENCRYPTION_KEY` must be set to the **same** base64 64-byte value in every region of a super cluster. Without it synthetics falls back to the per-org DEK and checks carrying secrets silently fail to run outside the scheduler region. It is read once at startup and pinned — changing it later requires a restart, and changing it at all orphans every secret already encrypted under the old value. > > **🛑 Deployment prerequisite (1 of 2).** 14 `O2_SYNTHETICS_*` env vars are hard-renamed to `ZO_SYNTHETICS_*` with **no fallback read**, and `ZO_SYNTHETICS_ENABLED` defaults to `false`. A deployment still setting the old names does not lose tuning — **synthetics stops running entirely, and the first symptom is checks silently not firing.** Update env vars *before* rolling the binary. > > #### Things a reviewer should look at first > > - **`super_cluster_queue` does not depend on `openobserve-synthetics`** (`cargo tree` → 0), which is what keeps the applier's loop-prevention guarantee a property of the crate graph rather than a convention. `a7d5fdd969` adds the assertion that fires if that ever changes. > - **`job_upload` was skipping authorization in OSS builds** (`4c3a0b8f0d`). Unreachable before, because no synthetics route was registered in OSS; registering it would have made it an unauthenticated artifact upload. `authorize_probe` is ungated and the bypass is gone. > - **The dependency edge had to reverse in a single commit** (`3247d4093f`). Cargo rejects an `o2_enterprise` ↔ OSS package cycle at the manifest level — `optional` and feature gating do not help — so the two edges could never coexist, and everything needed for the reversal lands together. This is why the spec's steps 4 and 5 are one commit and the config moved earlier than planned. > > #### Accepted OSS limitations, all documented in the commits > > - No notifications: `notify_check_result` ends in `alerts::alert::dispatch_notification`, which is enterprise. An OSS build runs the check and records the run; it has nowhere to send an alert, exactly as for alerts themselves. > - No per-resource RBAC on synthetics, and no OpenFGA ownership tuples — both are enterprise capabilities across the platform. > - Private locations are refused with a message rather than created. > - Agent-served public `net-*` pools are unavailable, so an OSS user needs a Lambda for every public pool (spec §1). --- Consumer half of synthetics super-cluster support. **Depends on openobserve/o2-enterprise#2420** (producer, message types, queue struct) — and **this side must deploy first**: `MessageType` bytes 83–85 hit a panicking `unreachable!()` in `From<u8>` on a consumer that does not know them, which takes down the subscriber task rather than failing one message. Based on `fix/lease-claimed-by-2270` (#13785), not `main`, so the diff is only this work. ## What this fixes Synthetics writes are not querier routes, so a create lands on ingesters in whichever region received the request and commits to that region's Postgres. There is no synthetics topic on the super-cluster queue, so a check created in a region that runs no scheduler is never seen by the region that does: the UI shows it enabled, the scheduler has never heard of it, and nothing reports the gap. ## Commits | commit | what | | --- | --- | | `8ceb041c43` | orphan-detection query + metrics | | `1c3e10da48` | apply replicated check config | | `79e6abc052` | apply replicated locations and probe tokens | | `16d4fa0fde` | render unknown live status instead of a false pending | | `5885c57b3a` | share the job-cluster predicate | ## Design notes worth reviewing **Config columns only, and the list is not restated.** `synthetics_checks::update` rebuilds its `ActiveModel` from the row already in this region, and `update_mutable_fields` writes only user-editable columns — so `next_run_at` and the alerting counters keep local values for free. Hand-listing them in the consumer would duplicate a list that already exists and let the two drift. A test asserts every runtime column comes back `Unchanged`. **`put()` is deliberately unused.** Its insert branch builds from `build_active_model`, which ends in `..Default::default()` and never assigns `owner`, `tz_offset` or `next_run_at` — a replicated create through it would drop the owner and fire a scheduled-later check immediately. **`create()` gains `use_given_id`**, matching `table::alerts::create`. It generated its own uuid, which would have made every region invent a different primary key for the same check. **Consumers apply through `infra::table` only.** The publish lives one layer up in the enterprise service, and `infra` cannot reach the enterprise crate — that layering is what stops a region re-broadcasting what it just applied. `source_cluster` does not cover this: it only stops a region re-consuming its *own* message. Each consumer greps its own source to keep this true. **Runtime tables are never replicated** — `synthetics_jobs`, `synthetics_runs`, `synthetics_agents`, and the locations `down_notified_at` claim. Asserted at both producer and consumer. ## Known gaps, named not hidden - If two regions mint a token (or location) with the same name for one org before either message crosses, the UNIQUE constraint rejects the arriving row. Local row wins, logged loudly. Erroring would redeliver to dead-letter, which is worse. - Pre-existing and untouched: `write_triggers_stream`/`write_results_stream` in the enterprise reaper post to `_meta` with the *org's* ingest token, which 401s, and neither checks response status. Orphan detection fixes this in its own path only. - Pre-existing: `create` never sets `tz_offset`, so a new check gets DB default 0 until its first edit. Replicates identically everywhere, so it causes no divergence. ## Spec corrections The spec (`docs/synthetics-lcl/2026-08-11-p1-3-super-cluster-spec.md`, local notes) is wrong in five places, all found by reading the tree: §1's invariant evidence (see the enterprise PR), §10's never-run guard, §4's column list (and it lists six runtime columns — there are seven, `alerting` is missing), §3's "private rows read-only when remote", and §7's claim that non-scheduler regions render OFFLINE (they render `pending`). ## Verification infra 42, super_cluster_queue 32, jobs 21, web 111/111 vitest + type-check + design lint, `cargo fmt --check` and clippy clean, full `--features enterprise` build. Pre-existing failures on this branch, unrelated and untouched: `super_cluster_queue::schemas::tests::test_parse_key_{valid,metrics}`, `infra::table::ratelimit::tests::test_add_batch_missing_rule_id`, and 8 web vitest failures in `buildPayload.spec.ts`/`payloadContract.spec.ts`. --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Hengfei Yang
|
46e28fe42e |
fix(auth): return 403 instead of panicking when user is not in the org (#13878)
## What
`check_stream_permissions` in `src/core/src/authz.rs` unwrapped the
result of the user lookup:
```rust
let user: User = crate::users::get_user(Some(org_id), user_id).await.unwrap();
```
When a non-root user hits an endpoint that checks permissions against an
org they are not a member of, `get_user` returns `None` and the handler
panics. The connection is aborted and the load balancer surfaces it as a
502.
The concrete trigger is `GET /api/{org}/search/profile` (search
inspector): it always checks against the `_meta` org's `default` traces
stream regardless of the org in the URL, so any SSO user who is not a
member of `_meta` got a 502 when clicking the Inspect button.
This change replaces the unwrap with a let-else that returns the same
`403 forbidden("Unauthorized Access")` response as the function's
existing deny path.
## Why
A user not being a member of the checked org is a normal deny scenario,
not an internal error. All 15 call sites of this function (search,
traces, alerts handlers) treat `Some(response)` as an opaque deny, so
they all benefit from the graceful path.
## Reviewer notes
- The change is entirely inside `cfg(feature = "enterprise")` code.
Verified with the enterprise Cargo.toml: `cargo check` and `cargo clippy
-D warnings` on `openobserve-core`, `openobserve-api-search`, and
`openobserve-api-management` all pass.
- Users who should be able to use Search Inspect still need search
permission on the `default` traces stream in `_meta` — this PR only
fixes the failure mode (502 → 403), not the permission model.
|
||
|
udev
|
84b666ce0b |
fix: annotation_queues item hydration intermittent 500s (#13881)
## Summary
RCA (2026-08-19, 08:20-08:22 UTC): `GET
/api/{org}/annotation_queues/{queue_id}/items/{item_id}` (and its
`/reviews` sibling) intermittently returned 500. Originally attributed
to search work-group admission capacity exhaustion; tracing the actual
code found a different root cause.
**Real mechanism**: `eval_jobs::search::execute()`'s registered
executors (in `src/jobs/src/job/mod.rs`) called
`search_service::search("", ...)` with a hardcoded empty `trace_id`.
With no propagated OTel span in that detached task,
`search_service::search()` falls back to OpenTelemetry's *invalid* trace
ID for every such call - the same `000...000` seen in every failing log
line. Every concurrent internal search therefore shared one
`QueryManager` map key: one query's cleanup (`SEARCH_SERVER.remove`)
deleted another's still in-flight registration, so the next
`insert_sender` failed with "trace_id not found" -> `SearchCancelQuery`
-> 500. `work_group_checking` never reached the real concurrency-wait
branch in this failure mode - "work-group capacity" was never the actual
mechanism.
## Changes (3 commits)
1. **`fix(annotation-queues): merge item detail and reviews into one
call`** - the Queue Workbench always fetches item detail + reviews
together (confirmed: no caller ever needs one without the other,
including after review submission); fold them into one backend call,
cutting the per-item load from 2 HTTP round trips to 1. See companion PR
openobserve/o2-enterprise#2473 for the response-shape change this
depends on.
2. **`fix(annotation-queues): map search admission cancellation to
429`** - `annotation_queue_error_response` unconditionally 500'd on
`AnnotationQueueError::Search`/`Hydration`, even when the underlying
cause already has a defined 429 `http_status()` (`SearchCancelQuery` /
`RatelimitExceeded`). Downcast and map correctly instead.
3. **`fix(search): stop colliding on trace_id in internal eval
searches`** - the actual root-cause fix: generate a real per-call
`trace_id` via `config::ider::generate_trace_id()` at all three internal
`search_service::search("", ...)` call sites instead of passing `""`.
## Dependency
This PR's enterprise build depends on openobserve/o2-enterprise#2473
(same branch name `ud/fix-annotation-queue-hydration-500s`) via the path
dependency in `Cargo.toml.openobserve` - that PR adds the `reviews`
field on `AnnotationQueueItemDetail` this PR's merge (commit 1)
consumes.
## Test plan
- [x] `cargo build --features enterprise`
- [x] `cargo test -p openobserve-api-management --lib --features
enterprise annotation_queue` - 11 passed (incl. 2 new tests for the 429
mapping), no regressions
- [x] `cargo test -p o2_enterprise --lib annotation_queues` (companion
repo) - 16 passed, no regressions
- [x] `cargo clippy --features enterprise -p openobserve-jobs -p
search_service -p openobserve-api-management --no-deps` - clean (no
warnings in touched files)
- [x] `cargo fmt --check` - clean
- [x] Frontend: `npx vitest run` on `llm-queues.service.spec.ts` +
`QueueWorkbenchPage.backTarget.spec.ts` - 9 passed
- [x] `vue-tsc --noEmit` (`type-check:app`) - clean
🤖 Generated with [Claude Code](https://claude.com/claude-code)
|
||
|
Hengfei Yang
|
c572727863 |
fix: wait for WAL settle window before merging the previous hour (#13880)
## What The compactor's hour-merge gate (`is_past_hour`) now measures the 3 × `ZO_MAX_FILE_RETENTION_TIME` settle window from the **end** of the offset hour instead of its start. ## Why The original design delayed merging the previous hour until the current hour's top plus 3 × `ZO_MAX_FILE_RETENTION_TIME` (default 30 min), giving data still buffered in ingester WAL when the hour closed time to upload. Since the multi-worker compactor refactor (#3526), the gate compared `now - offset` (offset = start of the hour to merge) against the window — a condition that is always already satisfied the moment the hour passes (1 h > 30 min), so the check was dead code and the merge fired right at the top of the hour. Files uploaded afterwards for that hour arrived as small files that the normal path never re-merged; the old-data fallback only picks an hour up once it has ≥ 10 small files, so a handful of stragglers stayed unmerged forever. ## How - `is_past_hour` rounds `offset` up to its hour's end and requires `now - hour_end > 3 * max_file_retention_time`. One fix covers all three call sites: scheduled job generation, the incremental-mode decision in `merge_by_stream` (jobs inside the settle window now correctly run incrementally: seal full-size groups only, carry the remainder), and the file_list dump gate. - Catch-up of older hours is unaffected — their hour end is long past the window, so they pass immediately (the behavior #3526 wanted to preserve). ## Reviewer notes The behavior change operators will see: the previous hour's final merge now starts ~30 minutes (3 × retention) after the hour closes instead of immediately, matching the pre-#3526 design. Streams with the ingester incremental trigger enabled keep getting intra-hour merges during that window. |
||
  Yashodhan JoshiandYashodhan Joshi
|
3f40d13f21 |
fix: always create file_list_dump_stats table (#13873)
Currently observed on v0.92.x , when file list dump is not enabled we do not create the dump stats table, but for some reason compactor tries to use the table and fails. Apart from this, even in the case when someone has not initially setup the dump enabled, and later enables it, the table will be missing as migration will not be run if db schema version matches. We should always create all the table schemas, as plain schema and indices will not take much db resources, but would be problematic to later debug and fix if missing. Thus this pr always created the file_list_dump_stats table irrespective of the env value. Signed-off-by: Yashodhan Joshi <yjdoc2@gmail.com> Co-authored-by: Yashodhan Joshi <yjdoc2@gmail.com> |
||
|
Huaijin
|
0641e1e486 |
refactor(merge): decide the merge kind once (MergeMode) and share the DataFusion pipeline (#13866)
## Why The kind of merge — plain time-ordered file, trace time-index aggregation, file-list ordering, enterprise metrics downsampling — was re-derived at every layer, each from stream type / stream name / config / batch time range: - compactor scheduler (`merge_by_stream`): `skip_group_files` (downsampling rule lookup #1) → whole-hour batch vs size groups - `merge_files`: `is_match_downsampling_rule` (lookup #2) → early returns, size-limit bypass - `merge_parquet_files`: lookup #3 + SQL selection by stream type, with `is_ingester: bool` standing in for the writer's role `merge_parquet_files` and `merge_parquet_files_with_downsampling` also carried two copies of the DataFusion context / union table / plan / execute-stream / channel / read-task plumbing. Reading the code meant reconciling three derivations of the same fact to know what a batch would produce. ## What - **`search::datafusion::merge::mode::MergeMode { Classic, TraceTimeIndex, FileList, Downsampling(rule) }`** — decided once: `MergeMode::for_compactor(stream_type, stream_name, max_ts)` by the scheduler (carried in `MergeBatch.mode`), `MergeMode::for_ingester(stream_type, stream_name)` by the movers. Every layer only asks it questions: `merges_whole_batch()`, `input_sort_order()`, `sql(schema)`. - **`MergeOutput { file_format, parquet_compression }`** (`for_ingester` / `for_compactor`) replaces the `is_ingester` bool. - **`merge_parquet_files(schema, tables, bloom, meta, &mode, output)`**: one `run_merge_query()` pipeline for every mode, then the mode's writer. Downsampling is a `MergeMode` arm calling `downsampling::write_files` instead of an early-return second implementation (`merge_parquet_files_with_downsampling` removed, −81 lines). - Compactor: `skip_group_files` / `is_match_downsampling_rule` and their `cfg(enterprise)` twin declarations are gone; `compaction` no longer imports `get_largest_downsampling_rule`. Commit 1 (`672ab315a1`) is pure refactor, no behavior change. ## Behavior change (commit 2, `8030eefd04`): whole-hour merges only for a closed hour, over every file of the hour Metrics downsampling used to merge the whole batch whenever the rule matched the batch's newest sample — including incremental rounds on the still-open hour — and it only ever saw the files below the 95%-of-`ZO_COMPACT_MAX_FILE_SIZE` bound of `query_for_merge`: files already above the bound were never downsampled, and small outputs were re-aggregated round after round. - `MergeMode::for_compactor(stream_type, stream_name, range_end_ts, finalize)` is decided **once per hour, before the file_list query**, from the end of the job's date range; a whole-hour mode (downsampling) is only chosen when `finalize` (the hour is closed). Incremental merges of a matching hour use the plain per-stream mode and leave the aggregation to the hour-end pass. - `query_for_merge` takes the `original_size` bound from the caller (`infra::file_list::merge_max_original_size()` = the classic 95% rule); a whole-hour merge asks for **every** file of the hour (`i64::MAX`), the same input range the TSID-major hour-end merge in #13839 uses. - Rule matching uses the hour's end instead of the batch's max `max_ts` (≤ 1h difference; the hour is decided as a unit). ## Guards (commit 3, cherry-picked from #13839) - compactor: fail the merge instead of deleting the sources when `merge_parquet_files` produced no files - search storage `file_list::get_scan_selection` strips the multi-account `{account}/::/` prefix so selections registered by `set` are found again (+ round-trip test) - `deleted.rs`: `delete_from_storage` takes the file slice and a key closure (`Cow`) instead of a pre-collected `Vec` ## Not in this PR The write stage still dispatches to four writers (`write_parquet`, `write_vortex`, `write_downsampled_parquet`, `write_downsampled_vortex`) that share the open / write / rotate / close skeleton and differ only in the rotation policy and how `FileMeta` is derived. Unifying them into one rotating writer is a follow-up (needs a decision on deriving `min_ts/max_ts` from the output batches for the classic path too). ## Validation - `cargo fmt --check`; `cargo clippy -p search -p compaction -p openobserve-jobs --tests` clean - `cargo test -p search --lib -- merge::` 6 passed (new: `mode_by_stream`, `sql_by_mode`, `output_file_format_uses_parquet_for_ingester_metrics`; trace-time-index merge test now goes through `MergeMode::for_ingester` and asserts `TraceTimeIndex`) - `cargo test -p compaction --lib -- merge` 30 passed; `cargo test -p infra --lib -- file_list` 49 passed - `downsampling.rs` is enterprise-only and not compilable locally; `write_files` / `pub(super) generate_downsampling_sql` were edited by hand and need the enterprise CI. |
||
|
Hengfei Yang
|
4be23092a7 |
fix: skip session_id in trace time index merge when files lack the column (#13863)
## Problem Ingester merge jobs fail repeatedly for trace time index streams: ``` merge_parquet_files error for stream: default/metadata/trace_time_index__evaluator, ... err: Schema error: No field named session_id. Valid fields are tbl._timestamp, tbl.max_ts, tbl.min_ts, tbl.trace_id. ``` The failing file can never merge, so the job retries it forever and the file stays stuck in the WAL, never uploaded to object storage. ## Root cause - The trace time index write path only inserts a `session_id` value when the trace actually carries one; the field is nullable in the schema. - When the ingester persists WAL files, `merge_record_batches` prunes columns that are entirely null — so a file whose records all lack a session is written **without the `session_id` column at all**. Since most traces have no session id, this is the common case, not an edge case. - The ingester merge job builds the DataFusion table schema from the files' own fields (intentionally, to avoid the wide latest schema), and `merge_parquet_files` hardcoded `MAX(session_id) AS session_id` in the merge SQL for trace time index streams — planning fails when the column is absent. ## Fix Only include `MAX(session_id) AS session_id` in the merge SQL when the resolved schema actually contains the field. Files without the column merge with the original 4-column shape (matching what search already handles via schema adaptation); when any file in the batch has the column, the union schema null-fills the rest and the full query works. This also covers files written before `session_id` was added to the index schema. ## Testing Existing `test_trace_time_index_merge_aggregates_by_trace_id` covers the with-session path through the conditional SQL; verified the module's tests pass. |
||
|
Huaijin
|
4e805038fe |
refactor: merge result / access-plan / deletion helpers groundwork for TSID-major metrics (#13861)
Behavior-neutral refactors split out of #13839 so that PR only carries the TSID-major feature. Nothing here changes what is written, read or deleted; every commit is independently reviewable. ## Commits - `cba75d244b` **promql**: `apply_matchers` split into `matcher_predicates(schema, matchers) -> Vec<Expr>` + `apply_matchers` (unchanged behavior; lets other inputs evaluate the same matcher semantics). - `b5ff6fed21` **parquet**: `new_parquet_writer` / `append_metadata` generic over `W: AsyncFileWriter` instead of `&mut Vec<u8>`; existing callers unchanged. - `b07d84216a` **merge**: `MergeParquetResult::{Single, Multiple}` → `MergeParquetResult { files: Vec<MergedFile { buf, meta }>, file_format }` + `into_single()`. The compactor's two copies of the upload / index / push sequence become one loop (`compressed_size` is set from the buffer for every file, as the `Multiple` arm already did); downsampling maps its `(bufs, metas)` into it; ingester movers use `into_single()` instead of panicking/erroring on `Multiple`. - `014b6c5721` **compactor**: `deleted.rs` `derived_files()` / `delete_from_storage()` replace three copies of collect / `storage::del` / ignore-not-found (data, `.ttv`, flattened). - `7163786362` **search**: `generate_parquet_access_plan_from_ranges` extracted from the bitmap path (`set_slices` → ranges); an out-of-range, empty or unsorted range is rejected with `assert!` (`56de386522`; a producer that violates the sorted/non-overlapping contract is a bug, not something to degrade around) instead of the previous `unreachable!()` on the bitmap path. Adds `FileSelection::RowRanges(Arc<Vec<Range<usize>>>)` as the matching selection kind (no producer yet; Parquet only, Vortex full scan like `RowGroups`). - `754014ccf3` **test**: `ListingTableAdapter` plan-shape test for `FileSortOrder::HashTimestampAsc` (from #13853): three overlapping hash-sorted files → `SortPreservingMergeExec` over three single-file groups, no `SortExec`, globally ordered rows. `tempfile` as `search` dev-dependency. ## Validation - `cargo fmt --check` - `cargo clippy -p config -p promql -p search -p compaction -p openobserve-jobs -p search_service -p promql-service --tests` clean - `cargo test -p promql --lib -- utils` 10, `-p search --lib -- merge:: helpers listing_adapter` 14, `-p compaction --lib -- merge deleted` 31, `-p config --lib -- test_file_key` 4 passed - `merge/downsampling.rs` is enterprise-only and was updated by hand; needs the enterprise CI compile. |
||
|
71ac1071be |
fix: draft workflows (#13666)
Signed-off-by: Yashodhan Joshi <yjdoc2@gmail.com> Co-authored-by: Yashodhan Joshi <yjdoc2@gmail.com> Co-authored-by: Yashodhan Joshi <yashodhan@openobserve.ai> |
||
|
Huaijin
|
617303a307 |
refactor(search): drop FileSortOrder from the distributed plan wire format (#13856)
Follow-up to #13853. The `(__hash__, _timestamp)` order is only relevant to the compactor merge; distributed query plans never carry it, so the wire-level `FileSortOrder` is unnecessary. ## Changes - `plan.proto` / generated: remove `enum FileSortOrder` and `NewEmptyExecNode.sort_order = 8`; `NewEmptyExecNode` keeps only the legacy `bool sorted_by_time = 6`. - `codec/empty_exec.rs`: encode/decode with the bool only (`FileSortOrder::from_sorted_by_time`); drop the enum-first/legacy-fallback logic and its test. - `sort_order.rs`: drop `to_proto` / `from_proto` and the round-trip test; `HashTimestampAsc` is documented as compactor-merge only. - `empty_exec.rs` tests: cover `None` / `TimestampDesc` output ordering only. Rolling upgrade: #13853 nodes still write and read `sorted_by_time`, nodes with this change only use `sorted_by_time` — no incompatibility. ## Validation - `cargo check -p search -p search_service --tests`, `cargo fmt --check` - `cargo test -p search --lib -- sort_order empty_exec` 26 passed |
||
|
Huaijin
|
516e225bde |
refactor(search): replace sorted_by_time bool with FileSortOrder enum (#13853)
## Summary Every table provider, `NewEmptyExec`, the memtable `SortExec`, the merge `ORDER BY` and the `ListingTable` `file_sort_order` shared a single `sorted_by_time: bool` that implicitly meant `_timestamp DESC`. TSID-major metrics files (see #13839) are sorted by `(__hash__ ASC, _timestamp ASC)` instead, so a bool can no longer describe the physical layout of the files behind a table. This PR introduces `search::datafusion::sort_order::FileSortOrder { None, TimestampDesc, HashTimestampAsc }` as the single place that knows how each order maps to DataFusion logical / physical sort expressions, and threads it through the whole pipeline: - `DataFusionContextBuilder::sort_order` → enables `split_file_groups_by_statistics` for any sorted order - `TableBuilder::sort_order` → `ListingOptions::with_file_sort_order(order.logical_sort_exprs())` - `ListingTableAdapter` → regroups files by statistics using the declared order (`order.physical_ordering(schema)`) instead of a hard-coded `_timestamp DESC`; the "reverse file groups" fallback is kept only for `TimestampDesc` - `NewMemTable` → `SortExec` on the declared order (no-op if a sort column is missing) - `NewEmptyTable` / `NewEmptyExec` → equivalence properties from the declared order; display shows `sort_order=timestamp_desc` - `search_service` (`flight` / `storage` / `wal` / `create_tables_from_files`) pass the enum through - merge `ORDER BY` string derived from `FileSortOrder::TimestampDesc.order_by_clause()` ### Wire format `NewEmptyExecNode` gains `FileSortOrder sort_order = 8`. The legacy `bool sorted_by_time = 6` is still **written** (`true` iff `TimestampDesc`) and **read** as a fallback when `sort_order` is `NONE`, so mixed-version clusters keep working during rolling upgrades. ### No behavior change Every previous `sorted_by_time(true)` maps to `FileSortOrder::TimestampDesc`; `Sql.sorted_by_time` stays a bool (it describes the query, not the files) and is converted at the boundary. `HashTimestampAsc` is not yet used by any caller — that lands with the TSID-major work. ## Validation - `cargo fmt --all -- --check` - `cargo check --workspace --tests` - `cargo clippy -p search -p search_service -p compaction -p openobserve-jobs --tests` clean - `cargo test -p search --lib`: all pass except 12 pre-existing `sql::histogram` failures caused by a local `.env` override (`ZO_HISTOGRAM_BREAKDOWN_FIELDS=""`), unrelated - `cargo test -p search_service --lib -- grpc::` 16 passed - New tests: `sort_order` (logical/physical exprs, ORDER BY clause, proto roundtrip, missing column), `NewEmptyExec` output ordering per variant, codec roundtrip for all variants + legacy-flag fallback decoding |
||
  
|
69bd367d9b |
feat: notification dependency view — where-used for templates, destinations & alerts (#13820)
## What Adds a **"View dependencies"** affordance to the **Templates**, **Notification Destinations**, and **Alerts** lists. A small `graph-1` icon on each row opens a compact popover that shows everything that entity connects to along the **Template → Destination → Alert** chain, with search, pagination, and per-row **Open** / **Delete**. ## Why The linkage between alert templates, destinations, and alerts is invisible today. There is **no reverse-lookup API** — the only way to learn "which alerts use this destination" is to try deleting it and hit the backend's `409 "Destination is currently used by alert: X"`. That's a dead end, not a map. This surfaces the dependency proactively so you can see the blast-radius *before* you change or delete something, and act on it in place (open or delete a related entity, with the same `409` guard still protecting in-use items). ## Design The three entities are linked **by name** (there are no ID foreign keys): - `Alert.destinations: string[]` → `Destination.name` - `Destination.template` → `Template.name` (a destination's default) - `Alert.template` → `Template.name` (an alert-level override) ```mermaid flowchart LR T["Template"] -->|"Destination.template"| D["Destination"] D -->|"Alert.destinations[]"| A["Alert"] T -. "Alert.template (override)" .-> A ``` Because the alerts **list** DTO previously omitted `destinations`, the whole graph would have needed N per-alert detail fetches. Instead a small **opt-in additive field** on the list item makes the entire graph **three list calls**, cross-referenced client-side and cached per-org: ```mermaid flowchart TD Row["List row · graph-1 icon"] --> Pop["DependencyChainPopover<br/>(OPopover + ConfirmDialog)"] Pop --> Panel["DependencyUsagePanel<br/>search · collapsible sections · pager"] Panel --> Rows["DependencyUsageRow ×N<br/>Open · Delete"] Panel --> Load["useDependencyGraph.loadGraph()<br/>per-org cache"] Load -->|"alerts?include_dependencies=true<br/>+ destinations + templates (3 list calls)"| Build["buildGraph() → nodes + edges"] Panel --> Focus["buildFocusChain(graph, focus)<br/>directional: a shared template never<br/>drags in sibling destinations"] Rows -. "requestDelete(node)" .-> Pop Rows -. "open(node) — same-kind, in-place" .-> Pop Pop -. "ConfirmDialog → service.delete()<br/>(409 'used by' → toast)" .-> Done["emit deleted → list refreshes"] ``` Why the ConfirmDialog lives in the popover **wrapper**, not the panel: a modal can't render inside the popover without dismissing it, so the row/panel only *request* a delete and the wrapper owns the real confirmation. **Open** works the same way for the in-place editors: templates/destinations edit on their own list route (which only opens from the page's mount-time handler), so a same-page open is bubbled to the host page to invoke its native editor rather than a no-op `router.push` to the same route. ## UX / behaviour - **Where-used, grouped:** collapsible **Templates · Destinations · Alerts** sections, each a clickable row (kind-coloured icon, name, and for destinations an "N alerts" **total-usage** badge). - **Search-first:** one search box filters all groups — the key affordance when a destination feeds thousands of alerts. - **Paged:** the alerts group renders **50 at a time** with a pager pinned beside its (sticky) header, so 4,000-alert destinations stay responsive. - **Act in place:** every row has **Open** (templates/destinations → their editor; alerts → the alert's detail page) and **Delete** (the same `ConfirmDialog` the list pages use; the backend `409` still blocks in-use items). - Consistent trigger on all three lists; **purely additive** — no existing button, icon, or column was moved or replaced. ## Backend change `ListAlertsResponseBodyItem` (`src/api/management/src/models/alerts/responses.rs`) gains two additive fields — the linkage the dependency view cross-references: - `destinations: Vec<String>` - `template: Option<String>` (the alert-level override) They serialize **only when the caller passes `?include_dependencies=true`** (which just the dependency popover does); the default paginated alerts-list path is unchanged, so ordinary consumers pay no extra bytes and receive no module-scoped destination/template names. Both are also `skip_serializing_if`-empty. Backward-compatible, OSS, no new endpoint. ## Testing - **Unit (vitest) — 17 tests:** - `useDependencyGraph.spec.ts` — `buildGraph` (linkage, unused/orphan, dangling refs, override edges, shared destinations) and directional `buildFocusChain` per focus kind (incl. the alert-focus total-usage count). - `DependencyUsagePanel.spec.ts` — sections render every kind, search + 50/page paging, collapse, `requestDelete`, the same-kind `open` bubble, and the `include_dependencies` opt-in. - `DependencyChainPopover.spec.ts` — trigger + lazy panel mount, `requestDelete` → ConfirmDialog → API + `deleted`, and the `open` bubble + close. - **Backend:** serialization test asserts `destinations`/`template` are omitted when empty. - `type-check`, `lint:design:strict`, and `prettier` all clean. - A popover e2e (Playwright) is a follow-up in a separate PR — the earlier spec targeted an interim dedicated page that this PR does not ship, so it was removed. ## Notes - Net change is popover-only: an earlier iteration added a dedicated Reliability page + route + rail entry; those were dropped in favour of the on-the-fly popover, so `router.ts` / `navGroups.ts` are untouched vs `main`. - Icon `graph-1` was added to the icon registry (additive) for the popover trigger. - **Deferred (see design doc Non-goals):** a server-side where-used / `?destinations=` endpoint so the popover fetches only the neighbourhood instead of the full alert list; and enterprise RBAC name-filtering on the gated fields. Design at: https://github.com/openobserve/designs/blob/main/alerts/notification-dependencies/design.md (designs#8: `alerts/notification-dependencies/design.md`) --------- Co-authored-by: Abhay Padamani <141122205+ktx-abhay@users.noreply.github.com> Co-authored-by: ktx-abhay <abhay.padamani@kiara.tech> |
||
|
Hengfei Yang
|
42d4e2a203 |
fix: return empty when usage is not there instead of report error (#13842)
- [x] Should start check job after node start, otherwise it will report gRPC can't connect. - [x] Should skip usage check when there is no usage stream, otherwise it will always report error. |
||
|
Hengfei Yang
|
6ca0eec107 |
fix: MCP tool calls return 404 when ZO_BASE_URI is set (#13844)
Fixes #13845 ## What changed The MCP shared HTTP client (used to execute tool calls against the local REST API) now includes `ZO_BASE_URI` in its base URL: `http://localhost:{port}{base_uri}` instead of `http://localhost:{port}`. ## Why When `ZO_BASE_URI` is set (e.g. `/abc`), all HTTP routes are mounted under that prefix. The MCP endpoint itself was reachable (`POST /abc/api/default/mcp` → 200), but every tool execution failed: the internal client sent requests to the unprefixed path and got a 404. ``` INFO rmcp_openapi::http_client: Final URL: http://localhost:5080/api/default/streams INFO config::axum::middlewares::access_log: 127.0.0.1 "GET /api/default/streams HTTP/1.1" 404 ... ``` ## Notes for reviewers - `cfg.common.base_uri` is already normalized at config load to have no trailing slash (empty when unset), so the empty-prefix behavior is unchanged. - The join semantics are safe for path-prefixed base URLs: rmcp-openapi's `with_base_url` appends a trailing slash and `build_url` strips the tool path's leading slash before `Url::join`, covered by the crate's `test_build_url_with_base_url_containing_path`. So `/abc` + `/api/{org}/streams` → `/abc/api/{org}/streams`. - The other `localhost:5080` occurrence in `tools.rs` is a test-only mock client; the OAuth protected-resource metadata handler already includes `base_uri`. - `cargo check -p openobserve-mcp` and `cargo test -p openobserve-mcp` pass. |
||
|
Hengfei Yang
|
9d76505882 |
feat(traces): resolve session time ranges through the trace time index (#13837)
Design at: [https://github.com/openobserve/designs/blob/main/infrastructure/file-list/session-time-index.md](https://github.com/openobserve/designs/blob/main/infrastructure/file-list/session-time-index.md) ## Summary - carry an optional `session_id` attribute on trace time index records, resolved from span attributes by the existing `SESSION_ID_COLUMNS` precedence; the first non-empty value in the request wins for the whole trace, stamped onto all of its hour-bucket records - keep the aggregation key `(hour, trace_id)` unchanged; hourly compaction picks the attribute up with `MAX(session_id)`, which ignores NULLs - reuse the locate/expand timer for sessions with a 7-day expand window (traces keep 24h); a UUID v7 session id doubles as a probe anchor and tightens the scan's left bound to the session's lifetime neighborhood - aggregate session lookups per trace, so a lookup resolves the session's range plus every member trace's real range - reshape the time-range endpoint to query parameters so one endpoint serves both lookup kinds - make the session-details caller range optional, union it with the indexed range (never narrowed), take trace ids from the index, and keep the legacy source-stream scan for pre-coverage data - add `session_id` to the default bloom filter fields (gated by the existing `ZO_FEATURE_DEFAULT_INDEX_FIELDS_ENABLED`, folds into the secondary-index field set like `trace_id`); streams without the column build nothing ## Why Session detail lookup depends on a caller-provided time range: the sessions list passes its own picker window into the details request, so an LLM session that started before the window silently loses its earlier turns, and a deep link without a range is a hard 400. The trace time index already turns a `trace_id` into a time range at bounded cost; a session is the same problem one level up — `session_id` → range plus member traces. ## Environment variables No new variables. The existing `ZO_TRACE_TIME_INDEX_ENABLED` (default `true`) gates the feature, session column included. The existing coverage marker is reused; there is no historical backfill. This change must ship in the same release as #13827: reusing the coverage marker is valid only while no release separates the two. ## API changes All timestamp parameters and returned ranges use microseconds. ### Time-range endpoint reshaped ```http GET /api/{org_id}/{stream_name}/traces/time_range?trace_id=xx&hint_ts=... GET /api/{org_id}/{stream_name}/traces/time_range?session_id=xx&hint_ts=... ``` Exactly one of `trace_id` / `session_id` is required; zero or both is `400`. This replaces `GET /{stream_name}/traces/{trace_id}/time_range`; the old path form is removed without a compatibility alias since #13827 has not shipped in a release. ### Session details: range becomes optional `GET /api/{org_id}/{stream_name}/traces/session/details` now accepts `start_time` / `end_time` as an optional pair plus an optional `hint_ts` (defaulting to the caller range's midpoint). The effective range is the union of the caller range and the indexed session range, so it never narrows what the caller asked for; `400` only when neither side yields a range. When the caller range lies entirely inside index coverage the source-stream trace-id scan is skipped; otherwise it still runs over the caller range and its results are merged, so behavior is never worse than before. No frontend changes are required — the frontend keeps sending its list window and the union completes the session. ## Validation - `cargo test -p openobserve-core traces::time_index --lib` — 13 passed - `cargo test -p search datafusion::merge --lib` — 5 passed, including NULL-mixed session aggregation and pre-column file compatibility - `cargo test -p openobserve-api-search --lib` — 149 passed - `cargo test -p openobserve-api-http --lib` — 53 passed, including OpenAPI/MCP initialization - `cargo test -p config router:: --lib` — 8 passed - `cargo test -p infra db::trace_time_index --lib` — passed - `cargo clippy` on all touched crates (`--all-targets`) and `cargo check --workspace` — clean - the enterprise repository has no references to any changed API and no enterprise-gated code was modified |
||
 Julián ValdésandHengfei Yang
|
06c6b515fb |
fix(stats): only read file_list_dump_stats when dumping is enabled (#13830)
Fixes #13829. ## Problem `file_list_dump_stats` is created **conditionally** — `handle_partitioned_tables` only adds it when `ZO_COMPACT_FILE_LIST_DUMP_ENABLED` is set, and that flag defaults to `false`. But `update_stats_from_file_list_for_stream` reads it **unconditionally**. On a PostgreSQL metastore where the table was never created: ``` [STATS] run update stream stats from file list error: SqlxError# error returned from database: relation "file_list_dump_stats" does not exist ``` **The failure is wider than the dump.** The `?` propagates before `set_stream_stats`, so the entire stats update is aborted — every stream reports `doc_num: 0` while ingestion, search and compaction keep working. It reads as a UI problem rather than a failing job, which is what makes it expensive to track down. SQLite creates the table unconditionally, so this only surfaces on deployments with an external PostgreSQL metastore. ## Change Gate the read on the same flag that gates the creation. One call site — `query_dump_stats_by_date_range` has no other caller. When dumping is off there is nothing to merge, so the stats are unchanged: `merge` over an empty result was already a no-op. The difference is that the surrounding update now completes instead of aborting. ## Alternative considered Always creating the table would also fix it, at the cost of an empty partitioned table on every deployment. Gating the read seemed closer to the existing intent — the table is deliberately optional — but I am happy to flip it if you prefer the schema to be unconditional. ## Testing Reproduced on v0.92.1 with `ZO_META_STORE=postgres` and the flag at its default: stats fail every cycle and all counters sit at zero. Creating the table by hand with the DDL from `create_partitioned_dump_stats_table` makes the error and the zeros go away, which confirms the missing table is the whole cause. I have not been able to build the project locally to run the test suite — no Rust toolchain on the machine I reproduced this on. The change is a single conditional and the import follows the one already used in `src/compaction/src/merge.rs` (`use config::{get_config, ...}`), but it deserves a CI run before merging. Signed-off-by: Julián Valdés <julianvaldes24@gmail.com> Co-authored-by: Hengfei Yang <hengfei.yang@gmail.com> |
||
|
udev
|
35860aff9d |
fix: route annotation queue score lookups to queriers (#13801)
## Summary - route Discovery and annotation queue endpoints to queriers so score lookups, item hydration, and review reads do not fall through to ingesters - make Discovery source_stream required at the API response boundary - cover Discovery, queue item detail, and review routing with regression tests The QueueItem remains reference-only. Workbench and enqueue validation derive source_stream from timestamp-bounded _llm_scores queries in the enterprise implementation. Depends on https://github.com/openobserve/o2-enterprise/pull/2423 ## Testing - 5 querier routing tests - Discovery response serialization test with the enterprise feature - 16 enterprise annotation queue tests - 14 enterprise Discovery tests |
||
|
udev
|
d770d0245a |
fix: preserve online evaluation provider API keys (#13739)
## Summary - preserve the stored provider API key when an edit submits a blank value - replace the stored key when a nonblank value is supplied - show the create-time required marker only for providers that require authentication - retain keyless creation for self-hosted providers ## Testing - ProviderFormPage Vitest suite passed: 8 tests - Targeted ESLint passed - cargo fmt check passed - git diff check passed - Focused Rust test compilation is blocked by unrelated existing alert-state model errors Related to #13655. Companion enterprise PR: https://github.com/openobserve/o2-enterprise/pull/2390 |
||
 Omkar Kesarkhaneandgithub-actions[bot]
|
ddb67f3efb |
fix(synthetics): record from a specific step, and stop silently downgrading recorded actions (#13799)
Supersedes #13777, which carried the record-from-step commits alone. Closing that in favour of this, because the two changes land in the same three files (`types/synthetics.ts`, `useSyntheticsRecorder.ts`, `JourneySteps.vue`) and reviewing them apart means resolving the same conflict twice. ## Problem **A recorded hover was stored as a click.** Playwright 1.56 added `hover` to the recorder's action model, reachable from its "Choose action" picker. The extension's action mapper seeds every step with `action: 'click'` as a placeholder for its switch to overwrite, and returned that placeholder from `default:` — so any action name the switch did not know became a click the author never performed. On the usual hover target — a nav trigger, a dropdown opener — clicking it can navigate or submit. The run still reports green. There are three separate guards meant to stop exactly this, and none of them fired, because the relabel happens upstream of all of them: | Guard | Why it missed | |---|---| | `UNSUPPORTED_REPLAY_ACTIONS` (extension) | step already said `click` | | `RETIRED_ACTIONS` → `isStorableAction` | step already said `click` | | `V2_STEP_ACTIONS` (Rust validation) | `click` is valid | The same picker offers **Right click** and **Double click** as explicit choices. Both were flattened too: `click_count` was dropped by the mapper and never existed downstream, and the probe's executor called `locator.click({ timeout })` while reading neither `button` nor `click_count`. A deliberately recorded right click replayed as a left click; a double click as a single one. **Recording could only start from scratch.** Fixing step 12 of a 15-step journey meant re-recording all 15. ## Change **Record from a specific step.** Each step gets a record action that replays the prefix before it, then flips the recorder into insert mode at that point. A restore that cannot reach the requested step reports where it stopped (`prefixFailed`) rather than silently recording from the wrong place. **Every action Playwright can record is now stored and replayed as itself.** - `hover` is a first-class v2 action — mapped, stored, validated, and executed by `Locator.hover` in the probe and `Frame.hover` in the extension player. - `button` and `click_count` travel to storage, so right click and double click replay as recorded. - An action name the mapper does not know now **throws** instead of returning the `click` placeholder. This is the part that is not hover-specific: upstream added one action in nine minors and will add more, and the next one must be a deliberate decision rather than a silent downgrade. **The extension handshake reports capabilities, not just presence.** `getStatus` now returns `extVersion` and a `capabilities` array; O2 gates affordances on `hasCapability()` with a defined pre-handshake fallback, and warns when the installed build is older than the minimum. Previously the protocol carried no version at all — Chrome updates the extension independently of when O2 deploys, so the two could disagree about the wire with no way to say so. **Two dead message declarations, opposite problems.** `setSources` no longer exists in playwright-core (sources arrive alongside the actions), so its declared variant could never fire — while its `generatedCode` field is exactly what an eject-to-code feature would reach for. Deleted. `elementPicked` is the reverse: it fires on every "Pick locator" and every click in inspecting mode, and was being discarded, which made inspect mode a dead end. Now kept. ### Compatibility `button` and `click_count` are optional with defined absent-behaviour — absent means a single left click. Every journey stored today stays valid, means the same thing, and serialises byte-identically. **No schema version bump and no migration.** A test pins that rather than leaving it as an intention. `hover` is a new vocabulary entry with no default, so the probe must be able to execute one before O2 is allowed to store one — the probe change ships first. ## Implementation | File | | |---|---| | `src/config/src/meta/synthetics.rs` | `hover` joins `V2_STEP_ACTIONS`/`V2_ELEMENT_ACTIONS`; `button` and `click_count` added to `BrowserStepV2` with validation (`left\|middle\|right`, `1..=3`) | | `web/src/types/synthetics.ts` | `button`/`clickCount` on both step types; `extVersion` + `capabilities` on `RecorderStatus`; `setSources` variant deleted and `elementPicked` typed; `prefixFailed` and insert-mode fields on `recordingStarted` | | `web/src/utils/synthetics/mapRecordedStep.ts` | promotes `button`/`clickCount` from the wire, like every other stored field | | `web/src/utils/synthetics/buildV2Steps.ts` | `hover` in `ACTION_TO_V2`; crosses camelCase → snake_case for the two click fields | | `web/src/constants/synthetics.ts` | `hover` out of `RETIRED_ACTIONS`; `MIN_EXTENSION_VERSION` | | `web/src/composables/useSyntheticsRecorder.ts` | `startRecordingFrom`, `hasCapability`, `pickedSelector`, `extensionOutdated` | | `web/src/components/synthetics/journey/{BrowserJourney,JourneySteps}.vue` | per-step record action and its wiring | `click_count` is capped at 3: the recorder only ever emits 1 or 2, and an unbounded count is a way to hold a page's event loop open. The two click fields cross from camelCase to snake_case in `buildV2Step` and nowhere else — reading `step.wire` directly inside it would have worked and invented a second path into storage. ### Depends on a recorder extension build `hover`, the click fields and the capability handshake all need the matching extension build. O2 degrades honestly without it: `hasCapability()` falls back to the pre-handshake set, and an older build raises the outdated-extension warning instead of failing on a message shape it does not recognise. ## Tests - **Rust** — hover accepted as an element action; `button`/`click_count` validated at both bounds; a step carrying neither still validates (the compatibility guarantee). Two existing tests encoded "hover is rejected" and now say the opposite. 101 pass. - **Probe** — hover hovers rather than clicks; a right click sends the recorded button; a double click sends two clicks; a journey stored before these fields replays unchanged. The action union is asserted against the Rust vocabulary, so the next divergence fails a test instead of reaching an executor with no case for it. Also pinned: a selector carrying a frame hop resolves into the iframe, which had no coverage anywhere. - **Web** — 1213 across 60 files. Nine existing tests encoded "hover is retired" across constants, `buildV2Steps`, `buildPayload` and the save-gate schema; each now states the opposite deliberately. - **Extension** — the unmapped-action throw, hover round-tripping both directions, and a double click surviving the round trip as two clicks. `vue-tsc`, `cargo clippy -D warnings`, and `prettier --check` are clean. The last commit is formatting only: the three record-from-step files arrived failing `format:check`, which is a gate of its own and would not have been caught by lint. --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> |
||
|
Hengfei Yang
|
41ff3cd9b6 |
feat: replace distinct_values_* streams with tantivy TopK for the _values API (#13834)
Design at: #13822 ## Summary `distinct_values_*` metadata streams existed to serve one reader: the `_values` API. They are a redundant — and lossier — materialization of what the tantivy secondary index already holds, and the index has the collectors needed to replace them outright (`SimpleTopN` / `SimpleDistinct`). This PR removes the distinct streams from the write and read paths and lets the `_values` API be served by the tantivy index optimizer. ### 1. Treat distinct value fields as secondary index fields (read-time union) - `get_stream_setting_index_fields()` — the single accessor used by both the index build side (ingester parquet/pack jobs, compactor merge) and the query side (leader/follower index optimizers) — now unions `distinct_value_fields` into the effective index field set, gated on `enable_distinct_fields`. Fields that cannot be secondary-indexed (full text search keys, partition keys, reserved columns) are skipped; the values API falls back to a normal scan for those. - The distinct list is deliberately **not merged into `settings.index_fields`**: the two stay independent, so removing a distinct value field (or disabling `enable_distinct_fields`) cleanly stops its indexing with no residue, and the user-managed `index_fields` list is never mutated behind their back. - Correctness gate: a distinct-only field is considered indexed since its `added_ts` (`get_stream_setting_index_updated_at_for_fields`), so parquet files older than that are served by DataFusion scan instead of the index (`split_file_list_by_time_range`). `added_ts` is set on add and reset when `enable_distinct_fields` is re-enabled (existing behavior). For streams configured before this release the fields only start being indexed at upgrade time, so `added_ts` is additionally clamped by a release-wide cutoff `/tantivy/distinct_fields/updated_at` — a lazy marker initialized to the first access after upgrade, following the existing `ttv_secondary_index_updated_at` pattern. No data migration and no `DB_SCHEMA_VERSION` bump needed. ### 2. `_values` API always queries the original stream - Removed `can_use_distinct_stream()` and the `distinct_values_*` / `SUM(count)` / `StreamType::Metadata` rewrite from `values_inner` (GET `/_values`, formerly `values_v1`) and `build_search_request_per_field` (POST `/_values_stream`). - The generated SQL shapes are exactly what the index optimizer detects: - `SELECT f AS zo_sql_key, COUNT(*) AS zo_sql_num FROM s GROUP BY zo_sql_key ORDER BY zo_sql_num DESC, zo_sql_key ASC` + limit → `SimpleTopN` - `no_count`: `SELECT f AS zo_sql_key FROM s GROUP BY zo_sql_key ORDER BY zo_sql_key ASC` + limit → `SimpleDistinct` - The `keyword` filter (`str_match_ignore_case`) is a valid index condition, so keyword-filtered values queries stay on the index path too. ### 3. Remove distinct values generation - Dropped the per-record distinct value collection in logs and traces ingestion and the whole `core::metadata` module (channel, mem-table, flush job, shutdown hook). - Removed the now-unused `DISTINCT_FIELDS` static and the `ZO_FEATURE_DISTINCT_EXTRA_FIELDS` / `ZO_DISTINCT_VALUES_INTERVAL` / `ZO_DISTINCT_VALUES_HOURLY` config knobs, including the `SUM(count)` merge rollup the last one gated — legacy `distinct_values_*` streams now compact as ordinary metadata streams. The default fields `DISTINCT_FIELDS` carried (`service_name`, `operation_name`) are already default secondary index fields via `SQL_SECONDARY_INDEX_SEARCH_FIELDS`, so traces `_values` queries on them remain index-served. - Renamed `values_v1` to `values_inner`: the `v2` variant was removed long ago (#5087) and the distinct stream switch inside `v1` is gone now too, so the version suffix was pure history. ### 4. Default fields after this change - `service_name`, `operation_name`: default secondary index fields (unchanged, via `SQL_SECONDARY_INDEX_SEARCH_FIELDS`). - `trace_id`: now also a built-in default **bloom filter** field (`BLOOM_FILTER_DEFAULT_FIELDS`, gated on `ZO_FEATURE_DEFAULT_INDEX_FIELDS_ENABLED`), in addition to being a default index field — bloom sits on top of the secondary index and prunes files for high-cardinality equality lookups. - There is no "default distinct fields" concept anymore. ### 5. Fix surfaced by e2e: SimpleDistinct returned null values as `""` The tantivy index encodes null/missing field values as empty strings (`index_builder` adds `""` for null docs), so the `SimpleDistinct` term-dictionary walk returned `""` as a distinct value — sorted first in ascending order, it surfaced as a blank first option in dashboard variable dropdowns, which the DataFusion `GROUP BY` path never returns (its null group sorts last). This was exposed once distinct fields became index-served; `handle_simple_distinct` now skips empty terms (with a unit test). ### 6. Drive-by: fix the failing `api_fuzz_tests` CI job `CreateAlertRequestSchema` was an untagged documentation union with two identical variants, producing `oneOf: [CreateAlertRequestBody, CreateAlertRequestBody]` in the OpenAPI spec. Under `oneOf` semantics (exactly one branch must match) no instance can ever validate, so schemathesis filtered out every generated example and `api_fuzz_tests` failed on `POST /api/v2/{org_id}/alerts` on every CI run since that schema landed. The handler deserializes `CreateAlertRequestBody` directly, so the annotation now documents exactly that type; runtime behavior is unchanged. ## What stays (intentionally) - `distinct_value_fields` / `enable_distinct_fields` in stream settings: still the user-facing way to declare values-API fields; the list now drives the read-time index-field union described above. - The `distinct_value_fields` DB table that tracks dashboard/report usage of these fields (still gates removal in the settings API). - `DISTINCT_STREAM_PREFIX` helpers, retention sync and stream-deletion cleanup for existing `distinct_values_*` streams — legacy stream data keeps compacting (as ordinary metadata streams) and ages out via retention; no new records are written to them. ## Testing - `cargo check --workspace --all-targets` and `cargo clippy --workspace --all-targets` clean. - Enterprise build verified with `Cargo.toml.openobserve` (`cargo check --all-targets`). - New unit tests for the index-field union (skip rules for FTS/partition/reserved fields, `enable_distinct_fields` gating) and the `added_ts` / release-cutoff indexed-since resolution; schema, infra, config and api-search unit tests pass. A few pre-existing failures on a clean tree (config `ssrf_guard`, one flaky sqlite-backed destinations test, one `.env`-dependent index-fields test) are local-environment issues and unrelated. Closes #13822 |
||
 Harsh Mahajan
|
7f74925605 | feat: banner as notification (#13667) | ||
 Prabhat Sharmaandgithub-actions[bot]
|
0dba67d638 |
feat: composite alerts (#13797)
Design at : alerts/composite_alerts/composite_alerts.md ## Summary - add composite alert graph persistence, evaluation, scheduling, lifecycle APIs, and deletion safeguards - add composite alert UI forms, preview, references, list/detail integration, and localization - add Rust, API, Vue, and Playwright contracts ## Verification - cargo fmt --all -- --check - cargo check -p openobserve-core - cargo check -p openobserve-api-management - npm run type-check:composite-alerts - npm run type-check:app - npm run lint:ci -- --quiet - design/token/style checks - targeted Rust/Vitest suites - browser QA against local alerts UI ## Follow-ups (intentionally deferred, documented in the spec) - `RunOutcome::Error`/`NotifyFailed` outcome records on composite delivery failure (§11.5) - Incident detail live-definition resolver for composites + "definition unavailable" retention (§9.6) - `alert_composite_*` observability metrics (§16) - SLO/stream delete-conflict 409 bodies with readable-vs-hidden parent split (§10.3) - Super-cluster startup preflight (§18) <img width="1650" height="979" alt="image" src="https://github.com/user-attachments/assets/3def5246-7bf7-48fe-9a42-c0ac08271ea9" /> --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> |
||
 TessaIO
|
30b938a1ed |
feat: introduce compactor ZO_COMPACT_DELETE_FILES_DELAY_MINUTES (#13828)
Resolves #13819 Signed-off-by: TessaIO <ahmedgrati1999@gmail.com> |
||
|
Hengfei Yang
|
3fda083f51 |
feat(traces): add trace time index (#13827)
Design at: [https://github.com/openobserve/designs/blob/main/infrastructure/file-list/trace-time-index.md](https://github.com/openobserve/designs/blob/main/infrastructure/file-list/trace-time-index.md) ## Summary - add an hourly Metadata time index per trace stream to resolve a trace ID to its full time range - add external time-range and trace-details APIs, including bounded locate/expand behavior and standard search responses - integrate index generation into trace ingestion and preserve query visibility through the synchronous Metadata write path - synchronize retention and deletion for associated Metadata streams - reuse the shared trace stream permission check across trace APIs ## Why Trace detail lookup currently depends on a caller-provided time range, which can omit spans outside that window. The time index provides a bounded, best-effort range lookup for `stream + trace_id` without scanning the full trace stream. Closes #13823 ## Environment variables | Variable | Default | Effect | | --- | --- | --- | | `ZO_TRACE_TIME_INDEX_ENABLED` | `true` | Enables per-trace-stream Metadata indexes, index writes during trace ingestion, and indexed time-range lookup. Only traces ingested after the feature is enabled are indexed; there is no historical backfill. | | `ZO_QUERY_TIMEOUT` | 600 seconds | Existing setting reused as the cumulative Trace Timer locate/expand deadline and as the default timeout for the trace-details search. | When enabled, each traces stream can create a related Metadata stream named `trace_time_index_{stream}`. Its retention settings follow the source stream, and it is removed with the source stream. ## API changes All timestamp parameters and returned time ranges use microseconds. ### Get a trace time range ```http GET /api/{org_id}/{stream_name}/traces/{trace_id}/time_range ``` Query parameters: - `hint_ts` (optional): timestamp hint used to start the index lookup Returns the source stream, trace ID, and the resolved `start_time` / `end_time`. The `range` field is `null` when the index cannot resolve the trace. ### Get trace details ```http GET /api/{org_id}/{stream_name}/traces/{trace_id}/details ``` Query parameters: - `start_time` and `end_time` (optional, but must be supplied together): caller-provided range - `hint_ts` (optional): timestamp hint for the time-index lookup - `timeout` (optional): trace-details search timeout in seconds The endpoint unions the caller range with the indexed range, so it never narrows a caller-provided range. If neither the caller range nor the index can provide a range, it returns `400`. The response uses the standard search response and includes the effective range in `new_start_time` and `new_end_time`. ## Validation - `cargo test -p openobserve-api-search --lib` — 145 passed - `cargo test -p openobserve-core traces::time_index::tests --lib` — 7 passed - `cargo test -p infra db::trace_time_index::tests::test_coverage_start_cache -- --exact --nocapture` — passed - `cargo check -p openobserve-api-search` - `cd web && npm run type-check:app` - end-to-end ingestion and query validation with the data generator ## Validation notes - targeted ESLint could not start because the local web dependencies are missing `@eslint/css` - enterprise-only compilation could not be completed from this OSS checkout because the local enterprise overlay is unavailable |
||
|
|
dbee572722 |
fix: slo bugs (#13784)
closes https://github.com/openobserve/o2-enterprise/issues/2397 --------- Signed-off-by: Yashodhan Joshi <yjdoc2@gmail.com> Co-authored-by: Yashodhan Joshi <yjdoc2@gmail.com> Co-authored-by: sai nikhil kethe <nikhil@openobserve.ai> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> |
||
|
Harsh Mahajan
|
0ed5336bf1 | chore: adding HA mode for o2-ai agent (#13525) | ||
|
Huaijin
|
3901c548a2 |
perf(promql): use Utf8View for metric labels (#13796)
## Summary
- read PromQL metric label columns as Arrow `Utf8View` when the existing
`ZO_UTF8_VIEW_ENABLED` setting is enabled
- preserve `__hash__` and exemplars in their original representation
while retaining schema metadata
- make PromQL equality, negative, and regex matchers type-aware for
`Utf8View`; cast regex operands to `Utf8` to avoid the DataFusion 54
mixed-type rewrite issue
- support `StringViewArray` in series-label loading and keep
compatibility with the process-wide series-label cache
No PromQL-specific environment variable is introduced, and metrics
continue to use the existing global `ZO_BATCH_SIZE`.
## Performance
For the filtered histogram query below, a controlled local A/B using the
same release binary and identical settings except Utf8View measured P50
latency improving from 0.964s to 0.677s, about 29.8%:
```promql
histogram_quantile(0.9, sum by(le, path) (rate(codelab_api_request_duration_seconds_bucket{path="/api/service-1"}[5m])))
```
Test range was one hour with `step=15` and `use_cache=false`.
## Testing
- `cargo fmt --all -- --check`
- `cargo test -p promql --lib utils::tests` (10 passed)
- `cargo test -p promql --lib series_labels::tests` (11 passed)
- `cargo test -p search --lib
datafusion::exec::tests::test_metrics_query_schema` (2 passed)
- `cargo check -p promql-service -p promql`
|
||
|
Hengfei Yang
|
6a2dfbb2b4 |
fix(compact): stop retention from deleting one extra day (#13804)
Fixes #13800 ## Problem With a 7 day retention the stream only kept ~6 days of data. From the issue: retention is 7 days, but the stream data range starts at `2026-08-07T00:00 UTC` while now is `2026-08-13T04:52 UTC` — the whole `2026-08-06` partition is gone. Retention deletes data at day granularity (`delete_by_date` wipes a full `[day, day+1)` partition), but the job generator stepped through the days with: ```rust while start < time_range.end { ... } // time_range.end == lifecycle_end ``` `lifecycle_end = now - retention_days` is a point in the middle of a day, so the day it falls into also matched the condition and got a delete job. Deleting that partition also removes everything between `lifecycle_end` and the end of that day, which is still inside the retention period. Effective retention became `(N-1, N]` days instead of `[N, N+1)`. This is a regression from #8058, which switched retention from one job per range to one job per day. Before that change the range end was formatted as `%Y-%m-%d`, so the boundary day was never included. The day-aligned fast path in `generate_jobs()` already assumes the boundary day is retained — it skips the stream when `doc_time_min >= retention_day_start` — so the two paths disagreed today. ## Fix Only generate delete jobs for days that lie fully before the retention boundary. Extracted the day splitting into `generate_deletion_dates()` so it can be unit tested without a database. Retained window goes back to `[N, N+1)` days: with a 7 day retention the stream now holds 7–8 days of data depending on the time of day, which is the expected behaviour for day-granular deletion. Day-aligned range ends (the ranges produced by extended retention splitting) are unaffected — `start + 1day <= end` and `start < end` agree there. ## Tests Three new unit tests for `generate_deletion_dates`: a mid-day boundary (the regression case), a day-aligned end, and ranges shorter than a day. ``` test retention::tests::test_generate_deletion_dates_keeps_the_day_of_the_retention_boundary ... ok test retention::tests::test_generate_deletion_dates_with_day_aligned_end ... ok test retention::tests::test_generate_deletion_dates_less_than_a_day ... ok ``` `cargo clippy -p compaction --tests` and `cargo fmt --check` are clean. `test_generate_retention_job` / `test_delete_all` fail locally on `main` as well (sqlite data dir missing in my checkout), unrelated to this change. ## Note on the `time_range_start >= time_range_end` guard The removed loop had a `continue` when the two formatted dates compared equal, but it could never fire: both dates come from the same `start` variable, formatted before and after `start += day_micros(1)`, and in UTC adding 24h always advances the calendar date. Simulating the old loop with the numbers from the issue (`lifecycle_end = 2026-08-06T04:52 UTC`) produces jobs for `08-04`, `08-05` **and `08-06`** — the guard never triggers. It was meaningful before #8058, where the two dates came from the two ends of the range (`format(range.start)` / `format(range.end)`) and could land on the same day — and where `format(lifecycle_end)` as the exclusive end is exactly what used to keep the boundary day alive. Second commit removes it as dead code. |
||
|
Hengfei Yang
|
3d0085c43e |
refactor(traces): remove the trace_list_index feature (#13809)
## What Removes the `trace_list_index` metadata stream and everything that fed it. ## Why On every trace ingestion, `write_traces` built one `TraceListItem` per span (`_timestamp`, `stream_name`, `service_name`, `trace_id`) and wrote the batch into a per-org `trace_list_index` metadata stream — creating the stream, its schema, a bloom filter on `trace_id`, and (on enterprise) an OpenFGA ownership tuple on first write. Nothing ever read it back. A repo-wide search for the stream name and every associated symbol turns up only the writer itself: no query path, no API handler, no UI reference, and no reference in the enterprise tree. The feature was pure write amplification on the trace ingestion hot path, plus a stream per org that users had to see and pay retention on. ## Changes - Delete `src/core/src/metadata/trace_list_index.rs` (the `TraceListIndex` writer, its schema/settings bootstrap, and its tests). - Drop `MetadataItem::TraceListIndexer` and `MetadataType::TraceListIndexer`, and their dispatch arms in `metadata::write` / `MetadataManager::close`. - Remove the per-record index build and the trailing write call in `write_traces`. The `service_name` lookup there had no other consumer, so it goes too. - Remove the now-unused `ZO_TRACES_LIST_INDEX_ENABLED` config option. - `MetadataItem` is now single-variant, so the refutable `if let` in `distinct_values::write` becomes a plain destructuring `let` (an `if let` there would warn as irrefutable). ## Notes - **Existing data is untouched.** Already-ingested `trace_list_index` streams stay on disk and remain visible under the Metadata stream type; after this change nothing writes to them. They can be dropped through normal stream deletion. Say the word if you'd rather this ship with a cleanup migration. - `ZO_TRACES_LIST_INDEX_ENABLED` is removed rather than deprecated. Deployments that set it will simply see it ignored — `env_config` does not error on unknown `ZO_*` vars. ## Testing - `cargo check --workspace --all-targets` — clean, no warnings. - `cargo test -p openobserve-core --lib metadata::` — 22 passed. - `cargo test -p openobserve-core --lib traces::` — 378 passed. - `cargo test -p config --lib` — 3148 passed, 2 failed: `utils::ssrf_guard::test_async_validator_rejects_literal_private` and `..._resolves_hostname_to_private`. Both are DNS-dependent and fail identically on `main` at `d1f8941512`, so they are unrelated to this change. - The `enterprise` feature could not be compiled locally (the `o2_enterprise` submodule is a stub here, and `openobserve-cipher` fails to build against it independently of this change). The only enterprise-gated code in scope lived inside the deleted file, and the enterprise tree has no references to the feature. |
||
|
7f647109fc |
fix: preserve projection schema when rewriting match_all (#13808)
## Summary Forward-port of #13798 to `main`. - rebuild `ProjectionExec` expressions against the rewritten child schema before DataFusion validates the projection - carry full-text search fields through intermediate projections and preserve the original `FilterExec` output schema - reject a full-text search field that a projection already exposes as an alias for a different expression, instead of silently searching the wrong data - add a regression for a projection sitting between the filter and the datasource ## Root cause `RewriteMatchPhysical` expanded the `NewEmptyExec` projection with full-text search fields. When an optimizer rule (CSE, for example) had inserted an intermediate `ProjectionExec`, adding the FST field changed the child column positions while the projection still referenced its columns by their old indices, so DataFusion rejected the plan. The old `filter_projection` was also computed as positions inside `parquet_projection`, i.e. in the datasource output space. That is only valid when the filter reads the datasource directly. It is now mapped by name against the actual filter input schema, and the field order of `filter.schema()` is preserved so the operators above the filter keep reading the same columns. Two latent issues are fixed along the way: - a scan with `projection == None` (all columns) was narrowed down to only the FST fields - `schema.index_of(...).unwrap()` in the datasource rewrite panicked instead of returning a plan error ## Differences from #13798 `main` is on DataFusion 54 and builds the filter through `FilterExecBuilder`, so step 5 uses `apply_projection` and keeps `with_fetch`. The regression test also differs. The CTE query used in #13798 no longer produces an intermediate projection on DataFusion 54 — CSE does not extract `btrim(log)` there — so the test would be vacuous. `test_projection_between_filter_and_scan` builds the offending plan shape directly instead, which does not depend on optimizer internals. ## Verification - `cargo test -p search --no-default-features --lib datafusion::optimizer::physical_optimizer::rewrite_match` — 7 passed, 0 failed - `test_projection_between_filter_and_scan` fails on `main` without the fix (`SchemaError: Unable to get field named "__common_expr_1"`) --------- Co-authored-by: Neha P <neha@openobserve.ai> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Hengfei Yang
|
83d562325c |
fix(alerts): type the aggregation HAVING clause from the aggregate, not the column (#13790)
Fixes #13786 ## Why Aggregation alerts that worked before 0.92.0 now fail at query time: ``` SELECT host, COUNT("host") AS alert_agg_value, ... GROUP BY host HAVING "alert_agg_value" < '1.0' Optimizer rule 'simplify_expressions' failed caused by Arrow error: Cast error: Cannot cast string '1.0' to value of Int64 type ``` ## Root cause Two defects in `build_sql`, both introduced by #13547 (first shipped in 0.92.0), compound: 1. `having_filter_value` returns an `f64`, so a threshold of `1` becomes `json!(1.0_f64)`. The workspace enables serde_json's `arbitrary_precision`, so the number is stored as the literal text `"1.0"` — never `"1"`. 2. `build_expr` was handed the data type of the **source** column (`agg.having.column`, i.e. `host`: Utf8) rather than the type of the aggregate. `COUNT("host")` is Int64, but the Utf8 branch quoted the literal as `'1.0'`, which DataFusion then failed to cast. Before #13547 the un-widened `agg.having` value was passed straight through, so the clause read `< '1'` — a string DataFusion casts to Int64 without complaint. Hence the reporter's "it worked before 0.92.0". ### The same change also silently zeroed thresholds on numeric columns `build_expr`'s integer branch called `as_i64().unwrap_or_default()`, and `as_i64()` returns `None` for a float-serialized number (`N::Float(_) => None`; under `arbitrary_precision`, `"100.0".parse::<i64>()` fails too). So an alert on `AVG(response_time) > 100` quietly became: ```sql HAVING "alert_agg_value" > 0 ``` No error, no log line — just a threshold replaced by zero, firing on every evaluation (or never, for `<`). This affects more alerts than the reported cast error, and does so invisibly. ## Fix - **`having_data_type()`** resolves the comparison type from the aggregate's OUTPUT rather than the column: `COUNT` → Int64, `AVG`/`MEDIAN`/`P50`–`P99` → Float64, `MIN`/`MAX`/`SUM` → the column type (they do return their input type, so string min/max keeps its string comparison). - **`build_having_expr()`** extracts the widening out of `build_sql` so it is unit-testable without an `infra::schema` fetch, and normalizes an integral f64 threshold back to an integer `Number`. That keeps the Utf8 min/max path emitting `'1'`, byte-identical to pre-0.92. - **`build_expr`'s integer branch** falls back to `as_f64()` instead of defaulting to 0. This also covers plain WHERE conditions, which had the same silent-zero hazard. A fractional value stays fractional rather than being truncated; a non-numeric value errors instead of defaulting. The multi-alert path was never affected — `severity_order_sql` formats its f64 as a bare numeric literal. The reporter's alert now generates `HAVING "alert_agg_value" < 1`. ## Tests Nine unit tests, including the exact #13786 shape (COUNT over a Utf8 column), the silent-zero regression, the widened warning threshold, and the full `having_data_type` mapping. `cargo clippy` and `cargo fmt` clean. > [!NOTE] > One pre-existing failure in the same suite, `alerts::destinations::tests::test_alert_destination_template_is_optional`, fails identically on unmodified `main` (SSRF guard rejects `127.0.0.1`) and is unrelated to this change. ## Note for the reporter The alert in #13786 will now execute without error, but it still won't fire: `GROUP BY host HAVING COUNT("host") < 1` cannot match, because any group keyed by a non-null `host` has a count of at least 1. Detecting "this host stopped logging" needs a different alert shape, not an aggregation threshold. --- ## Second commit: comment cleanup (no behaviour change) `alerts_2.md` does not exist anywhere in the tree, so every "see §4.4, option B" pointer in this file sent the reader looking for a file they cannot open. Same for the bare requirement tags — `M-6`, `M-9`, `D8`, `T-9` — which name nothing a reader of this repo can resolve. Each reference is replaced with the reasoning it was standing in for, in plain terms: | was | now | | --- | --- | | `alerts_2.md §4.4, option B` | what widening the HAVING clause does, and why the critical threshold alone would lose the warning band | | `the M-6 cap` | `the max-groups cap` | | `only resolve via M-7's timeout` | `resolve later via the reaper timeout` | | `T-9: label the worst SERIES` | just the behaviour being described | Kept as a separate commit from the fix so the fix stays reviewable on its own. > [!NOTE] > 81 references to `alerts_2.md` remain in other files under `src/` (mostly `src/config/src/meta/alerts/`). Happy to clean those up in a follow-up. |
||
|
Loakesh Indiran
|
4bb0908679 |
fix(synthetics): let the deployment ceilings survive a config reload (#13785)
Companion to o2-enterprise PR [#2375](https://github.com/openobserve/o2-enterprise/pull/2375). **Merge this first** — that branch does not compile without `set_limits`. ## The bug `job_lease_secs`, `max_check_budget_secs` and `max_net_timeout_ms` are declared in `o2_enterprise`'s `SyntheticsConfig` and pushed in here, because this crate cannot depend on `o2_enterprise`. The holder was a `OnceLock`, and `OnceLock::set` is a no-op after the first write. Boot wrote it once. A config reload is the second write, so it was silently discarded: an operator could edit `O2_SYNTHETICS_MAX_CHECK_BUDGET_SECS`, hit `/config/reload`, see no error, and still have every check validated against the boot-time ceiling until the process restarted. ## The change The holder is an `ArcSwap` and `set_limits()` overwrites. `ArcSwap` rather than a lock because `limits()` is read on every check-config validation — i.e. on request threads — while a reload writes it, and a reader-preferring lock would let those readers starve the writer. `init_limits` is unchanged for callers and now delegates to `set_limits`. ## One behaviour change worth reviewing **Rejection now keeps the last-good value rather than reverting to `DEFAULT_*`.** A deployment running a deliberately tight 540s/600s pair that reloads with a typo must not have its ceiling widened back to 840s/900s by the failure — that would silently accept checks it was configured to refuse, the opposite of what the operator asked for. At boot there is no last-good value, so the defaults stand and behaviour is unchanged from before. The `init_limits` doc comment is updated to say so. ## Tests 12 new tests in their own integration binary. That placement is deliberate: the holder is process-global and ~90 unit tests in this module validate check configs against `limits()`, so sharing a binary would make them fail intermittently whenever an overwrite test had a different ceiling installed. A separate `tests/*.rs` is a separate process. Two of them assert across `Synthetic::validate` rather than just reading `limits()` back — reading the holder proves the swap happened, not that validation actually moved. Hoisting `limits()` into a `LazyLock` would keep the holder tests green while freezing validation forever. The concurrency test asserts readers observe at least two distinct installed sets, so it cannot pass against a `set_limits` that silently drops writes. `cargo test -p config`: 12 new pass, 98 existing in `meta::synthetics` still pass. `cargo fmt --check` and `cargo clippy -p config --all-targets -- -D warnings` clean. --- *Replaces #13771 — identical content. Renamed the branch to `fix/lease-claimed-by-2270` to match the enterprise branch, because o2-enterprise's `unit-tests.yml` resolves its OSS checkout by looking for a branch of the same name and otherwise falls back to `main` (which lacks `set_limits`, so enterprise CI could not compile).* |
||
|
Hengfei Yang
|
c0d49810b3 |
perf: re-add process-wide promql series label cache (#13748)
## Summary Restores the process-wide PromQL series label cache that was added in #13109 and removed in #13148, ported onto the partitioned label-loading pipeline introduced by #13148. Labels are immutable per series hash, so a warm cache lets repeated queries skip the second (label-column) scan entirely — the biggest wins are on `by()`-grouped aggregations, where the label phase is a fixed cost that the cache removes. ## Changes - **`src/promql/src/label_cache.rs`** (ported from #13109): sharded LRU keyed by `(context fingerprint, series hash)`, bounded by **memory size** — `ZO_METRICS_LABEL_CACHE_MAX_SIZE` in MB, `0` (default) = auto: 5% of total memory clamped to [100 MB, 1 GiB]. - **Reads are always served; admission gates writes only.** A lookup cannot grow the cache, so even a query too big to admit benefits from entries other queries warmed. Admission is sized by the query's actual **misses** (the only entries it could insert) and bypasses **writes** when they exceed 50% of the budget, so one oversized query cannot evict the shared cache. Hit/miss metrics therefore reflect real lookup outcomes. - **`load_series_labels`** attaches cached labels partition-parallel (rayon) before the scan and collects per-partition missing hashes: - when every series hits, the DataFusion label scan is **skipped entirely** - the miss scan is narrowed to missing series: a hash in-list filter for up to 8192 misses, plus a per-missing-series max-timestamp filter (falls back to the sample scan's timestamp set when everything misses) - Partition tasks only attach/cache series in their missing set; extracted label sets are stored back into the cache, sharing the label interner's `Arc<Label>` values (hot label values are deduped inside the cache too). - Cached entries exclude the synthetic `__hash__` label; it is prepended per query, so raw-data and regular queries share entries. - Restores the `query_metrics_label_cache_hit_count` / `query_metrics_label_cache_miss_count` metrics (per organization). ## Performance Re-validated on the current partitioned pipeline: warm queries are **~20% faster than main** on the high-cardinality benchmark dataset below. The detailed tables are from #13109 (same cache design and dataset, measured on the pre-#13148 pipeline): single node (M-series Mac, 32GB), 1.34B rows / 3.17M series of a histogram bucket stream, result cache disabled, warm = median of 3 runs after first. **What the cache itself contributes** (cache on vs off on the same binary, `ZO_METRICS_LABEL_CACHE_MAX_SIZE=1` as the off-switch): | Query | cache on | cache off | cache contribution | |---|---|---|---| | `sum by (region) (rate(m[5m]))` | **3.62s** | 5.40s | **1.49x** | | `histogram_quantile(0.9, sum by (le) (rate(m[5m])))` | **3.53s** | 4.93s | **1.40x** | | `count by (path) (rate(m[5m]))` | **3.73s** | 5.56s | **1.49x** | | no-modifier aggregations (skip path), `topk` (write-bypassed), small filters | — | — | none (by design) | The cache is precisely a **`by()`-grouped-query optimization**: 1.4–1.5x there, zero effect (and zero overhead) everywhere else. Cold runs with the cache are on par with cache-off (`sum by (region)` cold: 5.33s cached vs 5.30s uncached). **Admission control** (`topk(5, rate(m[5m]))` needs full labels for all 3.17M series ≈ 3GB working set, past the 1 GiB auto budget): | Configuration | time | |---|---| | cache disabled (baseline behavior) | 10.4s | | cache without admission, 1 GiB budget (thrash) | 12.0s ⚠️ | | **cache with admission, 1 GiB budget (bypasses)** | **10.6s — parity restored** | | cache with `ZO_METRICS_LABEL_CACHE_MAX_SIZE=8192` (fits, admitted) | **4.5s — 2.3x faster** | The thrash row is what admission prevents — and it was caused by insert + eviction churn, which is why the current revision gates only writes and keeps reads always on. Notes on reproducing: this dataset is deliberately extreme, so its `by()` working sets (~0.6–0.9GB) exceed the default budget's 50% single-query share — set `ZO_METRICS_LABEL_CACHE_MAX_SIZE=2048` or higher to admit them. At typical production cardinality (≤1M series), `by()` working sets are admitted at defaults. Within a single query, selectors share the cache (the second operand of a binary op loads labels in 315ms vs the 2.57s the first paid). ## Operational notes Re-introduces `ZO_METRICS_LABEL_CACHE_MAX_SIZE` and the two cache metrics removed in #13148. Sizing guide: a typical series (~10 labels) costs ~1KB, so the 1 GiB auto cap holds roughly 1M series; column-pruned entries from `by()` queries are far smaller (~100B), so grouped queries fit ~10M series. A tiny budget (e.g. `1`) acts as a de-facto off switch for writes. ## Testing - 4 label-cache unit tests (get/put, byte-bounded eviction, admission control, fingerprint) ported from #13109 - New tests: partial-miss scan attaches only missing series and populates the cache; cached entries exclude `__hash__`; cached-attach hit/miss partitioning - All 374 `promql` crate tests pass; full workspace `cargo check` and clippy clean - Output verified numerically identical (#13109 methodology, max relative diff 1e-13 from float summation order) |
||
|
Huaijin
|
fd2306ffaa |
fix(search): preserve indexed top-n aggregation results (#13779)
- close: https://github.com/openobserve/openobserve/issues/13363 ## Summary - Prevent `AggregateTopkExec` from truncating file-local indexed aggregation rows before the final aggregate merges duplicate group keys. - Keep the Tantivy `SimpleTopN` optimization enabled while removing only the conflicting downstream top-k node. - Add a regression test that constructs the conflicting physical plan, verifies `AggregateTopkExec` is present before leader index optimization, and verifies it is removed afterward while `SimpleTopN` remains selected. ## Root cause `SimpleTopN` emits partial count rows per `(file, group key)`. `AggregateTopkExec` previously ranked and truncated those individual rows before the final aggregate could combine identical keys across files. With enough files, this silently undercounted results and removed entire groups while `is_partial` remained false. The rewrite removes the conflicting node bottom-up. This is important because `AggregateTopkExec` is inside the follower plan already serialized by `RemoteScanExec`; rebuilding the parent updates those serialized plan bytes before attaching `SimpleTopN`. ## Verification Using 121 Parquet files and 121 Tantivy index files containing 93,601 records across 12 groups: | Result | Before | After | |---|---:|---:| | Groups returned | 9 | 12 | | Sum of grouped counts | 83,201 | 93,601 | | Baseline `COUNT(*)` | 93,601 | 93,601 | | `is_partial` | false | false | Tests run: - `cargo fmt --all -- --check` - `cargo test -p search test_leader_rule_topn_removes_conflicting_aggregate_topk -- --nocapture` - `cargo test -p search datafusion::optimizer::physical_optimizer::index_optimizer::tests -- --nocapture` - `cargo test -p search aggregate_topk -- --nocapture` - `cargo check -p search` |
||
|
Prabhat Sharmaandgithub-actions[bot]
|
360b989272 |
feat(slos): PromQL support for count and time-sliced SLOs, plus SLO/alert regression fixes (#13761)
Supersedes #13738, whose commit is included here as the first of four. Adds PromQL support to count and time-sliced SLOs, lets metrics streams choose SQL or PromQL, and fixes three shipped bugs found along the way. ## Shipped bug: every SQL time-slice SLO recorded wrong history `fetch_rows`' `SliQueryPlan::Single` arm normalized every hit with `with_good_column: true`, but that arm serves **two** shapes and only one projects `zo_slo_good`: | shape | projects | result | |---|---|---| | count single-query | `zo_slo_good` + `zo_slo_value` | correct | | time slice | `zo_slo_value` only | `good` defaulted to `0.0` | So every SQL time-slice slice was classified from `0.0` instead of its aggregate. With a positive threshold, `<`/`<=` (e.g. `p95 < 300ms`) recorded every slice **good** — the SLO read ~100%, the budget never burned, and no alert on it could fire. `>`/`>=` recorded every slice **bad** and fired continuously. Shipped in v0.92.0 (`4772df1723`); `to_row` had no time-slice test, which is how it got through. Fixed by deriving the flag from the SLI type in a new `single_query_rows`. Deliberately **not** fixed inside `to_row` by falling back to `total` — that would read a count bucket whose `SUM` came back NULL as *fully good*, inventing uptime. A test forbids it. **Stored history is not rewritten.** It ages out within one window; an operator can force a rebuild sooner by making a real edit (a no-op re-save does nothing — `update` bumps only on `definition_changed`). No auto-migration: bumping every time-slice SLO at boot would freeze every attached alert org-wide until backfill cleared the coverage floor, and double each org's row budget for 97 days — a self-inflicted monitoring gap to repair a reporting error. ## PromQL support **Time slice** carried a `query_language` field the planner ignored, so a PromQL time-slice SLO saved cleanly and measured nothing, forever. - `SliQueryPlan::PromQlValue` — one expression, not a good/total pair, planned at slice ends from the same helper the count arm uses. - The expression is passed through **verbatim**. Grouping comes from series labels; wrapping it in `sum by (...)` the way the SQL arm injects GROUP BY would destroy the aggregate (the sum of four pods' p95 is not a p95). - `promql_value_rows` **rejects** two series colliding on one `(slice_start, group_key)` via `RejectReason::AmbiguousSeries` instead of summing. Summing is right for counters (what `promql_rows` does) and meaningless here. Detection is unconditional, so the ungrouped case — an unaggregated `histogram_quantile` returning one series per instance — is covered. Rejects are plumbed through `fetch_rows` into `PassResult.rejected` on both the live and backfill paths. **Count** already had a working `CountSource::PromQl` backend that no UI could reach. ## Metrics streams choose SQL or PromQL `language_suits_stream` refused SQL whenever `stream_type == "metrics"`, and the form derived the language from the stream type — so metrics had no SQL option. A metrics stream is an ordinary stream with a `value` column, and `SELECT histogram(_timestamp, '5 minute'), avg(value) FROM cpu_usage` is a perfectly good SLI. SQL is now valid for any stream type; PromQL stays metrics-only. The form asks via a toggle (defaulting to PromQL) instead of deriving. ## Validation - `scope` refused for PromQL — a SQL `WHERE` fragment with nowhere to go in a PromQL plan. - Empty `query` refused for **both** languages (the SQL aggregate was previously never validated — same silent-freeze shape). - Expressions parsed with `promql-parser` at save, in `core` (`config` has no parser). - **Parse-only by design.** A grouping check requiring the root to be an aggregation whose `by(...)` matches `group_by` wrongly rejects `histogram_quantile(0.95, sum by (le, region)(...))` (function-call root; `le` is a necessary label superset) and binary-op roots like `sum by (region)(increase(a[5m])) - sum by (region)(increase(b[5m]))`. A "warn instead" variant needs a warnings channel the save API doesn't have. ## Also fixed - **Time-slice comparator 422.** `SliConfig::TimeSlice.comparator` has no serde default, so a dropdown the user never opened was omitted and the save died with a bare 422 — blocking creation of *any* time-slice SLO from the UI. Seeded to `<` only when blank, so a stored comparator survives an edit. - **SLO/alert/service-stream regressions** (the commit from #13738): fractional-threshold serde round-trip, `SloAlertCondition` UI, alerts table, service streams. ## Verification - 487 config · 206 core · 45 infra · 418 frontend — all passing; clippy `-D warnings`, `cargo fmt`, prettier, eslint, `vue-tsc` all clean. - Driven through a real browser against a running server (isolated instance, own data dir). All four SLI/language combinations create and store correctly: count·PromQL, count·**SQL over metrics**, time-slice·PromQL, time-slice·**SQL over metrics**. Also verified: the toggle swaps field shapes both ways, scope shows for SQL and hides for PromQL, PromQL-only hints appear only in PromQL mode, and editing a stored SQL-over-metrics SLO reopens as SQL with its predicate intact. - Validation rejections confirmed live with actionable messages: PromQL-with-scope, unparseable PromQL, and an empty SQL aggregate. Known limitation, pre-existing: `threshold` likewise has no serde default and is not seeded, so leaving it blank still 422s. That is genuine user error rather than an untouched-control trap; adding form validation was out of scope. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> |
||
|
Hengfei Yang
|
9f76ff6388 | refactor(search): enhance filtering logic for FTS fields in search function (#13767) | ||
 Ashish Kolhe
|
de0d37e23d |
fix(alerts): stop losing Slack alerts on undeliverable link URLs (#13742) (#13753)
Fixes #13742 Fixes openobserve/o2-enterprise#2394 ## The bug A content template link with a non-http scheme (`javascript:alert(1)`) made Slack reject the **entire** notification with `400 invalid_attachments`. The alert was lost outright and `triggers` recorded `notify_failed`. ## Root cause `safe_url` neutralises a hostile scheme by substituting `BLOCKED_URL` (`"#"`). That is **safe** but not **deliverable** — Slack, Teams, Discord and PagerDuty validate action URLs server-side and reject the whole payload when one is not an absolute `http(s)` URL. A security substitution was turning into total message loss. The two properties are different, and links need both. So the fix splits by medium: | renderer | link handling | |---|---| | Slack buttons, Teams actions, Discord `embed.url`, PagerDuty `href` | `dispatchable_url` → **drop** | | HTML email, email plaintext, SNS plaintext, Discord description | `safe_url` → substitute `#` | | Webhook envelope | unfiltered — versioned machine contract, documented | Dropping beats rewriting: the alert body is worth more than the button, and a button pointing somewhere misleading is worse than no button. `dispatchable_url` requires a **non-empty authority**, not just an allowlisted scheme — `http:javascript:alert(1)` and `https://` are valid *opaque* RFC 3986 http(s) URLs that Slack still rejects. It also rejects raw spaces and control characters, which variable substitution can introduce. ## Related issues fixed in the same area - **Empty `ZO_WEB_URL`** (not in the report): `alert_url` becomes a root-relative `/web/logs?…`, which `safe_url` passes through by design, and it lands in the always-appended "View in OpenObserve" button. Same `invalid_attachments`, on deployments that never authored a hostile template. - **PagerDuty** validates `links[].href` server-side too — same lost-alert class, converted. - **Save-time rejection** (`ContentSpec::validate`): a clear 400 naming the offending link, instead of a template whose link silently never works. Also wired into **preview** and **test-send**, which previously accepted a draft that couldn't be saved and echoed the raw hostile URL back to the UI. - **Slack image-fallback misdiagnosis**: removing the image proves the image was at fault, but only an image pointing at *our* `web_url` justifies tripping the process-wide chart suppression (1h, across every alert) and telling the operator to fix `ZO_WEB_URL`. A custom template's third-party image no longer does either. - **a11y**: `OInput` links its error message to the control via `aria-describedby`; the textarea branch also gained `aria-invalid`. ## o2-enterprise#2394 (fixed here, not in the enterprise repo) Clearing the Monaco body and saving toasted "Please fill required fields" with no indication of *where*. The schema already produced an error on the `body` path — a bare Monaco bridged in via `setFieldValue` just had no `OFormInput` error slot to render it. Now a red border + message driven by the same field state `OFormInput` reads. The aria wiring goes on the **shell**, not on `<QueryEditor>`: `CodeQueryEditor` sets `inheritAttrs: false` and binds `$attrs` to a non-focusable wrapper, so attributes passed to the component would be inert — false assurance for a screen-reader user. > The issue names the title field too. It's an `OFormInput` and already highlights correctly; I added a regression test pinning that rather than changing it. This fix is entirely in OSS (`web/`) since the template form isn't enterprise-gated, so there's no companion enterprise PR. ## Layering note Save-time validation is a **UX gate, not the security boundary**. Link URLs are templates — a scheme that only materialises after `{variable}` substitution can't be judged at save time. The render layer is what actually stops delivery, and it also covers every template saved before this validation existed. Both layers are needed; neither is sufficient. ## Testing - Every fix written test-first (verified RED) and **mutation-tested** — each new test fails when its fix is reverted in substance. - Golden files unchanged: legitimate URLs render byte-identically, so no over-blocking. - Rust: `alerts::notifications` 159 · `config` 3130 · `api-management` 424 — all passing. `cargo clippy --all-targets` and `cargo fmt --check` clean. - Frontend: full suite **41,597 passing** (run because `OInput` is shared library code). `vue-tsc`, `eslint`, and `npm run format:check` clean. - Two independent cold-start review passes; findings from both are folded in, including a P0 where the first version of `dispatchable_url` still let the original bug through. Pre-existing and unrelated: `alerts::destinations::tests::test_alert_destination_template_is_optional` fails locally on a DB connection reset — verified failing on unmodified `main` too. |
||
|
HuaijinandHengfei Yang
|
4a837ee4b9 |
chore(config): remove unused environment variables (#13690)
## Summary - remove 10 EnvConfig entries that have no runtime consumers - stop advertising ineffective knobs across SLO, common, runtime, query, metadata, NATS, S3, and pipeline configuration - preserve active replacements such as ZO_FAKE_ES_VERSION and ZO_PIPELINE_BATCH_SIZE_BYTES These variables were parsed but never read, so this does not change runtime behavior. ## Testing - cargo fmt --all - cargo check -p config - cargo test -p config config::tests --lib (64 passed) - cargo clippy -p config --lib -- -D warnings --------- Co-authored-by: Hengfei Yang <hengfei.yang@gmail.com> |
||
|
Huaijin
|
d5fa24711c |
perf(metrics): choose label timestamp filter automatically (#13756)
## Summary - replace the `ZO_METRICS_INLIST_FILTER_ENABLED` switch with automatic timestamp filter selection - use an exact `IN` filter for up to 1024 timestamps and a bounded `BETWEEN` predicate above that limit - sort exact timestamp literals for stable query plans - add boundary coverage for empty, threshold, and over-threshold timestamp sets ## Rationale Large timestamp `IN` lists add planning and evaluation overhead, while always using `BETWEEN` can admit many unrelated label rows for sparse timestamp sets. A bounded exact-list strategy keeps the selective path for manageable sets without allowing query plans to grow indefinitely. Release benchmarks used 4.2M rows, five label columns, four hash partitions, and repeated median measurements: - with a representative sparse range returning about 262K rows through `BETWEEN`, 1024 timestamps favored `IN` (9.1 ms vs 10.1 ms), while 2048 favored `BETWEEN` (11.7 ms vs 10.2 ms) - when unique maximum timestamps equaled the series count and both predicates returned exactly one row per series, `BETWEEN` consistently won and the gap grew with list size, confirming the need for an upper bound ## Testing - `cargo test -p promql series_labels::tests` - `cargo clippy -p promql --tests --no-deps -- -D warnings` - `git diff origin/main...HEAD --check` ## Operational note `ZO_METRICS_INLIST_FILTER_ENABLED` is removed and no longer needs to be configured. |
||
|
Hengfei Yang
|
238b0d5262 |
fix: support zstd-compressed HTTP requests (#13728)
## Summary - enable tower-http zstd request decompression - add a router middleware regression test using a real zstd-compressed JSON payload - update decompression documentation to list zstd support ## Root cause The Axum router uses `RequestDecompressionLayer`, but the workspace did not enable tower-http's `decompression-zstd` feature. Requests from clients such as Vector configured with `compression: "zstd"` were therefore rejected with `415 Unsupported Media Type` before reaching the ingestion handler. ## User impact Vector HTTP sinks can continue using zstd compression when sending batches to OpenObserve ingestion endpoints. ## Validation - `cargo fmt --all -- --check` - `cargo test -p openobserve-api-http handler::http::router::decompression::tests` - `git diff --check` Fixes #13726 |
||
|
udevandsai nikhil kethe
|
bbd8601c64 |
feat(llm): add annotation queues and datasets (#13719)
## Summary - add annotation queue and dataset storage, management APIs, and review workflows - add discovery, dataset, queue, and review user interfaces - add RBAC synchronization, super-cluster support, tags, version history, and review progress ## Validation - CI should pair this branch with the identically named Enterprise branch for cumulative compilation testing ## Migration note This is an additional cumulative PR created from the committed ud/ebb3 tip. The existing SPR pull requests and branches are intentionally left unchanged. --------- Co-authored-by: sai nikhil kethe <nikhil@openobserve.ai> |
||
|
Hengfei Yang
|
ca75da8b78 |
fix(db): scope postgres catalog lookups to the active schema (#13723)
Fixes #11342. ## Problem Upgrading fails on any deployment using a non-`public` postgres schema: ``` [POSTGRES] Fresh install: creating partitioned table file_list db init failed: SqlxError# relation "file_list" already exists ``` The DDL and DML we emit are **unqualified** and resolve through `search_path`, but the catalog / `information_schema` lookups that decide *what* to emit were pinned to a literal `public`. The two disagree: `get_table_relkind` reports "table missing" for a table that plainly exists, we take the fresh-install branch, and `CREATE TABLE` collides with the table already sitting in the user's schema. **The panic is not the worst case.** When a stale same-named table exists in `public` (old deployment, another tenant), the probe returns `'r'` and we take the *regular-to-partitioned migration* branch against the wrong schema's table — renaming its indexes. That matches the index-name conflicts the reporter described. The reporter found 2 sites; there were 14 in total. ## Approach Replace the literal `'public'` with `current_schema()` inline in each catalog query. It evaluates on the very connection that runs the query — the first existing entry of that connection's `search_path`, i.e. exactly where its unqualified DDL/DML lands — so a probe can never disagree with the writes again. No Rust-side state. Two details: - `pg_class` lookups join `pg_namespace` on `nspname = current_schema()` instead of casting to `regnamespace`: the cast parses its input as an identifier and fails on schema names with special characters (e.g. `My-Schema.X`). - `verify_schema_consistency()` at startup checks the read-write, read-only and DDL pools resolve to the same schema. They come from three separately configured DSNs, and `ZO_META_DDL_DSN` is documented as a different **role**, which can carry its own `search_path` via `ALTER ROLE ... SET search_path` — e.g. `cache_indices` reads on `CLIENT` while `create_index` writes on `CLIENT_DDL`, so a divergence would silently relocate the same bug class. Fail loudly instead. ## Sites fixed | File | Functions | |---|---| | `file_list/postgres.rs` | `get_table_relkind`, `check_partition_exists`, `drop_empty_partitions`, `get_varchar_column_width`, `apply_autovacuum_tuning`, `align_identity_sequence` | | `db/postgres.rs` | `cache_indices` (had **no** schema filter at all), `add_column`, `drop_column`, meta `start_dt` probe | | `migration/adapter/postgres.rs` | `list_tables`, `get_columns`, `get_foreign_keys`, `sync_sequences` | | 2 sea-orm migrations | postgres branch had no schema filter, while the mysql branch correctly used `DATABASE()` | The migration-adapter ones matter independently: under a non-public schema `list_tables()` returned **zero** rows, so the cross-database migration tool reported success having migrated nothing. ## Verification Against a real PostgreSQL 18.4 instance with the app configured with `search_path=o2`: - **Fresh install** into schema `o2`: boots clean, all 121 tables land in `o2`. - **Forced upgrade path** (`db_schema_version` lowered) **with a stale regular `public.file_list` planted**: logs `Table file_list already partitioned, ensuring partitions`, upgrade completes, and the stale `public` table is untouched. On main this exact setup either panics (`relation "file_list" already exists`) or migrates the wrong schema's table. - Every rewritten query shape additionally verified in psql under schema `o2test` and under a schema named `My-Schema.X` (stale-table isolation, partition listing, varchar width, identity-sequence alignment, index cache, adapter `list_tables`). `cargo check --bin openobserve`, `cargo fmt --check`, `cargo clippy -p infra --all-targets` all clean; 108 infra postgres unit tests pass. ## Not covered here 1. **Recovery for already-broken databases.** This prevents the failure but does not unstick installs that are mid-migration with renamed indexes. Worth a follow-up (idempotent cleanup, or a documented recovery procedure). 2. **CI coverage.** There is no job exercising a non-`public` `search_path`, which is why these 14 sites accumulated across different changes. A postgres job with `search_path=o2test` would stop this recurring. |
||
|
Ashish Kolhe
|
b3e6a77270 |
fix(correlation): service-streams P2 behavioral fixes + discovered services flat table (#13710)
Correlation P2 round: behavioral/security fixes across the service-streams backend and correlation FE, plus the Discovered Services UX rework and the correlation e2e suites. **Companion enterprise PR (must merge together):** openobserve/o2-enterprise#2381 — the enterprise side calls the new 3-arg `put(org, record, max_streams_per_type)` introduced here. ## Commit 1 — behavioral fixes (F2–F38) **Backend** - F25/F26: RBAC on `list_services` (`service_streams` GET — the one previously-unenforced endpoint in the module) + validate `distinguish_by` ids on identity-config save - F2: validate trace filter param before WHERE-clause interpolation - F6: `_reset` invalidates caches on all nodes via reload event; org deletion clears service-streams caches - F8: shared coverage-based identity-set ranking for ingest and read - F10: purge rows of removed/changed identity sets on config save - F19: `put` returns deleted orphan keys for cache eviction - F28: discriminate no-match from failure; stop aliasing 404 to no-match - F32: cap DB-side stream union at `max_streams_per_type` - `all_dimensions` unions on put so late-tracked dimensions surface - F14: `StreamInfo.dropped_dimensions` contract field; allow empty `tracked_alias_ids` when identity sets exist **Frontend** - F35/F36/F31: per-stream filter-edit resolution via semantic groups; dimension edits in both key spaces; subject overrides never guess fields - F27/F30: echo `source_stream`/`source_type`; stop guessing another stream's filters; chip dedup - F2/F38: SQL field/value escaping via shared `quoteSqlLiteral` helper ## Commit 2 — Discovered Services UX + e2e - Flat table (one row per instance), same-name services merge their name cell (OTable `pivotRowColumns`); optional per-service collapse with aggregated summary row; search/filters override collapse - Fixes an OTable pivot-merge bug (value-keyed merge map blanked single-column runs) + regression test - Semantic Field Groups: category tabs with counts + cross-category search with match highlighting - Correlation Phase-1 API + Phase-2 UI Playwright suites ## ⚠️ Release note (enterprise + RBAC deployments) Viewing Discovered Services now requires the **Service Streams** permission (previously unenforced). Root/admin users are unaffected; custom roles may need a one-time grant via the role editor (resource already exists in the OFGA model). ## Checks - `cargo fmt --all --check` ✅ · `cargo clippy --all-targets` ✅ enterprise + OSS feature sets (0 warnings) - prettier ✅ · eslint 0 errors · 207 unit tests on touched specs ✅ - Compiles with paired enterprise branch ✅ |
||
|
Prabhat Sharma
|
087e1498ab |
feat(slos): add SLO burn-rate alerts (#13704)
design at /slo ## What Adds **SLO burn-rate alerts** — an alert family that fires off an SLO's error budget rather than a raw query — and finishes the SLO surface around it: navigation entry, detail-page fixes, and the flag removal now that SLOs have shipped. ## Changes **SLO burn-rate alerts (backend)** - New burn-rate evaluation (`config/src/meta/slo/burn.rs`, `condition.rs`) with multi-window burn conditions, and `core/src/slo/evaluate.rs` extended to evaluate them on the SLO job. - Alerts carry an `slo_id`; `infra/src/table/alerts` gains the column, filtering, and query support so an SLO's alerts can be listed and deleted with it. - Alert levels (`meta/alerts/level.rs`) and tags extended for the SLO family; scheduler handlers route SLO alerts through the same notification path. **SLO alert UI** - `SloAlertForm` + `AlertDetailSlo`: create, edit and view burn-rate alerts from the SLO detail page and the alert list. - Payload/routing helpers (`utils/alerts/sloAlertPayload.ts`, `sloAlertRouting.ts`, `alertPayload.ts`) with unit tests. - `AlertDestinationsField` extracted as the complete Destination form field — side label, tooltip, combined destinations/workflows picker, inline error, refresh, and create-in-new-tab. It replaces two verbatim copies in the generic alert form and is reused by the SLO form and synthetics `CheckAlerts` (which sets `supportsWorkflows: false`, since a check has no workflow routing). Fixes the SLO form's previously unwired refresh and Add Destination buttons. **SLO detail page fixes** - Burndown and burn-rate charts drew as bare axes when coverage was sparse: `showSymbol: false` means an isolated measured bucket between gaps has no segment and paints nothing. Symbols are now sized per point, and a sparse-coverage note states how many buckets actually recorded events. - Nothing scrolled below ~700px of viewport height — `OPageLayout`'s fixed flex body plus `OTabPanels` `scroll="none"` clipped the panels. Panels now grow and scroll; the stat strip and tab bar stay pinned. Verified at 1024x500, 1280x620, 1512x945. **Other** - `ZO_SLO_ENABLED` removed — SLOs have shipped, so the flag and its status/route gating are gone. - SLOs surfaced in the main nav. - Dashboard chart panel header gets its own tint via new `--color-panel-bar-bg` / `--color-panel-bar-border` tokens, and the bar is extracted into `PanelBar.vue` (it existed as five hand-copied class strings that had already drifted). - `tsconfig.vitest.tsbuildinfo` untracked; third-party vendor name removed from comments and test names. ## Testing - New Rust tests across `meta/slo/burn.rs`, `condition.rs`, `core/src/alerts/alert.rs` and `infra/src/table/alerts`. - New frontend suites: `sloAlertPayload.spec.ts`, `sloAlertRouting.spec.ts`, `alertPayload.spec.ts`, `AlertDetailSlo.spec.ts`, `SloDetailAlerts.spec.ts`, `SloListDelete.spec.ts`, `SloTimeSlicePreview.spec.ts`. - `cargo clippy --all-targets` and `web` `lint:ci` clean. |