[cluster telemetry] Internal service conversions by coszio · Pull Request #7631 · qdrant/qdrant

coszio · 2025-11-27T21:19:19Z

Depends on #7599

Implements conversions between internal telemetry service and existing types.

Tasks

grpc -> existing
existing -> grpc

coderabbitai

Actionable comments posted: 2

🧹 Nitpick comments (1)

lib/storage/src/content_manager/conversions.rs (1)

267-295: Prefer explicit type conversions over .into() and .try_into().

Lines 273-275 and 291 use implicit conversions. As per coding guidelines, prefer explicit conversions for better code clarity:

 match action {
-    Some(grpc::alias_operations::Action::CreateAlias(create)) => Ok(create.into()),
-    Some(grpc::alias_operations::Action::DeleteAlias(delete)) => Ok(delete.into()),
-    Some(grpc::alias_operations::Action::RenameAlias(rename)) => Ok(rename.into()),
+    Some(grpc::alias_operations::Action::CreateAlias(create)) => Ok(AliasOperations::from(create)),
+    Some(grpc::alias_operations::Action::DeleteAlias(delete)) => Ok(AliasOperations::from(delete)),
+    Some(grpc::alias_operations::Action::RenameAlias(rename)) => Ok(AliasOperations::from(rename)),
     _ => Err(Status::invalid_argument("Malformed AliasOperation type")),
 }

And in ChangeAliases conversion:

 let actions: Vec<AliasOperations> = actions
     .into_iter()
-    .map(|a| a.try_into())
+    .map(|a| AliasOperations::try_from(a))
     .collect::<Result<_, _>>()?;

Based on coding guidelines: Prefer explicit SomeType::from(x) over implicit x.into() in Rust.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a412db6 and 94c4271.

📒 Files selected for processing (15)

docs/redoc/master/openapi.json (3 hunks)
lib/api/src/grpc/proto/telemetry_internal.proto (4 hunks)
lib/api/src/grpc/qdrant.rs (4 hunks)
lib/collection/src/collection/telemetry.rs (1 hunks)
lib/collection/src/operations/conversions.rs (3 hunks)
lib/collection/src/operations/types.rs (2 hunks)
lib/collection/src/shards/resharding.rs (3 hunks)
lib/collection/src/shards/shard_holder/mod.rs (4 hunks)
lib/collection/src/shards/shard_holder/resharding.rs (10 hunks)
lib/collection/src/telemetry.rs (2 hunks)
lib/storage/src/content_manager/conversions.rs (10 hunks)
src/common/metrics.rs (1 hunks)
src/common/telemetry.rs (5 hunks)
src/common/telemetry_ops/conversions.rs (1 hunks)
src/common/telemetry_ops/mod.rs (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (5)

src/common/metrics.rs
lib/collection/src/operations/types.rs
src/common/telemetry_ops/conversions.rs
lib/collection/src/shards/shard_holder/mod.rs
lib/collection/src/shards/shard_holder/resharding.rs

🧰 Additional context used

📓 Path-based instructions (1)

**/*.rs

📄 CodeRabbit inference engine (.github/review-rules.md)

**/*.rs: Prefer explicit SomeType::from(x) over implicit x.into() in Rust
In new code, don't use transmute_from_u8, transmute_to_u8, transmute_from_u8_to_slice, transmute_from_u8_to_mut_slice, transmute_to_u8_slice - use bytemuck or zerocopy crates instead
Prefer explicit exhaustive matches over catch-all _ arm in Rust match expressions, so we don't miss handling new enum variants when they are added later (exceptions: tests and benchmarks, or when sure new variants won't affect logic)
Prefer explicit field ignoring using : _ over using .. in Rust struct destructuring (exceptions: tests and benchmarks, or when sure new fields won't affect logic)

Files:

lib/collection/src/operations/conversions.rs
lib/collection/src/collection/telemetry.rs
lib/collection/src/shards/resharding.rs
src/common/telemetry_ops/mod.rs
lib/collection/src/telemetry.rs
lib/api/src/grpc/qdrant.rs
src/common/telemetry.rs
lib/storage/src/content_manager/conversions.rs

🧠 Learnings (5)

📚 Learning: 2025-09-16T19:14:17.614Z

Learnt from: generall
Repo: qdrant/qdrant PR: 7183
File: lib/api/src/grpc/qdrant.rs:4263-4273
Timestamp: 2025-09-16T19:14:17.614Z
Learning: In qdrant, lib/api/src/grpc/qdrant.rs is auto-generated by prost-build; do not edit it directly. Make changes in lib/api/src/grpc/proto/points.proto (e.g., add [deprecated=true], doc comments, or encoding options), then regenerate the Rust code.

Applied to files:

lib/collection/src/operations/conversions.rs
lib/api/src/grpc/qdrant.rs
lib/storage/src/content_manager/conversions.rs

📚 Learning: 2025-09-08T08:47:43.795Z

Learnt from: timvisee
Repo: qdrant/qdrant PR: 7188
File: lib/collection/src/update_handler.rs:810-813
Timestamp: 2025-09-08T08:47:43.795Z
Learning: In the Qdrant codebase, CollectionId implements traits that allow &CollectionId to convert Into<String>, so passing &collection_name to functions requiring Into<String> works correctly without needing .clone() or .to_string().

Applied to files:

lib/collection/src/operations/conversions.rs
lib/storage/src/content_manager/conversions.rs

📚 Learning: 2025-08-10T18:30:02.986Z

Learnt from: generall
Repo: qdrant/qdrant PR: 7006
File: lib/collection/src/operations/verification/update.rs:158-174
Timestamp: 2025-08-10T18:30:02.986Z
Learning: In Qdrant's strict mode verification code (lib/collection/src/operations/verification/update.rs), exhaustive pattern matching without `..` is intentionally used for structs like PointsBatch and PointsList. This design pattern ensures compilation fails when new fields are added, forcing developers to explicitly consider how new fields should be handled in the indexed_filter_write method. This provides visibility and compile-time safety for struct evolution.

Applied to files:

lib/collection/src/operations/conversions.rs
src/common/telemetry.rs

📚 Learning: 2025-08-15T15:56:36.821Z

Learnt from: coszio
Repo: qdrant/qdrant PR: 7065
File: lib/collection/src/operations/universal_query/shard_query.rs:0-0
Timestamp: 2025-08-15T15:56:36.821Z
Learning: Internal gRPC APIs in Qdrant don't require additional runtime validation when proto-level validation is already configured via build.rs range constraints.

Applied to files:

lib/storage/src/content_manager/conversions.rs

📚 Learning: 2025-08-10T18:25:16.206Z

Learnt from: generall
Repo: qdrant/qdrant PR: 7006
File: docs/redoc/master/openapi.json:10626-10634
Timestamp: 2025-08-10T18:25:16.206Z
Learning: In Qdrant, docs/redoc/master/openapi.json is autogenerated from the Rust REST schema definitions (e.g., lib/api/src/rest/schema.rs). To change OpenAPI descriptions, update the Rust struct field doc comments/attributes rather than editing the JSON directly.

Applied to files:

lib/storage/src/content_manager/conversions.rs

🧬 Code graph analysis (3)

lib/collection/src/collection/telemetry.rs (1)

lib/collection/src/telemetry.rs (7)

from (162-183)

from (222-232)

from (236-246)

from (250-260)

from (288-297)

from (331-351)

from (417-448)

src/common/telemetry.rs (4)

lib/collection/src/operations/verification/mod.rs (1)

new_unchecked_verification_pass (26-28)

src/common/telemetry_ops/app_telemetry.rs (1)

collect (76-100)

lib/collection/src/telemetry.rs (11)

try_from (200-218)

try_from (266-284)

try_from (303-327)

try_from (357-413)

from (162-183)

from (222-232)

from (236-246)

from (250-260)

from (288-297)

from (331-351)

from (417-448)

lib/common/common/src/types.rs (1)

new (40-42)

lib/storage/src/content_manager/conversions.rs (1)

lib/api/src/grpc/conversions.rs (16)

from (132-147)

from (151-162)

from (166-177)

from (181-188)

from (192-204)

from (208-224)

from (228-240)

from (244-249)

from (253-286)

from (290-295)

from (299-311)

from (315-327)

from (331-342)

from (346-357)

from (361-372)

from (376-402)

⏰ 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). (11)

