Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Aug 4, 2025

The logs feature in opentelemetry-sdk/Cargo.toml was unnecessarily including serde_json as a dependency. After thorough analysis of the codebase, I confirmed that:

  • No logs-related code in src/logs/ uses serde_json
  • The serde_json dependency is correctly included in the jaeger_remote_sampler feature where it's actually needed for JSON parsing in the Jaeger remote sampler implementation
  • The logs feature works perfectly without this dependency

However, removing serde_json from the logs feature caused build failures in opentelemetry-proto when the with-serde feature was enabled, as it relied on the transitive dependency from the SDK. To resolve this:

Changes made:

  1. opentelemetry-sdk/Cargo.toml: Removed serde_json from the logs feature

    # Before
    logs = ["opentelemetry/logs", "serde_json"]
    
    # After  
    logs = ["opentelemetry/logs"]
  2. opentelemetry-proto/Cargo.toml: Added serde_json as an optional dependency when with-serde is enabled

    # Before
    with-serde = ["serde", "const-hex", "base64"]
    
    # After
    with-serde = ["serde", "const-hex", "base64", "serde_json"]

This change reduces unnecessary dependencies for users who only need logging functionality without the Jaeger remote sampler feature, while ensuring that opentelemetry-proto has access to serde_json when JSON serialization/deserialization is needed.

Fixes #3104.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI changed the title [WIP] OTel Logs SDK - should not need serde_json feature Remove unnecessary serde_json dependency from logs feature Aug 4, 2025
Copilot AI requested a review from cijothomas August 4, 2025 17:25
Copilot finished work on behalf of cijothomas August 4, 2025 17:25
@cijothomas cijothomas marked this pull request as ready for review August 4, 2025 17:27
@cijothomas cijothomas requested a review from a team as a code owner August 4, 2025 17:27
@cijothomas cijothomas changed the title Remove unnecessary serde_json dependency from logs feature fix: Remove unnecessary serde_json dependency from logs feature Aug 4, 2025
@codecov
Copy link

codecov bot commented Aug 4, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.5%. Comparing base (e0dd566) to head (bead802).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@          Coverage Diff          @@
##            main   #3114   +/-   ##
=====================================
  Coverage   80.5%   80.5%           
=====================================
  Files        126     126           
  Lines      22218   22218           
=====================================
+ Hits       17889   17890    +1     
+ Misses      4329    4328    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@cijothomas
Copy link
Member

@copilot the build fails as opentelemetry-proto relied on serde_json from the sdk/logs! Modify opentelemetry-proto to bring serde_json if with-serde feature is enabled in the opentelemetry-proto

Copy link
Contributor Author

Copilot AI commented Aug 4, 2025

@copilot the build fails as opentelemetry-proto relied on serde_json from the sdk/logs! Modify opentelemetry-proto to bring serde_json if with-serde feature is enabled in the opentelemetry-proto

I've fixed the build issue by adding serde_json as an optional dependency to opentelemetry-proto when the with-serde feature is enabled. The changes ensure that the build no longer fails when opentelemetry-proto requires serde_json for JSON serialization/deserialization tests and functionality. Build verified with various feature combinations. (commit bead802)

Copilot AI changed the title fix: Remove unnecessary serde_json dependency from logs feature fix: Remove unnecessary serde_json dependency from logs feature and ensure opentelemetry-proto compatibility Aug 4, 2025
Copilot AI requested a review from cijothomas August 4, 2025 19:25
Copilot finished work on behalf of cijothomas August 4, 2025 19:25
@cijothomas cijothomas merged commit 0640933 into main Aug 5, 2025
28 checks passed
@cijothomas cijothomas deleted the copilot/fix-3104-2 branch August 5, 2025 00:16
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.

OTel Logs SDK - should not need serde_json feature

3 participants