fix(eap): Don't allow a sampling factor of 0 to happen #7685
+40
−216
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…item queries When trace_filters are present in EndpointTimeSeries and EndpointTraceItemTable requests, adds ability to skip sampling on the outer query while the inner query (getting trace IDs) still uses sampling. This is controlled by the runtime config `cross_item_queries_no_sample_outer`: - When enabled (set to 1) and trace_filters are present: - Inner query uses sampling tier (queries downsampled storage) - Outer query skips sampling (queries full storage) - When disabled (default, 0): both queries use the same sampling tier Benefits: - Improves accuracy by querying full data in outer query - Maintains performance with sampled trace ID lookup in inner query - Safe rollout via runtime config Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Removed CROSS_ITEM_SAMPLING_IMPLEMENTATION.md file - Removed @pytest.mark.clickhouse_db (redundant with @pytest.mark.eap) - Fixed imports: wraps is from functools, not unittest.mock - Added proper type annotations for test helper functions - Simplified test assertions to match actual behavior - Tests now verify that: - When enabled: queries execute successfully with trace_filters - When disabled: all queries use the same storage tier
- Refactor tests to use Mock objects for routing_decision - Add track_storage_selections() helper function to track which storages are used - Mock RPCEndpoint.__before_execute to control routing behavior - Verify inner query uses downsampled storage (TIER_8) - Verify outer query uses full storage (EAP_ITEMS) when feature is enabled - Add @pytest.mark.redis_db decorator for state.set_config() usage - Configure mock routing_decision with all required attributes Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Move track_storage_selections() to test_utils.py to avoid duplication - Add create_mock_routing_decision() helper to centralize mock creation - Update both test files to use shared helpers - Reduce code duplication and improve maintainability Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Replace fully mocked RoutingDecision with real object - Only mock the strategy component as recommended - Create proper RoutingContext with Timer and request message - Update helper to accept in_msg parameter for proper construction Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Member
Author
|
Note: I am expecting CI to fail the first time because there are tests on the query side that test filtering out zero valued sampling factors. Once I see these tests fail I will remove them because they now test an impossible case |
phacops
approved these changes
Jan 29, 2026
Since the fix ensures a minimum sampling factor of 1e-9, zero-valued sampling factors are no longer possible. The tests that checked for filtering out zero sampling factors are now obsolete and have been removed from both time series and trace item table endpoints. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
MeredithAnya
approved these changes
Jan 29, 2026
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If the client and server sample rate were too small, they would result in zero stored sampling factors, which messes up extrapolation calculations by introducing divide by 0 errors. Make sure that we always store the minimum sampling factor (1e-9) always.
I triple checked the calculation changes but this would be very disastrous to get wrong so as a reviewer please check it again.