GitHub Check: e2e-tests
GitHub Check: test-shard-snapshot-api-s3-minio
GitHub Check: rust-tests (macos-latest)
GitHub Check: test-consistency
GitHub Check: rust-tests (windows-latest)
GitHub Check: test-consensus-compose
GitHub Check: rust-tests (ubuntu-latest)
GitHub Check: integration-tests
GitHub Check: integration-tests-consensus
GitHub Check: Build Qdrant Edge Python bindings
GitHub Check: lint

🔇 Additional comments (29)

lib/collection/src/shards/resharding.rs (1)

65-70: The match expressions in the codebase already handle the new WriteHashRingCommitted variant exhaustively. No issues found.

The only match expression on ReshardingStage (in lib/collection/src/shards/shard_holder/resharding.rs:632) explicitly handles all three variants without catch-all patterns:

MigratingPoints | ReadHashRingCommitted

WriteHashRingCommitted

None

The conversion functions in telemetry.rs and grpc/qdrant.rs also handle all variants. No verification is required.

Likely an incorrect or invalid review comment.

src/common/telemetry_ops/mod.rs (1)

4-4: LGTM!

The private module declaration for telemetry conversions is appropriate and follows standard Rust module organization patterns.

lib/collection/src/collection/telemetry.rs (1)

40-47: LGTM!

The changes correctly wrap telemetry fields in Some(...) to align with the updated optional field signatures. The use of explicit CollectionConfigTelemetry::from(...) follows the coding guidelines, and the .then_some() pattern for conditional shard_clean_tasks is idiomatic Rust.

src/common/telemetry.rs (3)

49-51: LGTM!

The field visibility and optionality changes align with the gRPC telemetry schema updates. Making app optional with skip_serializing_if is appropriate for fields that may not be present in internal service telemetry.

186-222: LGTM!

The reverse conversion from TelemetryData to grpc::PeerTelemetry is well-structured:

Uses explicit field destructuring with : _ following coding guidelines

Properly handles the CollectionTelemetryEnum::Aggregated variant with a clear error message

Applies explicit type conversions (grpc::CollectionTelemetry::from, grpc::ClusterTelemetry::from)

149-184: The empty id assignment is intentional and correct.

The grpc::PeerTelemetry message reserves fields 1 (id) and 2 (app) in the proto definition, so peer telemetry from remote nodes never includes process-specific identifiers. Setting id: "".to_string() is the appropriate placeholder for converting peer telemetry into TelemetryData, where id is a non-optional String field. This aligns with the bidirectional conversion logic: when converting back to gRPC (line 191), these fields are explicitly ignored since they don't belong in peer telemetry messages. The conversion correctly handles all other optional fields.

lib/collection/src/operations/conversions.rs (3)

1419-1419: LGTM!

Explicitly ignoring the stage field with a clear comment is good practice. The comment explains that stage is only communicated for ReshardingTelemetry in the internal service, making the intent clear for future maintainers.

1439-1446: LGTM!

The bidirectional conversion between gRPC and internal ReshardingDirection types uses exhaustive pattern matching without a catch-all _ arm, following the coding guidelines. This ensures that any new enum variants added in the future will be caught at compile time.

1590-1603: LGTM!

The conversion from internal to gRPC ShardTransferMethod uses exhaustive pattern matching, ensuring all variants are explicitly handled. This follows the coding guidelines and provides compile-time safety when new transfer methods are added.

lib/api/src/grpc/proto/telemetry_internal.proto (3)

13-26: LGTM!

The new GetPeerTelemetryResponse and PeerTelemetry messages are well-structured. The use of reserved fields (1: id, 2: app, 5-7: requests/memory/hardware) appropriately documents fields that are not communicated in the internal service but exist in other telemetry contexts.

104-104: LGTM!

The schema changes improve telemetry flexibility:

Adding map<uint64, PeerInfo> peers in ClusterTelemetry enables per-peer metadata

Making role and peer_id optional in ClusterStatusTelemetry is appropriate, as these fields may not always be available depending on cluster state

Also applies to: 114-116

139-141: LGTM!

The new PeerInfo message and PRE_CANDIDATE state addition are appropriate:

PeerInfo with a uri field provides basic peer connectivity information

PRE_CANDIDATE aligns with Raft consensus protocol states for pre-voting

Also applies to: 147-147

lib/collection/src/telemetry.rs (8)

24-28: LGTM!

Making init_time_ms and config optional with skip_serializing_if is appropriate for telemetry data that may not always be available, particularly when converting from the internal service gRPC representation.

186-219: LGTM!

The ShardTransferInfo conversion from gRPC handles all fields appropriately:

Uses explicit ShardTransferMethod::from(...) following coding guidelines

Properly handles empty strings for optional comment field

Error messages are clear and specific

221-247: LGTM!

The bidirectional ReshardingStage conversions use exhaustive pattern matching without catch-all _ arms, following the coding guidelines. This ensures compile-time verification when new stages are added.

249-261: LGTM!

The conversion from internal to gRPC ShardTransferTelemetry correctly:

Uses explicit grpc::ShardTransferMethod::from(...) following guidelines

Handles optional fields with unwrap_or_default() for gRPC representation

263-298: LGTM!

The bidirectional ReshardingInfo ↔ grpc::ReshardingTelemetry conversions are well-implemented:

Uses explicit type conversions throughout (following coding guidelines)

Proper error handling with descriptive messages

UUID parsing with appropriate error handling

300-352: LGTM!

The ShardCleanStatusTelemetry conversions use exhaustive pattern matching for all enum variants (Started, Progress, Done, Failed, Cancelled), following the coding guidelines. This ensures new variants will be caught at compile time.

354-414: LGTM!

The CollectionTelemetry conversion from gRPC is thorough and well-documented:

Explicit field destructuring follows coding guidelines

Clear comments explain fields not provided in internal service (lines 406-408)

Proper handling of empty collections with Option<Vec<...>> pattern

Error handling is comprehensive

416-449: LGTM!

The reverse conversion from internal to gRPC CollectionTelemetry correctly:

Uses explicit field destructuring with : _ for ignored fields (follows coding guidelines)

Flattens optional vectors with into_iter().flatten() pattern

Applies explicit grpc::*::from() conversions throughout

docs/redoc/master/openapi.json (2)

12032-12036: Required set relaxed to only “id” — double‑check API compatibility.

CollectionTelemetry no longer requires config or init_time_ms. Ensure this is a deliberate, versioned change and client SDKs relying on previous required fields won’t break. Consider documenting behavior when these fields are absent.

12040-12045: Nullable numeric with constraints — verify generator handling.

init_time_ms is now nullable with minimum: 0. Most tools ignore minimum when value is null, but some generators misinterpret. Please validate with your OpenAPI linter/CI.

lib/api/src/grpc/qdrant.rs (3)

12332-12350: Peer telemetry response/message shape looks consistent and well-scoped

GetPeerTelemetryResponse with result: Option<PeerTelemetry> and time: f64, plus PeerTelemetry aggregating per‑collection and cluster telemetry, aligns with existing response patterns in this file and keeps peer telemetry nicely encapsulated.

12470-12496: New peers map and optional role/peer_id: OK, but note API surface change

Adding ClusterTelemetry.peers: HashMap<u64, PeerInfo> is straightforward and correctly typed for peer IDs → URIs.

Changing ClusterStatusTelemetry.role and peer_id from plain values to Option<…> is wire‑compatible but changes the generated client API (Rust and other languages) from required to optional fields.

If the gRPC/proto surface is considered public and semver‑stable, double‑check that this behavioral/API change is intentional and documented in release notes.

12540-12545: PeerInfo message is minimal and coherent with new peers map

PeerInfo { uri: String } is a simple, focused message type and matches the usage in ClusterTelemetry.peers without colliding with the existing Peer { uri, id } raft type.

lib/storage/src/content_manager/conversions.rs (4)

5-6: LGTM!

The new imports are appropriate for the conversions implemented in this file. The grpc alias improves readability throughout the conversion implementations.

Also applies to: 23-23

68-226: LGTM!

The collection operation conversions are well-structured:

Exhaustive destructuring without .. catch-all (per coding guidelines)

Proper error handling with ? operator

Consistent use of grpc:: prefix

The strict_mode_from_api helper correctly maps all configuration fields

297-317: LGTM!

The bidirectional StateRole conversions are well-implemented:

Exhaustive pattern matching without _ catch-all (per coding guidelines)

Both directions handle all variants consistently (Follower, Candidate, Leader, PreCandidate)

Infallible conversions using From trait appropriately

319-373: LGTM!

The bidirectional ConsensusThreadStatus conversions are correctly implemented:

Exhaustive pattern matching for all variants (Working, Stopped, StoppedWithErr)

Proper timestamp validation when converting from milliseconds (line 328) with clear error message

The None case (line 343) reasonably defaults to Stopped as a fallback for unset protobuf fields

Reverse conversion (lines 348-373) correctly converts DateTime to milliseconds

The error handling and type safety are appropriate for these conversions.

timvisee · 2025-12-15T15:02:08Z

+            config: None, // Not provided in gRPC
+            peers: (!peers.is_empty()).then_some(peers),
+            peer_metadata: None, // Not provided in gRPC
+            metadata: None,      // Not provided in gRPC


I don't like these Nones here so much. It may not be obvious in code that these will always be empty for some code paths.

At the same time I don't have any better suggestion.

I don't like duplicating all types another time just to remove those fields. And having reserved in the gRPC variants makes a lot of sense.

I think this conversion + reserved fields is the best we can do. Unless there is another suggestion to evaluate

timvisee · 2025-12-15T16:01:41Z

+        let cluster = cluster.map(ClusterTelemetry::try_from).transpose()?;
+
+        Ok(TelemetryData {
+            id: "".to_string(),


Don't we need a collection name here?

No no, this is the telemetry collector ID, which we don't care about in internal service

timvisee

Question about shard method defaulting:

coderabbitai · 2025-12-22T16:57:36Z

📝 Walkthrough

Walkthrough

This PR updates telemetry schemas, protobufs, and conversion code: it introduces PeerTelemetry and PeerInfo in protobufs, makes several telemetry fields optional (app, CollectionTelemetry.config, init_time_ms, role, peer_id, and ShardTransferTelemetry.method), renames/extends resharding enums (ReshardStage → ReshardingStage with added variants), adds new conversion TryFrom/From implementations between gRPC and internal types, emits Option-wrapped telemetry fields and shard_clean_tasks, and updates code paths to use the renamed ReshardingStage and new peers map.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Telemetry improvements #6390 — Overlapping telemetry schema and resharding-stage refactor with similar optional-field and peer-telemetry changes.

Suggested reviewers

timvisee
generall
agourlay

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 24.07% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title '[cluster telemetry] Internal service conversions' clearly summarizes the main change: implementing conversions between internal telemetry service and existing types.
Description check	✅ Passed	The description is related to the changeset, mentioning conversions between internal telemetry service and existing types, with completed tasks for gRPC->existing and existing->gRPC conversions.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch telemetry-conversions

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

coderabbitai

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)

lib/api/src/grpc/qdrant.rs (1)

12505-12516: Minor doc typo: transfer method name in comment

The comment for to_shard_id says ReshardStreamRecords, but the enum variant is ReshardingStreamRecords. Consider fixing the wording in the .proto so generated docs stay accurate.

As this file is prost-generated, this should be changed in the corresponding proto (telemetry_internal.proto) and regenerated. Based on learnings, ...

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d093bd9 and ec20421.

📒 Files selected for processing (15)

docs/redoc/master/openapi.json
lib/api/src/grpc/proto/telemetry_internal.proto
lib/api/src/grpc/qdrant.rs
lib/collection/src/collection/telemetry.rs
lib/collection/src/operations/conversions.rs
lib/collection/src/operations/types.rs
lib/collection/src/shards/resharding.rs
lib/collection/src/shards/shard_holder/mod.rs
lib/collection/src/shards/shard_holder/resharding.rs
lib/collection/src/telemetry.rs
lib/storage/src/content_manager/conversions.rs
src/common/metrics.rs
src/common/telemetry.rs
src/common/telemetry_ops/conversions.rs
src/common/telemetry_ops/mod.rs

🧰 Additional context used

📓 Path-based instructions (1)

**/*.rs

📄 CodeRabbit inference engine (.github/review-rules.md)

**/*.rs: Prefer explicit SomeType::from(x) over implicit x.into() in Rust
In new code, don't use transmute_from_u8, transmute_to_u8, transmute_from_u8_to_slice, transmute_from_u8_to_mut_slice, transmute_to_u8_slice - use bytemuck or zerocopy crates instead
Prefer explicit exhaustive matches over catch-all _ arm in Rust match expressions, so we don't miss handling new enum variants when they are added later (exceptions: tests and benchmarks, or when sure new variants won't affect logic)
Prefer explicit field ignoring using : _ over using .. in Rust struct destructuring (exceptions: tests and benchmarks, or when sure new fields won't affect logic)

Files:

src/common/telemetry_ops/mod.rs
lib/collection/src/operations/types.rs
src/common/telemetry_ops/conversions.rs
lib/collection/src/collection/telemetry.rs
lib/collection/src/operations/conversions.rs
src/common/metrics.rs
lib/collection/src/shards/shard_holder/resharding.rs
lib/storage/src/content_manager/conversions.rs
lib/collection/src/shards/resharding.rs
src/common/telemetry.rs
lib/collection/src/shards/shard_holder/mod.rs
lib/collection/src/telemetry.rs
lib/api/src/grpc/qdrant.rs

🧠 Learnings (9)

📚 Learning: 2025-09-16T19:14:17.614Z

Learnt from: generall
Repo: qdrant/qdrant PR: 7183
File: lib/api/src/grpc/qdrant.rs:4263-4273
Timestamp: 2025-09-16T19:14:17.614Z
Learning: In qdrant, lib/api/src/grpc/qdrant.rs is auto-generated by prost-build; do not edit it directly. Make changes in lib/api/src/grpc/proto/points.proto (e.g., add [deprecated=true], doc comments, or encoding options), then regenerate the Rust code.

Applied to files:

lib/collection/src/operations/conversions.rs
lib/storage/src/content_manager/conversions.rs
lib/api/src/grpc/qdrant.rs

📚 Learning: 2025-08-15T15:56:36.821Z

Learnt from: coszio
Repo: qdrant/qdrant PR: 7065
File: lib/collection/src/operations/universal_query/shard_query.rs:0-0
Timestamp: 2025-08-15T15:56:36.821Z
Learning: Internal gRPC APIs in Qdrant don't require additional runtime validation when proto-level validation is already configured via build.rs range constraints.

Applied to files:

lib/storage/src/content_manager/conversions.rs

📚 Learning: 2025-08-10T18:26:33.017Z

Learnt from: generall
Repo: qdrant/qdrant PR: 7006
File: docs/redoc/master/openapi.json:10393-10401
Timestamp: 2025-08-10T18:26:33.017Z
Learning: In qdrant, docs/redoc/master/openapi.json is auto-generated. Any description changes must be made in the source (e.g., lib/api/src/rest/schema.rs and/or lib/api/src/grpc/proto/points.proto), not edited directly in the JSON.

Applied to files:

lib/storage/src/content_manager/conversions.rs

📚 Learning: 2025-11-25T11:56:33.176Z

Learnt from: CR
Repo: qdrant/qdrant PR: 0
File: .github/review-rules.md:0-0
Timestamp: 2025-11-25T11:56:33.176Z
Learning: Applies to **/*.rs : In new code, don't use `transmute_from_u8`, `transmute_to_u8`, `transmute_from_u8_to_slice`, `transmute_from_u8_to_mut_slice`, `transmute_to_u8_slice` - use `bytemuck` or `zerocopy` crates instead

Applied to files:

lib/storage/src/content_manager/conversions.rs

📚 Learning: 2025-08-10T18:25:16.206Z

Learnt from: generall
Repo: qdrant/qdrant PR: 7006
File: docs/redoc/master/openapi.json:10626-10634
Timestamp: 2025-08-10T18:25:16.206Z
Learning: In Qdrant, docs/redoc/master/openapi.json is autogenerated from the Rust REST schema definitions (e.g., lib/api/src/rest/schema.rs). To change OpenAPI descriptions, update the Rust struct field doc comments/attributes rather than editing the JSON directly.

Applied to files:

lib/storage/src/content_manager/conversions.rs

📚 Learning: 2025-09-08T08:47:43.795Z

Learnt from: timvisee
Repo: qdrant/qdrant PR: 7188
File: lib/collection/src/update_handler.rs:810-813
Timestamp: 2025-09-08T08:47:43.795Z
Learning: In the Qdrant codebase, CollectionId implements traits that allow &CollectionId to convert Into<String>, so passing &collection_name to functions requiring Into<String> works correctly without needing .clone() or .to_string().

Applied to files:

lib/storage/src/content_manager/conversions.rs

📚 Learning: 2025-11-10T14:50:02.722Z

Learnt from: generall
Repo: qdrant/qdrant PR: 7474
File: src/common/health.rs:328-330
Timestamp: 2025-11-10T14:50:02.722Z
Learning: In Qdrant's `ReplicaState`, the `ActiveRead` variant is intentionally considered unhealthy (returns `false` from `is_healthy()`) because it requires a shard transfer operation to finish. This is the correct behavior for health checks.

Applied to files:

lib/collection/src/shards/shard_holder/mod.rs

📚 Learning: 2025-12-22T16:30:28.630Z

Learnt from: coszio
Repo: qdrant/qdrant PR: 7631
File: docs/redoc/master/openapi.json:11774-11781
Timestamp: 2025-12-22T16:30:28.630Z
Learning: Qdrant’s OpenAPI (docs/redoc/master/openapi.json) is generated via schemars, and the project accepts the pattern anyOf: [ { "$ref": ... }, { "nullable": true } ] for nullable references because their autogenerated clients handle it. Prefer treating suggestions to switch to allOf + nullable as optional/non-blocking.

Applied to files:

docs/redoc/master/openapi.json

📚 Learning: 2025-08-23T22:24:44.276Z

Learnt from: generall
Repo: qdrant/qdrant PR: 7100
File: tests/openapi/test_fts.py:255-263
Timestamp: 2025-08-23T22:24:44.276Z
Learning: In Qdrant's OpenAPI schema, the `filter.must` field accepts either a single Condition object or an array of Condition objects, not just arrays. The schema uses `anyOf` to allow both formats.

Applied to files:

docs/redoc/master/openapi.json

🧬 Code graph analysis (4)

lib/collection/src/collection/telemetry.rs (1)

lib/collection/src/telemetry.rs (7)

from (162-183)

from (221-231)

from (235-245)

from (249-261)

from (289-298)

from (332-352)

from (406-437)

lib/storage/src/content_manager/conversions.rs (1)

lib/api/src/grpc/conversions.rs (32)

try_from (97-128)

try_from (426-436)

try_from (456-463)

try_from (468-482)

try_from (487-497)

try_from (502-508)

try_from (514-541)

try_from (546-587)

try_from (593-607)

try_from (612-618)

try_from (623-633)

try_from (638-645)

try_from (651-678)

try_from (684-725)

try_from (751-773)

try_from (912-938)

from (132-147)

from (151-162)

from (166-177)

from (181-188)

from (192-204)

from (208-224)

from (228-240)

from (244-249)

from (253-286)

from (290-295)

from (299-311)

from (315-327)

from (331-342)

from (346-357)

from (361-372)

from (376-402)

lib/collection/src/shards/shard_holder/mod.rs (2)

lib/collection/src/shards/shard_holder/resharding.rs (1)

resharding_state (22-24)

lib/collection/src/collection/resharding.rs (1)

resharding_state (15-22)

lib/collection/src/telemetry.rs (1)

lib/api/src/grpc/conversions.rs (2)

convert_shard_key_from_grpc_opt (87-94)

convert_shard_key_to_grpc (68-77)

⏰ 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). (11)

GitHub Check: test-shard-snapshot-api-s3-minio
GitHub Check: e2e-tests
GitHub Check: Test Python bindings
GitHub Check: test-consensus-compose
GitHub Check: integration-tests
GitHub Check: test-consistency
GitHub Check: lint
GitHub Check: rust-tests (ubuntu-latest)
GitHub Check: rust-tests (windows-latest)
GitHub Check: integration-tests-consensus
GitHub Check: rust-tests (macos-latest)

🔇 Additional comments (48)

docs/redoc/master/openapi.json (2)

11764-11826: TelemetryData.app optional/nullable schema looks consistent with telemetry refactor

Making app non-required and representing it as anyOf [AppBuildTelemetry, { "nullable": true }] aligns with the move to optional app telemetry and matches the established nullable-reference pattern in this OpenAPI file and the schemars-generated output. No changes needed here. Based on learnings, this anyOf + nullable pattern is expected in this repo.

12111-12135: CollectionTelemetry.init_time_ms/config now optional – ensure callers handle null/missing

CollectionTelemetry now only requires id, with init_time_ms nullable and config expressed as anyOf [CollectionConfigTelemetry, { "nullable": true }]. This matches the new internal/grpc telemetry where these fields can be absent, but it does mean previously non-null fields may now be missing/null in generated clients or downstream analytics. Please double-check that any consumers assuming these fields are always present have been updated to treat them as optional.

Based on learnings, the anyOf + nullable shape here is consistent with how nullable references are modeled throughout this OpenAPI.

lib/collection/src/shards/resharding.rs (1)

58-70: LGTM!

The rename from ReshardStage to ReshardingStage is consistent and the new WriteHashRingCommitted variant extends the ordered enum properly. The derives (Ord, PartialOrd) are correctly maintained for stage comparison logic used elsewhere.

src/common/telemetry_ops/mod.rs (1)

4-4: LGTM!

The private conversions module is appropriately scoped for internal telemetry type conversions.

src/common/metrics.rs (1)

131-133: LGTM!

The conditional handling of self.app correctly adapts to the optional AppBuildTelemetry field, gracefully skipping app metrics when not present while preserving the remaining metrics collection flow.

lib/collection/src/operations/types.rs (1)

349-354: LGTM!

The stage field is correctly added with #[serde(skip)] to exclude it from public API serialization while making it available for internal peer telemetry. The documentation clearly indicates its intended use.

lib/collection/src/operations/conversions.rs (3)

1411-1428: LGTM!

The stage: _ pattern correctly ignores the telemetry-only field per the coding guidelines for explicit field ignoring. The inline comment clearly documents why the stage is discarded (only communicated for internal telemetry service).

1439-1446: LGTM!

The reverse conversion from gRPC ReshardingDirection to internal ReshardingDirection uses explicit match arms, which aligns with the coding guidelines. This ensures compile-time errors if new variants are added to the gRPC enum.

1590-1603: LGTM!

The reverse conversion from internal ShardTransferMethod to gRPC type uses explicit match arms for all variants (StreamRecords, Snapshot, WalDelta, ReshardingStreamRecords), ensuring new variants will require explicit handling. As per coding guidelines, this prevents silently missing new enum variants.

lib/collection/src/shards/shard_holder/resharding.rs (3)

18-18: LGTM!

The import correctly reflects the renamed ReshardingStage enum.

627-660: Explicit match patterns correctly handle all ReshardingStage variants.

The match on state.stage explicitly handles:

MigratingPoints | ReadHashRingCommitted (grouped, as both expect resharding hashring)

WriteHashRingCommitted (expects non-resharding hashring)

None (no resharding in progress)

This covers all three ReshardingStage variants exhaustively. As per coding guidelines, this ensures compile-time errors if new variants are added, requiring explicit handling decisions.

662-673: LGTM!

The check_stage function signature correctly uses ReshardingStage and the closure logic is unchanged.

lib/collection/src/collection/telemetry.rs (1)

40-47: LGTM!

The changes correctly wrap init_time_ms and config in Some() to match their updated Option types, and the conditional population of shard_clean_tasks using then_some is idiomatic.

src/common/telemetry_ops/conversions.rs (5)

10-17: LGTM!

The helper function correctly handles the optional role conversion with proper error mapping. Using .transpose() to convert Option<Result<_, _>> to Result<Option<_>, _> is idiomatic.

19-52: LGTM!

Explicit field destructuring is used correctly, and the required consensus_thread_status field is properly validated before conversion. As per coding guidelines, explicit field ignoring is followed.

54-80: LGTM!

The conversion is symmetric with the TryFrom implementation and correctly maps all fields including the optional role.

82-102: Acknowledged: None values for fields not provided in gRPC.

The defaulting of enabled to true and setting unprovided fields to None is intentional per prior discussion. This is acceptable given the internal telemetry service context.

104-126: LGTM!

Explicit field ignoring with : _ is used correctly per coding guidelines, and unwrap_or_default() safely handles the optional peers field.

lib/collection/src/shards/shard_holder/mod.rs (4)

33-33: LGTM!

Import updated to reflect the renamed ReshardingStage enum.

317-319: LGTM!

Stage comparison updated to use ReshardingStage::WriteHashRingCommitted, consistent with the enum rename.

550-557: LGTM!

The stage field is now included in ReshardingInfo, providing visibility into the resharding progress in telemetry data.

597-602: LGTM!

Stage comparison updated to use ReshardingStage::ReadHashRingCommitted, consistent with the enum rename.

lib/collection/src/telemetry.rs (5)

24-28: LGTM!

The init_time_ms and config fields are now correctly declared as Option types with appropriate skip_serializing_if attributes to omit them when None.

301-353: LGTM!

The ShardCleanStatusTelemetry conversions exhaustively match all five variants (Started, Progress, Done, Failed, Cancelled) in both directions with proper error handling for the missing variant case.

355-438: LGTM!

The CollectionTelemetry conversions use explicit field destructuring and ignoring per coding guidelines. Fields not provided by the internal service (init_time_ms, config, shards) are correctly set to None or ignored with clear comments explaining the rationale.

197-218: LGTM!

The conversion correctly handles optional fields including the method and comment transformations. The pattern for filtering empty comments is appropriate.

220-246: The ReshardingStage conversions are correctly exhaustive. Both enums have exactly three variants (MigratingPoints, ReadHashRingCommitted, WriteHashRingCommitted), and both From implementations explicitly match all three variants without using a catch-all arm. No changes needed.

src/common/telemetry.rs (4)

50-52: LGTM!

The app field is now correctly an Option<AppBuildTelemetry> with skip_serializing_if to omit it when None, aligning with the internal telemetry service requirements.

129-133: LGTM!

app is correctly wrapped in Some() to match the updated Option type.

150-185: LGTM!

The conversion correctly transforms grpc::PeerTelemetry to TelemetryData, wrapping collections in CollectionTelemetryEnum::Full. Setting id to empty and app to None is intentional for internal service telemetry as confirmed in past review discussion.

187-223: LGTM!

Explicit field ignoring with : _ follows coding guidelines. The explicit error for CollectionTelemetryEnum::Aggregated is appropriate since this conversion expects details_level >= 2. This provides a clear error message rather than silently failing.

lib/api/src/grpc/proto/telemetry_internal.proto (7)

15-18: LGTM - Response structure follows established patterns.

The nesting of telemetry data into result is consistent with REST API conventions and other gRPC endpoints in the codebase, as previously discussed.

20-30: Good use of reserved fields for schema evolution.

The reserved declarations with comments documenting the original field purposes (id, app, requests, memory, hardware) follow protobuf best practices for backward-compatible schema evolution. This prevents accidental reuse of field numbers.

63-63: Making method optional is appropriate.

This change allows flexibility for cases where the transfer method may not be known or applicable. Ensure downstream conversion code handles the None case gracefully.

113-119: ClusterTelemetry structure is clean.

Reserved fields are properly documented. The peers map using uint64 keys (peer IDs) to PeerInfo values is a sensible design for exposing peer endpoint information.

126-128: Optional role and peer_id fields are appropriate.

Making these optional handles edge cases where consensus state may not be fully initialized or available. This is consistent with the PR's goal of handling optional telemetry data gracefully.

152-154: PeerInfo is minimal but sufficient for current needs.

The message currently only contains uri. If additional peer metadata is needed in the future (e.g., health status, version), the structure is extensible.

77-81: Verify enum value semantics for default case.

In proto3, unset enum fields default to the first value (0). Here MIGRATING_POINTS = 0 will be the default. Confirm this is the intended initial/default stage for resharding operations, or consider adding an explicit UNKNOWN = 0 sentinel if the absence of a stage should be distinguishable.

lib/api/src/grpc/qdrant.rs (5)

12463-12482: New GetPeerTelemetryResponse/PeerTelemetry shape looks consistent

GetPeerTelemetryResponse { result: Option<PeerTelemetry>, time } plus PeerTelemetry { collections, cluster } is a clean, explicit API; tags starting at 3/4 correctly avoid reusing removed fields. No functional issues spotted here.

12503-12527: Optional ShardTransferTelemetry.method is fine; ensure callers handle None

Making method an Option<i32> matches proto3 optional semantics and is appropriate for telemetry, but any conversion/aggregation code must treat None as “unknown/unspecified” rather than assuming a concrete method.

12602-12628: ClusterTelemetry/ClusterStatusTelemetry changes are reasonable but relax invariants

Adding peers: HashMap<u64, PeerInfo> to ClusterTelemetry is a nice structured view of peer metadata.

Making ClusterStatusTelemetry.role and peer_id optional is ABI-safe on the wire but changes invariants; downstream code must be ready for None in both fields and treat it as “status unknown / not yet initialized”.

No structural issues, just ensure all conversions and UI paths handle the newly-optional fields.

12671-12677: PeerInfo message is minimal and consistent

PeerInfo { uri } is straightforward and matches how other peer metadata is represented elsewhere; no issues from the wire-format perspective.

12679-12707: ReshardingStage enum definition and string mappings look correct

Variants, numeric values, and as_str_name/from_str_name mappings match typical proto enum patterns and are internally consistent; nothing to fix here.

lib/storage/src/content_manager/conversions.rs (5)

297-317: LGTM!

Both StateRole conversions use exhaustive matching, which aligns with the coding guidelines and ensures compile-time errors if new variants are added to either enum.

343-343: Verify that treating None status as Stopped is intentional.

When status is None, this defaults to ConsensusThreadStatus::Stopped. This may be intentional for backwards compatibility or default behavior, but could also mask cases where the status field wasn't properly set. Consider whether returning an error or a distinct state would be more appropriate.

348-373: LGTM!

The conversion uses exhaustive matching on all ConsensusThreadStatus variants and correctly converts the DateTime<Utc> to milliseconds timestamp. The implementation is symmetric with the TryFrom implementation above.

68-122: Changes look good overall.

The TryFrom<grpc::CreateCollection> implementation correctly destructures all fields explicitly and handles the various optional configurations appropriately. The error handling and metadata conversion are properly implemented.

124-167: Well-structured configuration mapping.

The strict_mode_from_api function uses explicit field destructuring for all fields and properly maps the gRPC config to the internal StrictModeConfig type. The explicit use of StrictModeMultivectorConfig::from and StrictModeSparseConfig::from on lines 163-164 follows the coding guidelines.

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

lib/api/src/grpc/proto/telemetry_internal.proto (1)
116-116: Remove redundant inline comment.

The inline comment // peers is redundant since the field is already named peers.
🔎 Suggested cleanup
-  map<uint64, PeerInfo> peers = 3; // peers
+  map<uint64, PeerInfo> peers = 3;

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ec20421 and 7c1b6b0.

📒 Files selected for processing (4)

lib/api/src/grpc/proto/telemetry_internal.proto
lib/api/src/grpc/qdrant.rs
lib/collection/src/operations/types.rs
lib/collection/src/shards/transfer/mod.rs

✅ Files skipped from review due to trivial changes (1)

lib/collection/src/shards/transfer/mod.rs

🧰 Additional context used

📓 Path-based instructions (1)

**/*.rs

📄 CodeRabbit inference engine (.github/review-rules.md)

**/*.rs: Prefer explicit SomeType::from(x) over implicit x.into() in Rust
In new code, don't use transmute_from_u8, transmute_to_u8, transmute_from_u8_to_slice, transmute_from_u8_to_mut_slice, transmute_to_u8_slice - use bytemuck or zerocopy crates instead
Prefer explicit exhaustive matches over catch-all _ arm in Rust match expressions, so we don't miss handling new enum variants when they are added later (exceptions: tests and benchmarks, or when sure new variants won't affect logic)
Prefer explicit field ignoring using : _ over using .. in Rust struct destructuring (exceptions: tests and benchmarks, or when sure new fields won't affect logic)

Files:

lib/collection/src/operations/types.rs
lib/api/src/grpc/qdrant.rs

🧠 Learnings (1)

📚 Learning: 2025-09-16T19:14:17.614Z

Learnt from: generall
Repo: qdrant/qdrant PR: 7183
File: lib/api/src/grpc/qdrant.rs:4263-4273
Timestamp: 2025-09-16T19:14:17.614Z
Learning: In qdrant, lib/api/src/grpc/qdrant.rs is auto-generated by prost-build; do not edit it directly. Make changes in lib/api/src/grpc/proto/points.proto (e.g., add [deprecated=true], doc comments, or encoding options), then regenerate the Rust code.

Applied to files:

lib/api/src/grpc/qdrant.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). (11)

GitHub Check: test-consistency
GitHub Check: test-shard-snapshot-api-s3-minio
GitHub Check: integration-tests
GitHub Check: test-consensus-compose
GitHub Check: e2e-tests
GitHub Check: integration-tests-consensus
GitHub Check: rust-tests (ubuntu-latest)
GitHub Check: lint
GitHub Check: rust-tests (macos-latest)
GitHub Check: rust-tests (windows-latest)
GitHub Check: Test Python bindings

🔇 Additional comments (12)

lib/collection/src/operations/types.rs (3)

56-56: LGTM! Import added for new field.

The import of ReshardingStage is correctly added to support the new stage field in ReshardingInfo.

306-306: LGTM! Documentation updated to reflect correct method name.

The comment correctly references ReshardingStreamRecords transfer method.

349-353: All construction sites have been properly updated with the new stage field.

The verification confirms:

TryFrom implementation in telemetry.rs constructs the stage field by converting from grpc::ReshardingStage

Direct construction in shard_holder/mod.rs provides stage from resharding_state.stage

Pattern matching in conversions.rs uses exhaustive destructuring with explicit stage: _, to indicate the field is intentionally excluded from the gRPC conversion

The design is sound: the stage field is properly initialized at all construction sites, and the exhaustive pattern matching ensures compile-time safety if additional fields are added in the future.

lib/api/src/grpc/proto/telemetry_internal.proto (5)

152-161: LGTM!

The new PeerInfo message and expanded StateRole enum look good. The additions are backward compatible and serve clear purposes.

20-30: No actionable changes needed. The PeerTelemetry message correctly uses protobuf's reserved keyword to preserve field numbers (id, app, requests, memory, hardware) for forward compatibility, which is standard practice and intentional design.

63-63: No action needed. The conversion code for ShardTransferTelemetry.method correctly handles the optional field using idiomatic Rust patterns (.map() and .transpose()), with proper error handling. The implementation safely manages the None case without unsafe unwraps.

53-53: The comment correctly references ReshardingStreamRecords as the transfer method name—this matches the actual enum value defined in ShardTransferMethod in collections.proto.

126-128: The conversion code properly handles the optional role and peer_id fields. The TryFrom implementation uses a dedicated helper function for safe enum conversion and .map() for field mapping, while the From implementation correctly converts optional values. Metrics code checks for Some before using peer_id, and role is not directly accessed without None checks. All downstream field accesses are defensive.

lib/api/src/grpc/qdrant.rs (4)

12464-12468: GetPeerTelemetryResponse / PeerTelemetry shape looks consistent with other APIs

GetPeerTelemetryResponse now wraps a single PeerTelemetry result plus time: f64, which matches the pattern used by other responses in this file, and the PeerTelemetry payload groups collection and cluster telemetry cleanly. Just make sure this file is regenerated from the updated telemetry_internal.proto rather than hand-edited, so future codegen doesn’t drop these changes.

Based on learnings, this file is expected to be prost-generated rather than maintained manually.

12505-12523: Optional method in ShardTransferTelemetry requires callers to handle None

Changing method to Option<i32> is wire‑compatible but changes the Rust API: telemetry consumers and conversion code must now handle method == None (e.g. for older peers that don’t send it, or where the method is unknown). Please double‑check that all conversion implementations and any display/logging paths treat None sensibly instead of unconditionally unwrapping. The updated to_shard_id comment referencing ReshardingStreamRecords also looks accurate with the current ShardTransferMethod enum.

12602-12607: ClusterTelemetry.peers + PeerInfo look reasonable and extensible

Adding ClusterTelemetry.peers: HashMap<u64, PeerInfo> keyed by peer ID with PeerInfo { uri } gives a clear, minimal structure for peer metadata and can be extended later with more fields if needed. The field number (tag = 3) avoids conflicts and should be safe for rollout across mixed versions.

Also applies to: 12672-12677

12612-12628: Making role and peer_id optional in ClusterStatusTelemetry changes Rust semantics

role and peer_id moving to Option<i32> / Option<u64> is wire‑compatible with older messages, but any existing Rust code that previously assumed these fields were always present must now handle None (e.g. on telemetry from partially initialized nodes or during upgrades). Please review the new conversion code to ensure it either provides a sensible default when mapping to internal types or propagates None safely without panics.

coderabbitai

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

lib/storage/src/content_manager/conversions.rs (1)

92-101: Use explicit conversions instead of implicit try_into()/into().

Lines 93, 100, and 101 use implicit conversions, which violates the coding guideline. Prefer explicit Type::from() or Type::try_from() forms.

🔎 Proposed fix

                 vectors: match vectors_config.and_then(|config| config.config) {
-                    Some(vector_config) => vector_config.try_into()?,
+                    Some(vector_config) => VectorsConfig::try_from(vector_config)?,
                     // TODO(sparse): sparse or dense vectors config is required
                     None => Default::default(),
                 },
                 sparse_vectors: sparse_vectors_config
                     .map(|v| SparseVectorsConfig::try_from(v).map(|SparseVectorsConfig(x)| x))
                     .transpose()?,
-                hnsw_config: hnsw_config.map(|v| v.into()),
-                wal_config: wal_config.map(|v| v.into()),
+                hnsw_config: hnsw_config.map(HnswConfig::from),
+                wal_config: wal_config.map(WalConfig::from),

As per coding guidelines, explicit conversions improve code clarity and make type transformations more discoverable.

🧹 Nitpick comments (1)

docs/redoc/master/openapi.json (1)

11672-11678: Optional: align to_shard_id doc wording with enum value

The description now mentions the ReshardingStreamRecords transfer method, while the corresponding ShardTransferMethod enum value exposed in the API is resharding_stream_records. If these are meant to be the same concept, consider adjusting the wording to reference the public enum string (or clarify both) so OpenAPI readers don’t see two slightly different names. Update the Rust doc comment rather than this JSON so the change survives regeneration.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7c1b6b0 and 2d89c84.

📒 Files selected for processing (4)

docs/redoc/master/openapi.json
lib/api/src/grpc/proto/telemetry_internal.proto
lib/api/src/grpc/qdrant.rs
lib/storage/src/content_manager/conversions.rs

🧰 Additional context used

📓 Path-based instructions (1)

**/*.rs

📄 CodeRabbit inference engine (.github/review-rules.md)

**/*.rs: Prefer explicit SomeType::from(x) over implicit x.into() in Rust
In new code, don't use transmute_from_u8, transmute_to_u8, transmute_from_u8_to_slice, transmute_from_u8_to_mut_slice, transmute_to_u8_slice - use bytemuck or zerocopy crates instead
Prefer explicit exhaustive matches over catch-all _ arm in Rust match expressions, so we don't miss handling new enum variants when they are added later (exceptions: tests and benchmarks, or when sure new variants won't affect logic)
Prefer explicit field ignoring using : _ over using .. in Rust struct destructuring (exceptions: tests and benchmarks, or when sure new fields won't affect logic)

Files:

lib/storage/src/content_manager/conversions.rs
lib/api/src/grpc/qdrant.rs

🧠 Learnings (16)

📚 Learning: 2025-09-16T19:14:17.614Z

Learnt from: generall
Repo: qdrant/qdrant PR: 7183
File: lib/api/src/grpc/qdrant.rs:4263-4273
Timestamp: 2025-09-16T19:14:17.614Z
Learning: In qdrant, lib/api/src/grpc/qdrant.rs is auto-generated by prost-build; do not edit it directly. Make changes in lib/api/src/grpc/proto/points.proto (e.g., add [deprecated=true], doc comments, or encoding options), then regenerate the Rust code.

Applied to files:

lib/storage/src/content_manager/conversions.rs
lib/api/src/grpc/qdrant.rs

📚 Learning: 2025-11-25T11:56:33.176Z

Learnt from: CR
Repo: qdrant/qdrant PR: 0
File: .github/review-rules.md:0-0
Timestamp: 2025-11-25T11:56:33.176Z
Learning: Applies to **/*.rs : Prefer explicit exhaustive matches over catch-all `_` arm in Rust match expressions, so we don't miss handling new enum variants when they are added later (exceptions: tests and benchmarks, or when sure new variants won't affect logic)

