Skip to content

Conversation

@parmesant
Copy link
Contributor

@parmesant parmesant commented Dec 11, 2025

In case of content-type JSON, metric type Summary was not getting deserialized properly due to missing fields and improper type conversion

in the otel proto package

Fixes #XXXX.

Description


This PR has:

  • been tested to ensure log ingestion and log query works.
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added documentation for new or modified features or behaviors.

Summary by CodeRabbit

  • New Features

    • Made opentelemetry_proto publicly accessible in the crate API.
  • Chores

    • Switched opentelemetry-proto dependency to a git-based source (keeps prior feature set) and adjusted dependency declarations.
  • Style

    • Minor formatting cleanups in manifest and code (spacing, trailing commas, small refactorings).

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 11, 2025

Walkthrough

Replaces the opentelemetry-proto dependency with a git-based source (specific rev) and adds a public re-export of opentelemetry_proto from the crate root; also includes minor formatting changes in a query module.

Changes

Cohort / File(s) Summary
Dependency manifest edits
Cargo.toml
Replaces opentelemetry-proto = "0.30.0" entry with a git-based dependency (specific repo and rev), preserves features (gen-tonic, with-serde, logs, metrics, trace), and adds a commented note showing previous approach and rationale. Also reformats some other dependency feature lists (multi-line arrays, trailing commas).
Public re-export
src/lib.rs
Adds pub use opentelemetry_proto; to publicly re-export the opentelemetry_proto crate from the crate root (with a preceding comment).
Minor formatting change
src/query/stream_schema_provider.rs
Small formatting rewrite inside StandardTableProvider::create_parquet_physical_plan when mapping partitions to file_groups (single-line map instead of multi-line); no functional change.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Check the git rev and repository URL in Cargo.toml for correctness and intended permanence.
  • Ensure the public pub use opentelemetry_proto; won't unintentionally expose undesired types or collide with existing names.
  • Quick lint/format verification for the formatting-only change in stream_schema_provider.rs.

Possibly related PRs

  • Protobuf ingestion #1391 — Related work consuming opentelemetry-proto types; likely to be impacted by the dependency/re-export change.

Suggested reviewers

  • nitisht

Poem

🐰 I hopped in code to lend a paw,
A proto branch to fix the law,
Now metrics speak with clearer tune,
I share the types beneath the moon,
Hop, ship, and patch — a joyful chew. 🥕

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Title check ⚠️ Warning The PR title 'Fix for otel-metrics JSON' is too vague and doesn't clearly describe the specific issue being addressed—it lacks detail about what the actual fix entails. Improve the title to be more specific, such as 'Fix Summary metric deserialization for JSON content-type in opentelemetry-proto' or similar.
Description check ⚠️ Warning The PR description is largely incomplete: it lacks a filled issue reference, provides minimal context about the solution, misses key implementation details, and has all testing/documentation checkboxes unchecked. Complete the description by filling the issue number, explaining the root cause and solution in detail, summarizing key changes (including why opentelemetry-proto was switched to git), and marking relevant checkboxes.
✅ Passed checks (1 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6752bee and 8f63a79.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (3)
  • Cargo.toml (4 hunks)
  • src/lib.rs (1 hunks)
  • src/query/stream_schema_provider.rs (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • src/query/stream_schema_provider.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-18T11:15:10.836Z
Learnt from: nikhilsinhaparseable
Repo: parseablehq/parseable PR: 1340
File: src/storage/object_storage.rs:31-41
Timestamp: 2025-06-18T11:15:10.836Z
Learning: DataFusion's parquet reader defaults to using view types (Utf8View, BinaryView) when reading parquet files via the schema_force_view_types configuration (default: true). This means StringViewArray and BinaryViewArray downcasting is required when processing Arrow arrays from DataFusion parquet operations, even though these types are behind nightly feature flags.

Applied to files:

  • Cargo.toml
🧬 Code graph analysis (1)
src/lib.rs (2)
src/handlers/airplane.rs (1)
  • server (286-351)
src/handlers/livetail.rs (1)
  • server (174-233)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (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 Default x86_64-unknown-linux-gnu
  • GitHub Check: Build Kafka aarch64-apple-darwin
  • GitHub Check: Build Kafka 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: coverage
🔇 Additional comments (3)
Cargo.toml (3)

14-17: LGTM!

Multi-line formatting improves readability and aligns with project conventions.


39-45: LGTM!

Multi-line formatting improves readability for the five features.


89-105: Clarify the comment terminology and add upstream tracking link.

Good progress: The dependency now correctly pins to a commit hash (line 99), addressing reproducibility concerns.

However, the comment at line 98 needs improvement:

  • It says "custom branch" but actually uses rev (commit hash)—this is misleading and should be corrected
  • Add upstream PR/issue link for tracking (you'll need to provide the link to the opentelemetry-rust issue or PR this fix addresses)
  • Add a TODO comment to revert once merged upstream

Suggested improvement:

-# add custom branch with fix until it gets merged
+# Temporary: Using fork commit with JSON deserialization fix for Summary metrics
+# TODO: Revert to crates.io version once upstream PR is merged
+# Upstream tracking: [link to opentelemetry-rust issue/PR]
 opentelemetry-proto = { git = "https://github.com/parmesant/opentelemetry-rust/", rev = "45fb828769e6ade96d56ca1f5fa14cf0986a5341", features = [

Comment @coderabbitai help to get the list of available commands and usage tips.

In case of content-type JSON, metric type Summary was not getting deserialized properly due to missing fields and improper type conversion

in the otel proto package
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
Cargo.toml (1)

80-87: Remove commented-out dependency.

The commented-out code adds clutter and can cause confusion. Since there's already an explanation in line 89 about using the custom branch temporarily, the old version is no longer needed.

Apply this diff:

-# # Logging and Metrics
-# opentelemetry-proto = { version = "0.30.0", features = [
-#     "gen-tonic",
-#     "with-serde",
-#     "logs",
-#     "metrics",
-#     "trace",
-# ] }
-
-# add custom branch with fix until it gets merged
+# Logging and Metrics
+# Using custom branch with Summary metrics JSON fix until it gets merged upstream
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 072330f and 6752bee.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (2)
  • Cargo.toml (1 hunks)
  • src/lib.rs (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-21T02:22:24.403Z
Learnt from: nikhilsinhaparseable
Repo: parseablehq/parseable PR: 1448
File: src/parseable/mod.rs:419-432
Timestamp: 2025-10-21T02:22:24.403Z
Learning: In Parseable's internal stream creation (`create_internal_stream_if_not_exists` in `src/parseable/mod.rs`), errors should not propagate to fail server initialization. The function creates both pmeta and pbilling internal streams, and failures are logged but the function always returns `Ok(())` to ensure server startup resilience. Individual stream creation failures should not prevent syncing of successfully created streams.

Applied to files:

  • src/lib.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
  • GitHub Check: coverage
  • GitHub Check: Build Default x86_64-unknown-linux-gnu
  • GitHub Check: Build Default aarch64-unknown-linux-gnu
  • GitHub Check: Build Default x86_64-pc-windows-msvc
  • GitHub Check: Build Kafka x86_64-unknown-linux-gnu
  • GitHub Check: Build Kafka aarch64-apple-darwin
  • GitHub Check: Quest Smoke and Load Tests for Distributed deployments
  • GitHub Check: Quest Smoke and Load Tests for Standalone deployments

@nitisht nitisht merged commit 27056b2 into parseablehq:main Dec 11, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants