Skip to content

Conversation

@Nemo157
Copy link
Contributor

@Nemo157 Nemo157 commented Nov 4, 2025

Add custom Display, FromStr, Serialize, and Deserialize implementations for ProcessId, ThreadId, and SpanContext types.

These provide a consistent hex-encoded string format with colon separators for composite IDs (process:thread for ThreadId, process:span for SpanContext). This makes telemetry ids more readable and provides a unified format for logging and serialization.

@github-actions
Copy link

github-actions bot commented Nov 4, 2025

@codecov
Copy link

codecov bot commented Nov 4, 2025

Base automatically changed from wim/push-qrznukrqspqw to main November 5, 2025 12:54
@Nemo157 Nemo157 force-pushed the wim/push-pxywosxxsluy branch from ed54e1e to c0ffbb7 Compare November 5, 2025 13:08
@Nemo157 Nemo157 marked this pull request as ready for review November 5, 2025 13:09
@claude
Copy link

claude bot commented Nov 5, 2025

Change Summary

This PR adds custom Display, FromStr, Serialize, and Deserialize implementations for telemetry ID types (ProcessId, ThreadId, SpanContext). The implementation provides a unified hex-encoded string format with colon separators for composite IDs (e.g., process:thread for ThreadId, process:span for SpanContext), improving readability in logs and serialization. The UI components have been updated to use these new Display implementations instead of Debug formatting.


Issues Found

Found 3 issues that should be addressed:

🔴 Critical

  1. Variable naming issue in SpanContext::deserialize (veecle-telemetry/src/id.rs:299-300) - Variable named thread should be span since it represents a SpanId, not a ThreadId. This appears to be copy-pasted from the ThreadId implementation.

🟡 Important

  1. Inconsistent error message (veecle-telemetry/src/id.rs:292) - Error message says "byte 33" but the check is bytes[32]. Should use consistent indexing terminology.

  2. Same error message issue (veecle-telemetry/src/protocol.rs:202) - Same inconsistency in the ThreadId deserialization.

All issues have inline comments with suggested fixes.

Add custom `Display`, `FromStr`, `Serialize`, and `Deserialize`
implementations for `ProcessId`, `ThreadId`, `ExecutionId`, and
`SpanContext` types.

These provide a consistent hex-encoded string format with colon
separators for composite IDs (`process:thread` for `ExecutionId`,
`process:span` for `SpanContext`). This makes telemetry ids more
readable and provides a unified format for logging and serialization.

Signed-off-by: Wim Looman <[email protected]>
@Nemo157 Nemo157 force-pushed the wim/push-pxywosxxsluy branch from c0ffbb7 to 22ef8b0 Compare November 5, 2025 13:20
@Nemo157 Nemo157 requested a review from HectorMRC November 6, 2025 08:15
Copy link

@HectorMRC HectorMRC left a comment

Choose a reason for hiding this comment

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

LGTM

@Nemo157 Nemo157 added this pull request to the merge queue Nov 6, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 6, 2025
@Nemo157 Nemo157 added this pull request to the merge queue Nov 6, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 6, 2025
@Nemo157 Nemo157 added this pull request to the merge queue Nov 6, 2025
Merged via the queue into main with commit 07de023 Nov 6, 2025
26 checks passed
@Nemo157 Nemo157 deleted the wim/push-pxywosxxsluy branch November 6, 2025 12:19
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.

4 participants