refactor: specialized flatten happens at the same semantic location #1177
Conversation
WalkthroughThe changes simplify the ingestion process for OpenTelemetry data by consolidating the logic used in handling OTEL logs, metrics, and traces. In the HTTP handler, custom deserialization and flattening have been removed and replaced with a unified call to Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client Request
participant Handler as HTTP Handler (ingest.rs)
participant Utils as Ingestion Utils (flatten_and_push_logs)
participant Flattener as OTEL Flatten Functions
participant Stream as Log Stream (push_logs)
Client->>Handler: Send OTEL ingestion request
Handler->>Utils: Call flatten_and_push_logs(json, stream, log source)
Utils->>Flattener: Deserialize & flatten based on log source
Flattener-->>Utils: Return flattened data
Utils->>Stream: push_logs(flattened data)
Stream-->>Utils: Acknowledge log push
Utils-->>Handler: Return result
Handler-->>Client: Send response
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
⏰ Context from checks skipped due to timeout of 90000ms (10)
🔇 Additional comments (9)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
de-sh
force-pushed
the
ingest-without-refactor
branch
from
66f231a to
a46e4db
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/handlers/http/modal/utils/ingest_utils.rs (2)
46-48: Consider partial success or error aggregation
Currently, if one record fails to be pushed, the entire loop exits with an error. If partial success is desired, you could collect errors while continuing to process the remaining records.Would you like me to propose a pattern for error aggregation or partial success handling?
50-56: Potential logging enhancement
It may be helpful to log how many Otel logs were processed or flattened to support observability.src/handlers/http/ingest.rs (1)
44-44: Unused import?
push_logsappears to be imported here but not actually used. Consider removing it if it’s truly unused to keep imports clean.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/handlers/http/ingest.rs(3 hunks)src/handlers/http/modal/utils/ingest_utils.rs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: coverage
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
🔇 Additional comments (8)
src/handlers/http/modal/utils/ingest_utils.rs (5)
22-24: No issues with the new imports
Imports for OTEL data structures look appropriate and provide the necessary context for processing logs, metrics, and traces.
32-35: Approved changes to import references
The expanded imports of OTEL flatten methods seem correct and well-integrated.
57-63: Consistent OtelTraces approach
The code mirrors the OtelLogs logic; no issues spotted.
64-70: Consistent OtelMetrics approach
Implementation aligns with the existing patterns for logs and traces.
78-78:❌ Incorrect review comment
Confirm non-public visibility
Removingpubmay limit its usage to this module only. Verify that no external calls requirepush_logs.
🏁 Script executed:
#!/bin/bash # Searching for usage of push_logs throughout the codebase rg 'push_logs' -A 2Length of output: 3932
Keep
push_logsas public.
Our search results confirm thatpush_logsis being imported and used externally (e.g., insrc/handlers/http/ingest.rs), so removingpubwould break these external references. Please ensure that its public visibility is maintained.Likely an incorrect or invalid review comment.
src/handlers/http/ingest.rs (3)
133-133: Unified ingestion call
Callingflatten_and_push_logshere neatly removes duplication. Nicely done.
164-164: Streamlined OTEL metrics ingestion
Consistent with the updated architecture for OTEL ingestion; looks good.
192-192: Consistent traces ingestion
Aligns with the new flattening approach. No concerns.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/handlers/http/ingest.rs (1)
332-825: Consider adding tests for the unified flattening logic.While the test coverage is comprehensive for data transformation and schema validation, it would be beneficial to add specific tests for the unified
flatten_and_push_logsfunction to ensure it correctly handles all OTEL log sources (logs, metrics, and traces).Would you like me to help generate test cases for the unified flattening logic?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/handlers/http/ingest.rs(4 hunks)src/handlers/http/modal/utils/ingest_utils.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/handlers/http/modal/utils/ingest_utils.rs
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: coverage
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
🔇 Additional comments (2)
src/handlers/http/ingest.rs (2)
41-41: LGTM!The import is correctly placed and follows Rust's module system conventions.
130-130: Great refactoring!The consolidation of flattening logic into
flatten_and_push_logsacross all OTEL handlers improves code maintainability while preserving the specific validation requirements for each log source.Also applies to: 161-161, 189-189
Fixes #XXXX.
Description
This PR has:
Summary by CodeRabbit
New Features
Refactor
push_logsfunction to private, streamlining the function's access.