add trace and ingestion endpoints - #1744
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a tenant-aware OpenTelemetry generator with lifecycle controls and OTLP export. Adds trace list and detail APIs with local or remote queries. Updates HTTP routes, demo-data execution, CORS origin handling, and alert SQL identifier quoting. ChangesOpenTelemetry observability features
Alert SQL identifier quoting
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant GeneratorHandlers
participant Ingestor
participant OtelGenerator
Client->>GeneratorHandlers: start, stop, or status request
GeneratorHandlers->>Ingestor: forward request in Query or Prism mode
GeneratorHandlers->>OtelGenerator: execute request in Ingest or All mode
Ingestor-->>GeneratorHandlers: return generator response
OtelGenerator-->>GeneratorHandlers: return generator response
sequenceDiagram
participant Client
participant TraceHandlers
participant QueryTarget
participant TraceStore
Client->>TraceHandlers: list or detail request
TraceHandlers->>QueryTarget: build trace SQL
QueryTarget->>TraceStore: execute local or remote query
TraceStore-->>QueryTarget: return trace records
QueryTarget-->>TraceHandlers: reconstruct span hierarchy
TraceHandlers-->>Client: return trace response
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (6)
src/handlers/http/otel_generator.rs (1)
211-219: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGate the forwarded authorization on the HTTP method, not on payload presence.
The condition uses
body.is_some(), which today means "POST start". The ingestor needs the forwarded credential only for start, so the behavior is correct now. The coupling is implicit. If a future route sends a body for stop or status, or start becomes body-less, the condition silently changes meaning. ComparemethodagainstMethod::POSTinstead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/handlers/http/otel_generator.rs` around lines 211 - 219, Update the authorization-forwarding condition in the request-building flow to check whether the HTTP method is Method::POST instead of checking body.is_some(). Preserve the existing multi-tenant and authorization-header checks and forwarding behavior.src/otel_generator.rs (3)
84-94: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove or serialize the
runningfield.
runningis marked#[serde(skip_serializing)], so API clients never receive it. No other code in this cohort reads it. Thestatefield already carries the same information. Either drop the field or serialize it.♻️ Proposed change
pub struct OtelGeneratorStatus { pub state: String, - #[serde(skip_serializing)] - pub running: bool, #[serde(skip_serializing_if = "Option::is_none")] pub endpoint: Option<String>,Update the two construction sites in
statusaccordingly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/otel_generator.rs` around lines 84 - 94, Remove the unused running field from OtelGeneratorStatus, since state already conveys the status and the field is currently excluded from serialization. Update both status construction sites to stop initializing running while preserving the remaining fields and serialization behavior.
224-275: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
stopandstatusnever return an error.Both methods return
Result<_, OtelGeneratorError>, but neither constructs an error value. Onlystartcan fail. TheErrarms instop_otel_generatorandget_otel_generator_statusinsrc/handlers/http/otel_generator.rsare therefore unreachable. Consider returning the values directly and removing the dead error handling in the handlers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/otel_generator.rs` around lines 224 - 275, The stop and status methods never produce errors, so change OtelGenerator::stop and OtelGenerator::status to return OtelGeneratorResult and OtelGeneratorStatus directly. Update stop_otel_generator and get_otel_generator_status to remove unreachable Err handling and return the direct values while preserving existing responses.
783-806: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe lifecycle test performs real network I/O.
startspawnsrun_generator, and the firstinterval.tick()resolves immediately. The task then attempts a real HTTP POST tohttp://127.0.0.1:1. On loopback this fails fast, so the test should stay bounded, but it depends on the sandbox network stack and it logs export warnings. Consider adding a way to run the lifecycle without export, for example a zero-tick or injectable sender, so the test is deterministic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/otel_generator.rs` around lines 783 - 806, Make lifecycle_is_per_tenant deterministic by adding and using a test-supported way to run start without performing exporter HTTP requests, such as a zero-tick mode or injectable sender. Update the relevant OtelGenerator start/run_generator path while preserving normal export behavior, then configure the test to use the non-networking mode and retain its tenant lifecycle assertions.src/handlers/http/modal/server.rs (1)
254-297: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReduce duplication between the two generator scopes.
get_otel_generator_webscopeandget_otel_generator_ingest_webscopedefine the same path, methods, handlers, and authorization. Only.wrap(IntraClusterRequest)differs. Extract a helper that takes a flag, or build the resource once, so a future route change cannot diverge between the two copies.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/handlers/http/modal/server.rs` around lines 254 - 297, Reduce duplication between get_otel_generator_webscope and get_otel_generator_ingest_webscope by extracting shared route construction into a helper that conditionally applies IntraClusterRequest, or by reusing one resource definition. Preserve the existing handlers, methods, authorization, path, and ensure only the ingest scope applies the wrapper.src/handlers/http/traces.rs (1)
710-731: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFail fast when multi-tenant Prism lacks
P_CLUSTER_SECRET.When multi-tenancy is enabled and
CLUSTER_SECRETis unset, return a configuration error beforesend_query_request. Its fallback sends only the querier token and omits the tenant and cluster-secret headers.Handle failed
HeaderValue::from_strconversions instead of silently creating partial authentication headers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/handlers/http/traces.rs` around lines 710 - 731, Update build_auth_headers to return a configuration error when multi-tenancy is enabled but CLUSTER_SECRET is unavailable, and ensure send_query_request propagates that error before sending. Replace the silent HeaderValue::from_str failures with propagated errors so authentication headers are never partially constructed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/handlers/http/demo_data.rs`:
- Around line 60-62: Update all three execute_demo_script call sites in
src/handlers/http/demo_data.rs at lines 60-62, 69-71, and 81-83 to use
tokio::task::spawn_blocking instead of detached async tasks. Convert
execute_demo_script to synchronous execution, and log both script execution
errors and spawn_blocking task-join errors.
In `@src/handlers/http/otel_generator.rs`:
- Around line 49-60: Validate duration_secs in start_otel_generator before
starting the generator: reject zero and values above
MAX_GENERATOR_DURATION_SECS, using the proposed seven-day constant alongside
default_duration. Return the established client-error response for invalid input
while preserving the 24-hour default when the request omits duration_secs.
- Around line 256-265: Update generator_authorization to avoid embedding the
configured administrator username and password in the OtelGenerator session; use
a scoped credential limited to Action::Ingest or propagate the caller’s
authorization instead. Also update the generator endpoint selection in get_url
so generator exports require TLS rather than falling back to http.
In `@src/handlers/http/traces.rs`:
- Around line 702-705: Update the unexpected-response handling in the traces
query handler to avoid exposing response contents: replace the warn! payload
interpolation with a non-sensitive description of the response shape, and return
a fixed error message without response data. Use the existing TraceError variant
that matches the API contract rather than embedding the upstream payload.
- Around line 596-613: Update the recursive CTE span_hierarchy to impose a
finite depth bound on its recursive term, such as a WHERE condition on sh.level,
while preserving the existing parent-link join and span_levels MIN(level)
aggregation.
- Around line 270-313: Update the trace detail flow around
validate_trace_dataset, build_trace_bounds_sql, and build_trace_detail_sql to
pass dataset_info.time_column into both SQL builders. Ensure the bounds
aggregate and detail query use the dataset’s configured time partition column,
including the p_timestamp fallback, instead of relying on a hardcoded column.
- Around line 418-420: Remove the no-op count_api_bin_interval call and its
misleading validation comment in the surrounding handler, or replace them with
explicit validation that rejects unsupported time ranges before continuing.
- Around line 341-349: Update build_conditions_filter and the SQL construction
used by get_filter_string to protect request-supplied condition.column values:
either escape embedded double quotes by doubling them before interpolation or
validate each column against the dataset schema. Preserve existing literal-value
escaping and filter generation behavior for valid columns.
---
Nitpick comments:
In `@src/handlers/http/modal/server.rs`:
- Around line 254-297: Reduce duplication between get_otel_generator_webscope
and get_otel_generator_ingest_webscope by extracting shared route construction
into a helper that conditionally applies IntraClusterRequest, or by reusing one
resource definition. Preserve the existing handlers, methods, authorization,
path, and ensure only the ingest scope applies the wrapper.
In `@src/handlers/http/otel_generator.rs`:
- Around line 211-219: Update the authorization-forwarding condition in the
request-building flow to check whether the HTTP method is Method::POST instead
of checking body.is_some(). Preserve the existing multi-tenant and
authorization-header checks and forwarding behavior.
In `@src/handlers/http/traces.rs`:
- Around line 710-731: Update build_auth_headers to return a configuration error
when multi-tenancy is enabled but CLUSTER_SECRET is unavailable, and ensure
send_query_request propagates that error before sending. Replace the silent
HeaderValue::from_str failures with propagated errors so authentication headers
are never partially constructed.
In `@src/otel_generator.rs`:
- Around line 84-94: Remove the unused running field from OtelGeneratorStatus,
since state already conveys the status and the field is currently excluded from
serialization. Update both status construction sites to stop initializing
running while preserving the remaining fields and serialization behavior.
- Around line 224-275: The stop and status methods never produce errors, so
change OtelGenerator::stop and OtelGenerator::status to return
OtelGeneratorResult and OtelGeneratorStatus directly. Update stop_otel_generator
and get_otel_generator_status to remove unreachable Err handling and return the
direct values while preserving existing responses.
- Around line 783-806: Make lifecycle_is_per_tenant deterministic by adding and
using a test-supported way to run start without performing exporter HTTP
requests, such as a zero-tick mode or injectable sender. Update the relevant
OtelGenerator start/run_generator path while preserving normal export behavior,
then configure the test to use the non-networking mode and retain its tenant
lifecycle assertions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 79ee0358-8500-4064-a463-84868bbeecb3
📒 Files selected for processing (10)
src/handlers/http/cluster/mod.rssrc/handlers/http/demo_data.rssrc/handlers/http/mod.rssrc/handlers/http/modal/ingest_server.rssrc/handlers/http/modal/query_server.rssrc/handlers/http/modal/server.rssrc/handlers/http/otel_generator.rssrc/handlers/http/traces.rssrc/lib.rssrc/otel_generator.rs
💤 Files with no reviewable changes (1)
- src/handlers/http/cluster/mod.rs
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/handlers/http/demo_data.rs (1)
84-99: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRequire a successful liveness status before selecting an ingestor.
check_livenessmust reject HTTP error responses, such as503, instead of treating transport-level success as liveness. Otherwise,get_live_ingestor_urlcan select an unavailable ingestor and skip later healthy ingestors. Add a test with a first ingestor returning503and a second returning200.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/handlers/http/demo_data.rs` around lines 84 - 99, Update check_liveness used by get_live_ingestor_url to return true only for successful HTTP status responses, rejecting statuses such as 503 so iteration continues to later ingestors. Add a test covering a first ingestor returning 503 and a second returning 200, asserting the healthy ingestor is selected.
🧹 Nitpick comments (2)
src/otel_generator.rs (1)
685-692: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winReport
request_duration_msas a gauge.
up_down_counter_metricemits aSumwithAggregationTemporality::Cumulative. The value passed is the latency of a single request, not a cumulative total. A backend that computes rates or deltas over cumulative sums will produce wrong results for this series.gauge_metricmatches the semantics of an instantaneous latency value.♻️ Proposed change
- up_down_counter_metric( + gauge_metric( "request_duration_ms", "Request duration in ms", - duration_ms as i64, - sequence, + duration_ms as f64, now, duration_metric_attributes, ),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/otel_generator.rs` around lines 685 - 692, Update the request_duration_ms metric construction in the surrounding generator flow to use gauge_metric instead of up_down_counter_metric, preserving the existing name, description, duration value, sequence, timestamp, and attributes.src/alerts/alerts_utils.rs (1)
895-913: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the null-condition path.
The implementation quotes identifiers in null, list, and scalar predicates. The test covers only scalar and list predicates. Add a null-condition assertion with an identifier containing a double quote and injection text.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/alerts/alerts_utils.rs` around lines 895 - 913, Add coverage for the null-condition path in condition_columns_are_quoted_as_identifiers by invoking the null predicate helper with an identifier containing a double quote and injection text, then assert the resulting SQL quotes and escapes the identifier correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/handlers/http/demo_data.rs`:
- Around line 101-116: Update spawn_demo_script and execute_demo_script to
accept and propagate the normalized tenant identity, using a tenant-scoped
credential for direct ingestion or preserving the authenticated intra-cluster
forwarding path. Ensure the generated ingest request includes the
server-controlled tenant context and cannot be overridden by client-supplied
tenant values, while retaining the existing script execution behavior.
---
Outside diff comments:
In `@src/handlers/http/demo_data.rs`:
- Around line 84-99: Update check_liveness used by get_live_ingestor_url to
return true only for successful HTTP status responses, rejecting statuses such
as 503 so iteration continues to later ingestors. Add a test covering a first
ingestor returning 503 and a second returning 200, asserting the healthy
ingestor is selected.
---
Nitpick comments:
In `@src/alerts/alerts_utils.rs`:
- Around line 895-913: Add coverage for the null-condition path in
condition_columns_are_quoted_as_identifiers by invoking the null predicate
helper with an identifier containing a double quote and injection text, then
assert the resulting SQL quotes and escapes the identifier correctly.
In `@src/otel_generator.rs`:
- Around line 685-692: Update the request_duration_ms metric construction in the
surrounding generator flow to use gauge_metric instead of
up_down_counter_metric, preserving the existing name, description, duration
value, sequence, timestamp, and attributes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 57a68959-cb23-40d6-86df-1286e69fe216
📒 Files selected for processing (6)
src/alerts/alerts_utils.rssrc/handlers/http/demo_data.rssrc/handlers/http/modal/server.rssrc/handlers/http/otel_generator.rssrc/handlers/http/traces.rssrc/otel_generator.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/handlers/http/otel_generator.rs
- src/handlers/http/traces.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/handlers/http/mod.rs (1)
110-123: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a case for the opaque-origin filter.
The test covers CDN inclusion, path stripping, and deduplication. It does not cover the
origin != "null"branch. Add a URL with a non-special scheme, for exampledata:orfile:///tmp/app, and assert the result excludes"null".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/handlers/http/mod.rs` around lines 110 - 123, Add an opaque-origin case to cdn_origin_is_always_allowed_in_addition_to_configured_origins by including a non-special-scheme URL such as data: or file:///tmp/app in configured, then assert normalize_cors_origins excludes "null" while preserving the existing CDN, path-stripping, and deduplication expectations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/handlers/http/mod.rs`:
- Around line 79-104: Update normalize_cors_origins and its callers to include
PARSEABLE_CDN_ORIGIN only when the new configuration flag explicitly enables it,
defaulting that flag to disabled for self-hosted deployments. Pass the flag into
normalize_cors_origins so the behavior remains testable, and update the related
unit test to cover the opt-in CDN origin.
---
Nitpick comments:
In `@src/handlers/http/mod.rs`:
- Around line 110-123: Add an opaque-origin case to
cdn_origin_is_always_allowed_in_addition_to_configured_origins by including a
non-special-scheme URL such as data: or file:///tmp/app in configured, then
assert normalize_cors_origins excludes "null" while preserving the existing CDN,
path-stripping, and deduplication expectations.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f4bcc232-47e6-47ed-8aeb-60219325e673
📒 Files selected for processing (2)
src/handlers/http/mod.rssrc/otel_generator.rs
2 participants
Summary by CodeRabbit
New Features
Improvements