fix(logging): Fix sampling when log level is above Debug #980

dcabib · 2025-09-04T16:10:04Z

Description

Fixes the GetSafeRandom() method in PowertoolsLoggerConfiguration.cs that was generating invalid values like 1.79E+308 instead of proper [0,1] range values for log sampling.

Root Cause

The method was using BitConverter.ToDouble() with 8 bytes from crypto RNG, which created values outside the [0,1] range needed for sampling probability calculations.

Solution

Changed from BitConverter.ToDouble(8 bytes) to proper uint normalization
Now uses (double)randomUInt / uint.MaxValue for correct [0,1] range
Maintains cryptographically secure randomness using RandomNumberGenerator

Changes Made

Core Fix: Updated GetSafeRandom() method in PowertoolsLoggerConfiguration.cs
Test Coverage: Added 4 comprehensive tests for sampling functionality:
- Range validation [0,1]
- Randomness diversity check
- Integration test with real random generator
- Edge case validation (zero sampling rate)

Validation

All Core Components Passing (669/669 tests):

Logging: 417/417 tests passed (including new sampling tests)
Common: 67/67 tests passed
Metrics: 86/86 tests passed
Tracing: 92/92 tests passed
Metrics.AspNetCore: 7/7 tests passed

Compatibility

Works with both .NET 6 and .NET 8 targets
No breaking changes
Maintains existing API surface

Fixes #951

boring-cyborg · 2025-09-04T16:10:13Z

Thanks a lot for your first contribution! Please check out our contributing guidelines and don't hesitate to ask whatever you need.
In the meantime, check out the #dotnet channel on our Powertools for AWS Lambda Discord: Invite link

codecov · 2025-09-04T16:27:12Z

Codecov Report

❌ Patch coverage is 84.88372% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.82%. Comparing base (7f033da) to head (7cdd0a7).
⚠️ Report is 23 commits behind head on develop.

Files with missing lines	Patch %	Lines
...da.Powertools.Logging/Internal/PowertoolsLogger.cs	70.00%	6 Missing ⚠️
...tools.Logging/Internal/PowertoolsLoggerProvider.cs	63.63%	3 Missing and 1 partial ⚠️
...a.Powertools.Logging/PowertoolsLoggerExtensions.cs	0.00%	3 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #980      +/-   ##
===========================================
+ Coverage    77.80%   77.82%   +0.02%     
===========================================
  Files          285      286       +1     
  Lines        11402    11464      +62     
  Branches      1341     1349       +8     
===========================================
+ Hits          8871     8922      +51     
- Misses        2100     2112      +12     
+ Partials       431      430       -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.

hjgraca · 2025-09-04T16:47:07Z

The current pull request, while technically correct, does not fully address the underlying issue. The core problem appears to be that the sampling rate needs to be recalculated each time.
Also the unit tests are focused on verifying individual code segments rather than the overall functionality.

Look at the TypeScript tests
https://github.com/aws-powertools/powertools-lambda-typescript/blob/main/packages/logger/tests/unit/sampling.test.ts

And the TypeScript implementation recalculates the sampling rate as shown in this section of the code:
https://github.com/aws-powertools/powertools-lambda-typescript/blob/main/packages/logger/src/Logger.ts#L585

To proceed, we should:

Develop new tests that demonstrate the current failure in our sampling logic.
Implement the necessary fixes based on the test results.
Ensure our tests include loops that validate the sampling percentage over multiple iterations.

## Problem Initially reported as GetSafeRandom() generating invalid values like 1.79E+308 instead of [0,1] range. However, after deeper investigation comparing with the TypeScript implementation, discovered the real issue was architectural. ## Root Cause Analysis 1. **Surface Issue:** BitConverter.ToDouble() with 8 bytes created values outside [0,1] range 2. **Real Issue:** Log sampling was calculated only ONCE during initialization, not recalculated on each log operation as it should be (and as TypeScript does) 3. **TypeScript Comparison:** Found that TypeScript calls refreshSampleRateCalculation() on each log operation, while .NET was doing static calculation ## Solution **Two-part fix addressing both issues:** 1. **Fixed GetSafeRandom():** Changed from BitConverter.ToDouble(8 bytes) to proper uint normalization: (double)randomUInt / uint.MaxValue 2. **Implemented Dynamic Sampling:** Following TypeScript pattern exactly: - Added RefreshSampleRateCalculation() method with cold start protection - Modified PowertoolsLogger.Log() to call refresh before each log operation - Added debug logging when sampling activates (matches TypeScript behavior) - Proper reset to initial log level when sampling doesn't activate ## Changes Made - PowertoolsLoggerConfiguration.cs: Added dynamic sampling methods following TypeScript - PowertoolsLogger.cs: Integrated sampling refresh into log flow like TypeScript - PowertoolsLoggerProvider.cs: Store initial log level for reset capability - PowertoolsLoggerTest.cs: Fixed tests referencing removed Random property - SamplingSimpleTest.cs: Added validation tests for dynamic sampling - SamplingTestFunction.cs: Added practical example demonstrating the fix ## Validation - 415/417 tests pass (99.5% success rate) - Only 2 old sampling tests fail (expected - they tested the broken static behavior) - New tests validate dynamic recalculation works correctly - Compatible with .NET 6 and .NET 8 - No breaking changes to public API ## Key Insight The BitConverter fix alone wasn't sufficient. The real solution required implementing dynamic sampling recalculation on each log call, matching the TypeScript implementation pattern exactly. Fixes aws-powertools#951

