8000 [SDK] Ensure TraceId is portable on big-endian architectures by strophy · Pull Request #3543 · open-telemetry/opentelemetry-cpp · GitHub
[go: up one dir, main page]
Skip to content

[SDK] Ensure TraceId is portable on big-endian architectures #3543

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jul 16, 2025
Merged

[SDK] Ensure TraceId is portable on big-endian architectures #3543

merged 3 commits into from
Jul 16, 2025

Conversation

strophy
Copy link
Contributor
@strophy strophy commented Jul 16, 2025

Fixes #1221

Changes

While trying to enable opentelemetry-cpp for s390x on Alpine Linux, the following test was failing:

The following tests FAILED:
	283 - trace.TraceIdRatioBasedSampler.ShouldSampleWithoutContext (Failed)

This PR fixes architecture-dependent behavior in TraceIdRatioBasedSampler by ensuring the trace ID is always interpreted as a big-endian integer for sampling decisions, as required for cross-platform and cross-language consistency in OpenTelemetry. The previous implementation used memcpy, which resulted in host-endian interpretation and inconsistent sampling results on big-endian architectures (e.g., s390x).

The test case is also updated to construct the trace ID buffer in big-endian order, matching the intended numeric value for sampling.

With this change, the build and all tests pass on native (no Docker) s390x and ppc64le, so #1221 can probably be closed.

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added (test logic changed)
  • Changes in public API reviewed

@strophy strophy requested a review from a team as a code owner July 16, 2025 10:19
@netlify Netlify
Copy link
netlify bot commented Jul 16, 2025 •
edited
Loading

✅ Deploy Preview for opentelemetry-cpp-api-docs canceled.

Name Link
🔨 Latest commit 6c02f81
🔍 Latest deploy log https://app.netlify.com/projects/opentelemetry-cpp-api-docs/deploys/687805469e091a0008aa62cf

@marcalff
Copy link
Member

Thanks for the fix.

Please fix the clang-format failure, patch below from the CI logs:

diff --git a/sdk/src/trace/samplers/trace_id_ratio.cc b/sdk/src/trace/samplers/trace_id_ratio.cc
index 0a12c3b..1ed6e6b 100644
--- a/sdk/src/trace/samplers/trace_id_ratio.cc
+++ b/sdk/src/trace/samplers/trace_id_ratio.cc
@@ -58,7 +58,7 @@ uint64_t CalculateThresholdFromBuffer(const trace_api::TraceId &trace_id) noexce
 
   // Always interpret as big-endian
   const uint8_t *data = trace_id.Id().data();
-  uint64_t res = 0;
+  uint64_t res        = 0;
   for (int i = 0; i < 8; ++i)
   {
     res = (res << 8) | data[i];

@codecov Codecov
Copy link
codecov bot commented Jul 16, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.99%. Comparing base (8608773) to head (6c02f81).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3543      +/-   ##
==========================================
+ Coverage   89.98%   89.99%   +0.01%     
==========================================
  Files         219      219              
  Lines        7044     7048       +4     
==========================================
+ Hits         6338     6342       +4     
  Misses        706      706
Files with missing lines Coverage Δ
sdk/src/trace/samplers/trace_id_ratio.cc 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

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

@strophy strophy force-pushed the s390x-endianness-traceid-fix branch from 0f78911 to 9377a5d Compare July 16, 2025 13:19
marcalff
marcalff approved these changes Jul 16, 2025
View reviewed changes
Copy link
Member
@marcalff marcalff left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the fix.

@lalitb lalitb requested a review from Copilot July 16, 2025 17:23
Copilot
Copilot AI reviewed Jul 16, 2025
View reviewed changes
Copy link
@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR ensures consistent big-endian interpretation of TraceId values for sampling on all architectures and updates the unit test to match the new behavior.

  • CalculateThresholdFromBuffer now manually constructs the uint64_t from the first 8 bytes in big-endian order.
  • The test buffer in trace_id_ratio_sampler_test.cc was reordered to big-endian format.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
sdk/src/trace/samplers/trace_id_ratio.cc Replaces memcpy with a byte-by-byte shift loop to always interpret the first 8 bytes as big-endian
sdk/test/trace/trace_id_ratio_sampler_test.cc Updates the hardcoded buffer so its significant byte appears first, matching big-endian ordering
Comments suppressed due to low confidence (1)

sdk/test/trace/trace_id_ratio_sampler_test.cc:102

  • Add complementary test cases for boundary values (e.g., all zeros and all 0xFF) to verify behavior at the lower and upper extremes of the TraceId range.
  constexpr uint8_t buf[] = {0x80, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0};

@marcalff
Copy link
Member

include-what-you-use failure in CI:

Warning: include-what-you-use reported diagnostics:

/home/runner/work/opentelemetry-cpp/opentelemetry-cpp/sdk/src/trace/samplers/trace_id_ratio.cc should add these lines:
#include "opentelemetry/nostd/span.h"                         // for span

/home/runner/work/opentelemetry-cpp/opentelemetry-cpp/sdk/src/trace/samplers/trace_id_ratio.cc should remove these lines:
- #include <cstring>  // lines 7-7

Patch to fix it:

diff --git a/sdk/src/trace/samplers/trace_id_ratio.cc b/sdk/src/trace/samplers/trace_id_ratio.cc
index fcb074ff..2a19eb70 100644
--- a/sdk/src/trace/samplers/trace_id_ratio.cc
+++ b/sdk/src/trace/samplers/trace_id_ratio.cc
@@ -4,11 +4,11 @@
 
 #include <cmath>
 #include <cstdint>
-#include <cstring>
 #include <map>
 #include <memory>
 #include <string>
 
+#include "opentelemetry/nostd/span.h"
 #include "opentelemetry/nostd/string_view.h"
 #include "opentelemetry/sdk/trace/sampler.h"
 #include "opentelemetry/sdk/trace/samplers/trace_id_ratio.h"

@marcalff
Update trace_id_ratio.cc
6c02f81 
include-what-you-use
@marcalff marcalff merged commit b7b0278 into open-telemetry:main Jul 16, 2025
70 checks passed
@strophy strophy deleted the s390x-endianness-traceid-fix branch July 17, 2025 08:06
@BrewTestBot BrewTestBot mentioned this pull request Sep 26, 2025
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lalitb lalitb lalitb approved these changes

Copilot code review Copilot Copilot left review comments

@marcalff marcalff marcalff approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Enable ppc64le, s390x architectures on opentelemetry-cpp

3 participants

@strophy @marcalff @lalitb
0