Skip to content

Fix trace flags type from i32 to u32 in Geneva exporter #356

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented Jul 8, 2025

This PR fixes the trace flags field type in the Geneva exporter's OTLP encoder to use u32 instead of i32, aligning with the OpenTelemetry protobuf specification.

Changes Made

  1. Updated field type definition: Changed BondDataType::BT_INT32 to BondDataType::BT_UINT32 for trace flags in the determine_fields() method
  2. Updated value serialization: Changed log.flags as i32 to log.flags as u32 in the write_row_data() method
  3. Added test coverage: Added test_trace_flags_uint32_type() to verify the fix works correctly

Background

The issue was identified during code review where it was noted that trace flags should be unsigned 32-bit integers according to the OpenTelemetry protobuf specification, but the Geneva exporter was incorrectly treating them as signed 32-bit integers.

Testing

  • All existing tests continue to pass
  • New test specifically validates that trace flags fields use the correct BT_UINT32 type
  • Code compiles successfully with no warnings

The changes are minimal and surgical, affecting only the specific type handling for trace flags without impacting any other functionality.

Fixes #355.


💡 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.

Copy link

linux-foundation-easycla bot commented Jul 8, 2025

CLA Missing ID CLA Not Signed

@Copilot Copilot AI changed the title [WIP] [Geneva Exporter] trace flags should be u32, not i32 as implemented. Fix trace flags type from i32 to u32 in Geneva exporter Jul 8, 2025
@Copilot Copilot AI requested a review from lalitb July 8, 2025 22:17
Copilot finished work on behalf of lalitb July 8, 2025 22:18
@lalitb
Copy link
Member

lalitb commented Jul 8, 2025

Unit-test is not required. also check the lint.

Copy link

codecov bot commented Jul 8, 2025

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 49.5%. Comparing base (106d201) to head (14172ad).

Files with missing lines Patch % Lines
...eneva-uploader/src/payload_encoder/otlp_encoder.rs 0.0% 2 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff          @@
##            main    #356   +/-   ##
=====================================
  Coverage   49.5%   49.5%           
=====================================
  Files         67      67           
  Lines       8909    8909           
=====================================
  Hits        4413    4413           
  Misses      4496    4496           

☔ 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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

- Removed test_trace_flags_uint32_type() as requested by maintainer
- Fixed unnecessary cast warning by removing "as u32" cast since log.flags is already u32
- All linting checks now pass

Co-authored-by: lalitb <[email protected]>
Copy link
Contributor Author

Copilot AI commented Jul 8, 2025

Unit-test is not required. also check the lint.

Removed the unit test and fixed the linting issue (unnecessary cast warning). All linting checks now pass. Changes in commit 14172ad.

Copilot finished work on behalf of lalitb July 8, 2025 23:33
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.

[Geneva Exporter] trace flags should be u32, not i32 as implemented.
2 participants