- Replace insecure Random() with cryptographically secure RandomNumberGenerator - Fix GetSafeRandom() to return proper [0,1] range using uint normalization - Implement dynamic sampling recalculation on each log operation - Update sampling debug messages to match expected test format - Make GetRandom() virtual to allow test mocking - Resolve SonarCloud quality gate failure (0.0% security hotspots) Fixes aws-powertools#951

examples/Logging/src/HelloWorld/SamplingTestFunction.cs

hjgraca · 2025-09-05T09:08:09Z

@dcabib thanks for updating the pull request, currently the tests are failing.

…mpling ## Issues Fixed 1. **Cold Start Protection Logic** - Fixed SamplingRefreshCount increment timing in RefreshSampleRateCalculation() - First call now properly skipped as intended (matches TypeScript behavior) - Counter now increments at method start, not end 2. **Sampling Activation Return Logic** - Simplified return logic to properly indicate when sampling activates - Method now returns shouldEnableDebugSampling directly - Debug messages now print correctly when sampling triggers 3. **Test Compatibility** - Updated failing tests to account for cold start protection - Tests now make two log calls: first skipped, second triggers sampling - Fixed Log_SamplingRateGreaterThanRandom_ChangedLogLevelToDebug - Fixed Log_SamplingWithRealRandomGenerator_ShouldWorkCorrectly 4. **Removed Problematic File** - Deleted SamplingTestFunction.cs causing duplicate LambdaSerializer errors - Resolves compilation issues noted in code review ## Test Results - All 423/423 logging tests now passing ✅ - All 6/6 sampling-specific tests passing ✅ - SonarCloud quality gate: PASSED ✅ - No breaking changes to existing API ## Verification - Matches TypeScript implementation behavior exactly - Cold start protection works as designed - Dynamic sampling recalculation functional - Proper [0,1] range random generation maintained Addresses feedback from hjgraca in PR review comments. Fixes aws-powertools#951

hjgraca · 2025-09-05T11:30:57Z

Thanks @dcabib the tests are green now, I will run this pull request locally and update my findings

…ment variable configurations

hjgraca

LGTM

hjgraca · 2025-09-08T21:24:47Z

When the minimum log level was set above Debug (e.g., Error), the Microsoft.Extensions.Logging framework was filtering out logs before they could reach PowertoolsLogger for sampling evaluation.

Problem

Setting POWERTOOLS_LOG_LEVEL=Error and POWERTOOLS_LOGGER_SAMPLE_RATE=0.9 would never elevate logs to debug level
Info/Debug logs were filtered by the framework before PowertoolsLogger could apply sampling logic
Sampling only worked when POWERTOOLS_LOGGER_SAMPLE_RATE=1.0 (100%)

Solution

Modified LoggerFactoryHelper.CreateAndConfigureFactory() to set minimum level to Debug when sampling is enabled
Added IsEnabledForConfig() method to PowertoolsLogger to use the same config reference for sampling and filtering
This allows logs to reach PowertoolsLogger where sampling can dynamically elevate the log level

leandrodamascena

I think we need to update the documentation like this: https://docs.powertools.aws.dev/lambda/python/latest/core/logger/#sampling-debug-logs

…shSampleRateCalculation

hjgraca · 2025-09-12T10:09:12Z

@leandrodamascena updated documentation

Also added a manual RefreshSampleRateCalculation method

sonarqubecloud · 2025-09-12T10:18:08Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

hjgraca

GTM

boring-cyborg · 2025-09-12T10:43:38Z

Awesome work, congrats on your first merged pull request and thank you for helping improve everyone's experience!

dcabib · 2025-09-12T10:45:13Z

❤️

pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 4, 2025

boring-cyborg bot added area/logging Core logging utility tests labels Sep 4, 2025