Applied to files:

lib/storage/src/content_manager/conversions.rs

📚 Learning: 2025-11-25T11:56:33.176Z

Learnt from: CR
Repo: qdrant/qdrant PR: 0
File: .github/review-rules.md:0-0
Timestamp: 2025-11-25T11:56:33.176Z
Learning: Applies to **/*.rs : Prefer explicit field ignoring using `: _` over using `..` in Rust struct destructuring (exceptions: tests and benchmarks, or when sure new fields won't affect logic)

Applied to files:

lib/storage/src/content_manager/conversions.rs

📚 Learning: 2025-04-22T23:17:41.734Z

Learnt from: xzfc
Repo: qdrant/qdrant PR: 6245
File: lib/segment/src/index/hnsw_index/point_scorer.rs:116-121
Timestamp: 2025-04-22T23:17:41.734Z
Learning: The method `is_none_or` exists in the Qdrant codebase and compiles correctly, despite not being part of standard Rust's Option type. Code reviews should verify compilation issues before reporting them.

Applied to files:

lib/storage/src/content_manager/conversions.rs

📚 Learning: 2025-11-25T11:56:33.176Z

Learnt from: CR
Repo: qdrant/qdrant PR: 0
File: .github/review-rules.md:0-0
Timestamp: 2025-11-25T11:56:33.176Z
Learning: For Rust code review, checking whether code compiles is out of scope, including: missing imports for `size_of` or `align_of` (included in Rust 1.80+ prelude), methods not implemented for types, different method names across `rand` crate versions (e.g., `random_range` vs `gen_range`), and other compiler errors or warnings

Applied to files:

lib/storage/src/content_manager/conversions.rs

📚 Learning: 2025-08-10T18:30:02.986Z

Learnt from: generall
Repo: qdrant/qdrant PR: 7006
File: lib/collection/src/operations/verification/update.rs:158-174
Timestamp: 2025-08-10T18:30:02.986Z
Learning: In Qdrant's strict mode verification code (lib/collection/src/operations/verification/update.rs), exhaustive pattern matching without `..` is intentionally used for structs like PointsBatch and PointsList. This design pattern ensures compilation fails when new fields are added, forcing developers to explicitly consider how new fields should be handled in the indexed_filter_write method. This provides visibility and compile-time safety for struct evolution.

Applied to files:

lib/storage/src/content_manager/conversions.rs

📚 Learning: 2025-10-08T13:36:02.791Z

Learnt from: coszio
Repo: qdrant/qdrant PR: 7359
File: lib/segment/src/vector_storage/query/feedback_query.rs:111-125
Timestamp: 2025-10-08T13:36:02.791Z
Learning: When reviewing Rust code, do not flag compile-time bugs such as type errors, incorrect destructuring patterns, or syntax issues that would be caught by the Rust compiler. If the code compiles, focus reviews on logic, design patterns, runtime behavior, and semantic correctness instead of syntax-level concerns.

Applied to files:

lib/storage/src/content_manager/conversions.rs

📚 Learning: 2025-10-16T13:00:46.809Z

Learnt from: coszio
Repo: qdrant/qdrant PR: 7368
File: lib/shard/src/query/mod.rs:313-341
Timestamp: 2025-10-16T13:00:46.809Z
Learning: In the Qdrant codebase, for the `Feedback` query variant in `lib/shard/src/query/mod.rs`, validation for sparse vectors should happen down the line rather than early in the `query_enum_from_grpc_raw_query` function. The `Nearest` variant is treated as a special case where early validation with better error messages is acceptable.

Applied to files:

lib/storage/src/content_manager/conversions.rs

📚 Learning: 2025-11-02T19:16:42.527Z

Learnt from: generall
Repo: qdrant/qdrant PR: 7474
File: lib/collection/src/shards/transfer/stream_records.rs:46-55
Timestamp: 2025-11-02T19:16:42.527Z
Learning: In the Qdrant repository, do not flag compilation errors such as partial-move errors, type errors, or any syntax issues that would be caught by the Rust compiler during the build process.

Applied to files:

lib/storage/src/content_manager/conversions.rs

📚 Learning: 2025-08-15T15:56:36.821Z

Learnt from: coszio
Repo: qdrant/qdrant PR: 7065
File: lib/collection/src/operations/universal_query/shard_query.rs:0-0
Timestamp: 2025-08-15T15:56:36.821Z
Learning: Internal gRPC APIs in Qdrant don't require additional runtime validation when proto-level validation is already configured via build.rs range constraints.

Applied to files:

lib/storage/src/content_manager/conversions.rs

📚 Learning: 2025-08-10T18:25:16.206Z

Learnt from: generall
Repo: qdrant/qdrant PR: 7006
File: docs/redoc/master/openapi.json:10626-10634
Timestamp: 2025-08-10T18:25:16.206Z
Learning: In Qdrant, docs/redoc/master/openapi.json is autogenerated from the Rust REST schema definitions (e.g., lib/api/src/rest/schema.rs). To change OpenAPI descriptions, update the Rust struct field doc comments/attributes rather than editing the JSON directly.

Applied to files:

lib/storage/src/content_manager/conversions.rs
docs/redoc/master/openapi.json

📚 Learning: 2025-09-08T08:47:43.795Z

Learnt from: timvisee
Repo: qdrant/qdrant PR: 7188
File: lib/collection/src/update_handler.rs:810-813
Timestamp: 2025-09-08T08:47:43.795Z
Learning: In the Qdrant codebase, CollectionId implements traits that allow &CollectionId to convert Into<String>, so passing &collection_name to functions requiring Into<String> works correctly without needing .clone() or .to_string().

Applied to files:

lib/storage/src/content_manager/conversions.rs

📚 Learning: 2025-08-10T18:26:33.017Z

Learnt from: generall
Repo: qdrant/qdrant PR: 7006
File: docs/redoc/master/openapi.json:10393-10401
Timestamp: 2025-08-10T18:26:33.017Z
Learning: In qdrant, docs/redoc/master/openapi.json is auto-generated. Any description changes must be made in the source (e.g., lib/api/src/rest/schema.rs and/or lib/api/src/grpc/proto/points.proto), not edited directly in the JSON.

Applied to files:

docs/redoc/master/openapi.json

📚 Learning: 2025-08-10T18:26:12.443Z

Learnt from: generall
Repo: qdrant/qdrant PR: 7006
File: docs/redoc/master/openapi.json:13645-13652
Timestamp: 2025-08-10T18:26:12.443Z
Learning: In qdrant, docs/redoc/master/openapi.json is auto-generated from the REST schemas. To change field docs, edit lib/api/src/rest/schema.rs (e.g., add doc comments or #[schemars(description = ...)]). Specifically, UpdateVectors.update_filter lacked a description and should state: "If specified, only update vectors for points that match this filter; points not matching the filter are left unchanged."

Applied to files:

docs/redoc/master/openapi.json

📚 Learning: 2025-12-22T16:30:28.630Z

Learnt from: coszio
Repo: qdrant/qdrant PR: 7631
File: docs/redoc/master/openapi.json:11774-11781
Timestamp: 2025-12-22T16:30:28.630Z
Learning: Qdrant’s OpenAPI (docs/redoc/master/openapi.json) is generated via schemars, and the project accepts the pattern anyOf: [ { "$ref": ... }, { "nullable": true } ] for nullable references because their autogenerated clients handle it. Prefer treating suggestions to switch to allOf + nullable as optional/non-blocking.

Applied to files:

docs/redoc/master/openapi.json

📚 Learning: 2025-08-23T22:24:44.276Z

Learnt from: generall
Repo: qdrant/qdrant PR: 7100
File: tests/openapi/test_fts.py:255-263
Timestamp: 2025-08-23T22:24:44.276Z
Learning: In Qdrant's OpenAPI schema, the `filter.must` field accepts either a single Condition object or an array of Condition objects, not just arrays. The schema uses `anyOf` to allow both formats.

Applied to files:

docs/redoc/master/openapi.json

⏰ 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). (11)

GitHub Check: test-consensus-compose
GitHub Check: test-consistency
GitHub Check: integration-tests-consensus
GitHub Check: e2e-tests
GitHub Check: test-shard-snapshot-api-s3-minio
GitHub Check: integration-tests
GitHub Check: rust-tests (macos-latest)
GitHub Check: rust-tests (windows-latest)
GitHub Check: rust-tests (ubuntu-latest)
GitHub Check: Test Python bindings
GitHub Check: lint

🔇 Additional comments (13)

docs/redoc/master/openapi.json (2)

11773-11781: TelemetryData.app nullability change looks consistent

Making app an optional/nullable field via anyOf aligns the schema with the new optional internal telemetry field and matches the existing nullable‑reference pattern used elsewhere in this OpenAPI. No further changes needed here.

12112-12135: CollectionTelemetry: config / init_time_ms now optional & nullable—check client impact

CollectionTelemetry no longer requires config or init_time_ms, and both fields can now be null/absent. This matches the internal telemetry changes (making these fields optional), but it does change the OpenAPI contract: previously generated clients may have treated these as non‑null required properties.

If external SDKs or integrations were generated against the old schema, please double‑check they tolerate missing/null config and init_time_ms values. Any further tweaks should be done in the Rust schema definitions that feed schemars, not by editing this JSON directly.

lib/storage/src/content_manager/conversions.rs (5)

124-167: LGTM! Explicit conversions and field destructuring.

The function correctly uses explicit field destructuring and explicit conversions throughout (as usize, f64::from(), StrictModeMultivectorConfig::from()).

169-212: LGTM! Follows coding guidelines.

The implementation uses explicit field destructuring and explicit conversion functions throughout.

214-295: LGTM! Clean alias operation conversions.

All implementations use explicit field destructuring and the match statement correctly uses explicit None instead of a catch-all _ arm, as addressed in the previous review.

297-317: LGTM! Exhaustive bidirectional StateRole conversions.

Both conversion directions use exhaustive pattern matches covering all four variants (Follower, Candidate, Leader, PreCandidate), ensuring compile-time safety when new variants are added.

319-373: LGTM! Well-implemented bidirectional ConsensusThreadStatus conversions.

The implementations correctly:

Use exhaustive pattern matches

Handle timestamp conversion between DateTime<Utc> and milliseconds with proper error handling

Default None to Stopped state (reasonable for proto oneofs)

Preserve all error information in StoppedWithErr variant

The timestamp conversion is invertible at millisecond precision, which is appropriate for this use case.

lib/api/src/grpc/proto/telemetry_internal.proto (5)

15-30: LGTM! Clean telemetry response structure.

The refactoring into a nested PeerTelemetry message with result and time fields aligns with the existing API patterns (as noted in past review). The use of reserved fields for future expansion is a good proto design practice.

113-119: LGTM! Peer tracking in cluster telemetry.

The new peers map enables tracking peer information by ID. The reserved fields maintain proto evolution flexibility. The structure is consistent with the new PeerInfo message definition.

152-161: LGTM! Clean message and enum definitions.

PeerInfo provides a simple, extensible structure for peer URIs. The StateRole enum correctly defines all standard Raft consensus roles and aligns with the bidirectional conversions implemented in lib/storage/src/content_manager/conversions.rs.

63-63: The optional method field is intentional and properly handled. The None case has semantic meaning—it directs the system to automatically choose the transfer method. Conversions in lib/collection/src/telemetry.rs correctly handle the optional field using .map(), and the internal ShardTransferInfo explicitly documents this behavior: "Method to transfer shard with. None to choose automatically." All code paths safely handle the Option type.

Likely an incorrect or invalid review comment.

126-128: This change is appropriate. The role and peer_id fields are legitimately optional in ClusterStatusTelemetry since they only exist when the cluster is enabled. The Rust struct correctly models both as Option types, conversions properly handle them, and peer_id is always wrapped in Some() during construction when the cluster status is enabled. A helper method this_peer_id() safely extracts the value when needed.

lib/api/src/grpc/qdrant.rs (1)

12463-12706: Telemetry schema changes look consistent; please verify downstream conversions and remember this file is generated.

GetPeerTelemetryResponse and PeerTelemetry follow the same result + time and nested-telemetry patterns used elsewhere in this file, so the new RPC surface is structurally sound.

Making ShardTransferTelemetry.method, ClusterStatusTelemetry.role, and ClusterStatusTelemetry.peer_id optional is a reasonable way to support partial/older telemetry, but it does require all conversion code and consumers to handle None explicitly rather than assuming defaults.

Adding ClusterTelemetry.peers: HashMap<u64, PeerInfo> aligns with the rest of the API (peer IDs as u64, URIs as String) and gives a clean place to attach per-peer info without changing existing messages.

The new ReshardingStage enum (with MigratingPoints, ReadHashRingCommitted, and WriteHashRingCommitted) is well-formed and consistent with other enums here; just ensure all producers/consumers of resharding telemetry are updated to use the renamed/extended stages.

Given this file is auto-generated by prost-build, any further tweaks should come from the corresponding .proto definitions (e.g. telemetry_internal.proto) and then be regenerated rather than edited here directly. Based on learnings, lib/api/src/grpc/qdrant.rs is treated as generated output.

* implement conversions * clippy * rename `ReshardStage` -> `ReshardingStage` * impl conversions to grpc * upd response in proto * convert TelemetryData * add peer info * upd openapi * add conversion from TelemetryData to PeerTelemetry * review nit * re-style * make `method` optional * change comments to reference `ReshardingStreamRecords` * missing nits

coszio marked this pull request as ready for review November 28, 2025 19:33

coszio force-pushed the telemetry-conversions branch from 11472e0 to 7038300 Compare November 28, 2025 19:39

github-actions Bot mentioned this pull request Nov 28, 2025

Flaky test tests::snapshot_test::test_snapshot_collection_listener #5218

Open

coszio added this to the Cluster Telemetry milestone Dec 2, 2025

coszio mentioned this pull request Dec 2, 2025

[cluster telemetry] Internal telemetry endpoint #7672

Merged

Base automatically changed from grpc-types-for-peer-telemetry to dev December 4, 2025 15:08

This comment was marked as resolved.

Sign in to view

qdrant deleted a comment from coderabbitai Bot Dec 5, 2025

coszio force-pushed the telemetry-conversions branch from dc482ed to 2477dc0 Compare December 8, 2025 19:58

This comment was marked as resolved.

Sign in to view

coszio force-pushed the telemetry-conversions branch from 2477dc0 to d3b27aa Compare December 8, 2025 20:14

This comment was marked as resolved.

Sign in to view

coszio force-pushed the telemetry-conversions branch from 0af693e to a412db6 Compare December 9, 2025 13:55

This comment was marked as resolved.

Sign in to view

coszio requested a review from timvisee December 9, 2025 17:26

coszio force-pushed the telemetry-conversions branch from a412db6 to 94c4271 Compare December 9, 2025 18:00

coderabbitai Bot reviewed Dec 9, 2025

View reviewed changes

Comment thread docs/redoc/master/openapi.json

Comment thread docs/redoc/master/openapi.json

qdrant deleted a comment from coderabbitai Bot Dec 15, 2025

timvisee reviewed Dec 15, 2025

View reviewed changes

This comment was marked as resolved.

Sign in to view

coszio requested a review from timvisee December 16, 2025 13:17

coszio added 9 commits December 16, 2025 19:29

implement conversions

2676b3c

clippy

e9f448f

rename ReshardStage -> ReshardingStage

a8abe95

impl conversions to grpc

c5a820d

upd response in proto

10dde81

convert TelemetryData

63fd397

add peer info

2a5161e

upd openapi

4404ae9

add conversion from TelemetryData to PeerTelemetry

f4a6b56

review nit

063c5c3

coszio force-pushed the telemetry-conversions branch from 756b4e8 to 063c5c3 Compare December 16, 2025 23:07

This comment was marked as resolved.

Sign in to view

re-style

af70f2d

qdrant deleted a comment from coderabbitai Bot Dec 22, 2025

timvisee reviewed Dec 22, 2025

View reviewed changes

Comment thread lib/collection/src/telemetry.rs Outdated

make method optional

ec20421

coderabbitai Bot reviewed Dec 22, 2025

View reviewed changes

Comment thread lib/storage/src/content_manager/conversions.rs

change comments to reference ReshardingStreamRecords

7c1b6b0

coderabbitai Bot reviewed Dec 22, 2025

View reviewed changes

missing nits

2d89c84

coderabbitai Bot reviewed Dec 23, 2025

View reviewed changes

timvisee approved these changes Dec 23, 2025

View reviewed changes

coszio merged commit 64c806d into dev Dec 23, 2025
15 checks passed

coszio deleted the telemetry-conversions branch December 23, 2025 17:00

coderabbitai Bot mentioned this pull request Dec 29, 2025

[cluster telemetry] Include peers versions #7848

Merged

Conversation

coszio commented Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Tasks

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

timvisee Dec 15, 2025

Choose a reason for hiding this comment

Uh oh!

coszio Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

timvisee Dec 15, 2025

Choose a reason for hiding this comment

Uh oh!

coszio Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

timvisee left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot commented Dec 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Estimated code review effort

Possibly related PRs

Suggested reviewers

Pre-merge checks and finishing touches

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coszio commented Nov 27, 2025 •

edited

Loading

coderabbitai Bot commented Dec 22, 2025 •

edited

Loading