Skip to content

Conversation

@crazyuploader
Copy link
Contributor

Closes: #2049

@crazyuploader crazyuploader self-assigned this Nov 20, 2025
@crazyuploader crazyuploader added the documentation Improvements or additions to documentation label Nov 20, 2025
@vercel
Copy link

vercel bot commented Nov 20, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
signoz-web Ready Ready Preview Comment Nov 21, 2025 11:00am

@crazyuploader
Copy link
Contributor Author

@claude /review

@claude
Copy link
Contributor

claude bot commented Nov 21, 2025

Claude finished @crazyuploader's task —— View job


Review Complete

I've reviewed the Rust OpenTelemetry instrumentation documentation update. This is a significant improvement that updates the code from OpenTelemetry 0.18.0 to 0.27.0, modernizing the API usage throughout. However, there are several issues that need to be addressed.

Summary of Findings

Critical Issues:

  1. Missing doc_type in frontmatter - Required by CONTRIBUTING.md for all docs
  2. Missing tonic dependency - Required in 4 sections where .with_tonic() is used but tonic isn't listed in dependencies
  3. Incorrect error handling - All tracer_provider.shutdown() calls use let _ = which silently ignores errors
  4. Windows OTel Collector path issue - Instructions create a .env file but don't configure dotenv to load it

Minor Issues:

  1. Title capitalization - "Opentelemetry" should be "OpenTelemetry" (capital T)
  2. Environment variable naming - Good fix changing SIGNOZ_ACCESS_TOKEN to SIGNOZ_INGESTION_KEY, but consider adding migration note

Technical Accuracy Verification

I verified the updated code against official OpenTelemetry sources:

Version 0.27.0 is valid - Released November 11, 2024 (source)

API usage is correct - The new builder pattern with SpanExporter::builder().with_tonic() is the correct API for 0.27.0 (source)

TracerProvider pattern is correct - Using TracerProvider::builder() and global::set_tracer_provider() matches 0.27.0 API

⚠️ Shutdown handling needs improvement - tracer_provider.shutdown() returns a Result that should be handled (source)

Checklist Against CONTRIBUTING.md

Based on the Docs PR Checklist:

  • Frontmatter includes date, id, title, description, tags
  • Frontmatter includes doc_type ❌ (missing)
  • SEO: keywords in title, description, first paragraph ✓
  • Commands explain what they do ✓
  • Code snippets are annotated ✓
  • Code examples have all required dependencies listed ❌ (tonic missing in 4 places)
  • Cross-links existing SigNoz docs ✓
  • Links use absolute URLs ✓

Detailed Issues

I've provided inline comments on specific lines with:

  • Exact line numbers and code suggestions
  • Source URLs for verification
  • Reasoning for each change

The inline comments cover:

  • Line 5: Title capitalization
  • Line 7: Missing doc_type
  • Line 42: Tonic dependency explanation
  • Line 108: Environment variable naming note
  • Line 173, 278, 388, 555, 660, 786: Shutdown error handling (6 occurrences)
  • Line 197, 307, 579, 707: Missing tonic dependency (4 occurrences)
  • Line 663: Windows .env file workflow issue

Recommendation

The documentation update is valuable and improves the code significantly. After addressing the critical issues (especially the missing dependencies and doc_type), this will be ready to merge.


@crazyuploader crazyuploader marked this pull request as ready for review November 21, 2025 10:52
@claude
Copy link
Contributor

claude bot commented Nov 21, 2025

Claude finished @crazyuploader's task —— View job


Review Update: Previous Issues Addressed ✅

I've reviewed the current state of the PR and I'm pleased to report that the author has addressed all of the critical issues from the previous review:

✅ Issues Fixed

  1. doc_type added - Line 7 now includes doc_type: howto as required by CONTRIBUTING.md
  2. Title capitalization corrected - Line 5 now properly uses "Rust OpenTelemetry Instrumentation"
  3. Shutdown error handling fixed - All 6 occurrences now properly use .expect("Failed to shutdown tracer provider") instead of silently ignoring errors with let _ =
  4. Windows .env file workflow fixed - Lines 675-678 now correctly use set commands for Windows environment variables instead of creating a non-functional .env file

⚠️ Correction to Previous Review

The previous review incorrectly flagged missing tonic dependency in 4 sections (OTel Collector paths). After verifying against the official OpenTelemetry Rust documentation, I can confirm that those sections are actually correct as-is.

Why tonic is NOT needed in those sections:

According to docs.rs/opentelemetry-otlp/0.27.0, when you enable the grpc-tonic feature (which all sections do), tonic 0.12.3 is included transitively as an optional dependency. You only need to add tonic as a direct dependency if you're using tonic's API directly (like tonic::metadata::MetadataMap or tonic::transport::ClientTlsConfig).

Current dependency usage is correct:

  • Direct to Cloud sections (VM & Windows): Include tonic = { version = "0.12" } because they use tonic::metadata::MetadataMap and tonic::transport::ClientTlsConfig
  • OTel Collector sections (VM, K8s, Windows) and Self-Host: Do NOT include tonic because they only use .with_tonic() from the opentelemetry-otlp API, not tonic's API directly

Source: https://docs.rs/opentelemetry-otlp/0.27.0/

Final Status

All previously identified issues have been properly resolved. The documentation is now technically accurate and follows the CONTRIBUTING.md guidelines. The PR is ready to merge from a technical review perspective.


@makeavish
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update Rust Docs with updated gRPC instructions

2 participants