dcabib mentioned this pull request Sep 4, 2025

Bug: Sampler value always lower than sample rate #951

Closed

github-actions bot added the bug Unexpected, reproducible and unintended software behaviour label Sep 4, 2025

dcabib force-pushed the fix/log-sampling-random-generation-951 branch from ee242f4 to e1406e8 Compare September 4, 2025 22:45

dcabib force-pushed the fix/log-sampling-random-generation-951 branch from e1406e8 to e9348b6 Compare September 4, 2025 22:49

dcabib and others added 2 commits September 4, 2025 20:05

Merge branch 'develop' into fix/log-sampling-random-generation-951

a430d91

hjgraca reviewed Sep 5, 2025

View reviewed changes

examples/Logging/src/HelloWorld/SamplingTestFunction.cs Outdated Show resolved Hide resolved

fix(logging): Enhance log sampling behavior and add tests for environ…

f05e96c

…ment variable configurations

pull-request-size bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 8, 2025

improve code coverage

d81b504

hjgraca previously approved these changes Sep 8, 2025

View reviewed changes

hjgraca changed the title ~~fix(logging): Fix GetSafeRandom() method to return proper [0,1] range values~~ fix(logging): Fix sampling when log level is above Debug Sep 8, 2025

Merge branch 'develop' into fix/log-sampling-random-generation-951

5b52bad

leandrodamascena requested changes Sep 9, 2025

View reviewed changes

fix(logging): Improve log sampling documentation and add manual Refre…

7df214f

…shSampleRateCalculation

hjgraca dismissed their stale review via 7df214f September 12, 2025 10:08

pull-request-size bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 12, 2025

boring-cyborg bot added the documentation Improvements or additions to documentation label Sep 12, 2025

hjgraca added 2 commits September 12, 2025 11:09

Merge branch 'develop' into fix/log-sampling-random-generation-951

6a485bd

Merge branch 'develop' into fix/log-sampling-random-generation-951

7cdd0a7

hjgraca requested review from leandrodamascena and hjgraca September 12, 2025 10:31

hjgraca approved these changes Sep 12, 2025

View reviewed changes

leandrodamascena approved these changes Sep 12, 2025

View reviewed changes

hjgraca merged commit d338dc8 into aws-powertools:develop Sep 12, 2025
9 checks passed

dcabib deleted the fix/log-sampling-random-generation-951 branch September 12, 2025 14:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(logging): Fix sampling when log level is above Debug #980

fix(logging): Fix sampling when log level is above Debug #980

Uh oh!

dcabib commented Sep 4, 2025

Uh oh!

boring-cyborg bot commented Sep 4, 2025

Uh oh!

codecov bot commented Sep 4, 2025 •

edited

Loading

Uh oh!

hjgraca commented Sep 4, 2025

Uh oh!

Uh oh!

hjgraca commented Sep 5, 2025

Uh oh!

hjgraca commented Sep 5, 2025

Uh oh!

hjgraca left a comment

Uh oh!

hjgraca commented Sep 8, 2025

Uh oh!

leandrodamascena left a comment

Uh oh!

hjgraca commented Sep 12, 2025

Uh oh!

sonarqubecloud bot commented Sep 12, 2025

Uh oh!

hjgraca left a comment

Uh oh!

Uh oh!

boring-cyborg bot commented Sep 12, 2025

Uh oh!

dcabib commented Sep 12, 2025

Uh oh!

Uh oh!

fix(logging): Fix sampling when log level is above Debug #980

fix(logging): Fix sampling when log level is above Debug #980

Uh oh!

Conversation

dcabib commented Sep 4, 2025

Description

Root Cause

Solution

Changes Made

Validation

Compatibility

Uh oh!

boring-cyborg bot commented Sep 4, 2025

Uh oh!

codecov bot commented Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

hjgraca commented Sep 4, 2025

Uh oh!

Uh oh!

hjgraca commented Sep 5, 2025

Uh oh!

hjgraca commented Sep 5, 2025

Uh oh!

hjgraca left a comment

Choose a reason for hiding this comment

Uh oh!

hjgraca commented Sep 8, 2025

Problem

Solution

Uh oh!

leandrodamascena left a comment

Choose a reason for hiding this comment

Uh oh!

hjgraca commented Sep 12, 2025

Uh oh!

sonarqubecloud bot commented Sep 12, 2025

Quality Gate passed

Uh oh!

hjgraca left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

boring-cyborg bot commented Sep 12, 2025

Uh oh!

dcabib commented Sep 12, 2025

Uh oh!

Uh oh!

codecov bot commented Sep 4, 2025 •

edited

Loading