Skip to content

Conversation

hannahhaering
Copy link
Contributor

@hannahhaering hannahhaering commented Aug 18, 2025

Fixes: #6451

Some tests failed on machines using another localization (decimal separator for numbers). I noticed this running the tests locally on a German Windows 11 machine. I tried to fix this by using culture specific test data and making the parsing of config values culture invariant.

Changes

  • The TracerProviderSdk now parses a double value from configuration (TraceIdRatio) using InvariantCulture instead of the CurrentCulture.
  • Some tests tried to override the current culture before making assertions. This did not work because the string was already generated at this point of time. Also, this approach can lead to race conditions when running multiple tests in parallel, so I removed it.

Merge requirement checklist

  • CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
  • Unit tests added/updated
  • Appropriate CHANGELOG.md files updated for non-trivial changes
  • Changes in public API reviewed (if applicable)

Copy link

linux-foundation-easycla bot commented Aug 18, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

@github-actions github-actions bot added the pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package label Aug 18, 2025
@hannahhaering hannahhaering marked this pull request as ready for review August 18, 2025 19:18
@hannahhaering hannahhaering requested a review from a team as a code owner August 18, 2025 19:18
@Kielek Kielek changed the title Fix test localization Fix parsing configuration for TraceIdRatioBasedSampler on non standard locales + fix localization related test Aug 25, 2025
Copy link
Contributor

@Kielek Kielek left a comment

Choose a reason for hiding this comment

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

LGTM, you can consider CHANGELOG entry to cover this parsing fix.

Copy link

codecov bot commented Aug 25, 2025

Codecov Report

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

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #6444   +/-   ##
=======================================
  Coverage   86.68%   86.68%           
=======================================
  Files         258      258           
  Lines       11878    11878           
=======================================
  Hits        10296    10296           
  Misses       1582     1582           
Flag Coverage Δ
unittests-Project-Experimental 86.56% <100.00%> (+0.20%) ⬆️
unittests-Project-Stable 86.29% <100.00%> (-0.26%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/OpenTelemetry/Trace/TracerProviderSdk.cs 99.36% <100.00%> (ø)

... and 6 files with indirect coverage changes

Co-authored-by: Fabrizio Ferri-Benedetti <[email protected]>
@Kielek Kielek requested a review from martincostello August 26, 2025 07:22
Copy link
Contributor

github-actions bot commented Sep 4, 2025

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or pushing will instruct the bot to automatically remove the label. This bot runs once per day.

@github-actions github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Sep 4, 2025
@Kielek Kielek removed the Stale Issues and pull requests which have been flagged for closing due to inactivity label Sep 4, 2025
@rajkumar-rangaraj rajkumar-rangaraj merged commit 493032d into open-telemetry:main Sep 10, 2025
47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug] OTEL_TRACES_SAMPLER_ARG not parsed invariantly
5 participants