-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Added changes for rate limited sampler (azure-distro changes) #41976
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
base: main
Are you sure you want to change the base?
Added changes for rate limited sampler (azure-distro changes) #41976
Conversation
There was a problem hiding this 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 extends the azure-distro to let users choose between a fixed‐percentage and a rate‐limited traces sampler via OTEL environment variables. Key changes include:
- Parsing
OTEL_TRACES_SAMPLER
(sampler type) andOTEL_TRACES_SAMPLER_ARG
(its argument) in_get_configurations
. - Adding new constants (
RATE_LIMITED_SAMPLER
,FIXED_PERCENTAGE_SAMPLER
,SAMPLING_TRACES_PER_SECOND_ARG
) and updatingazure/monitor/opentelemetry/_constants.py
. - Updating
_configure.py
to selectRateLimitedSampler
whensampling_traces_per_second
is set.
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
.../tests/utils/test_configurations.py | Replaced SAMPLING_RATIO_ENV_VAR , added rate‐limited and fixed tests |
.../azure/monitor/opentelemetry/_utils/configurations.py | Updated sampling defaults to branch on sampler type and argument |
.../azure/monitor/opentelemetry/_constants.py | Introduced new constants for sampler types and trace‐per‐second arg |
.../azure/monitor/opentelemetry/_configure.py | Added logic to wire up RateLimitedSampler when appropriate |
...-exporter/tests/trace/test_rate_limited_sampling.py | New comprehensive tests for RateLimitedSampler |
...-exporter/azure/.../_utils.py | Added _get_djb2_sample_score and rounding helper |
...-exporter/azure/.../_rate_limited_sampling.py | Implemented RateLimitedSamplingPercentage and RateLimitedSampler |
Comments suppressed due to low confidence (2)
sdk/monitor/azure-monitor-opentelemetry/tests/utils/test_configurations.py:178
- This test sets an invalid sampler arg (
"Half"
) but does not assert thatsampling_ratio
falls back to the default. Consider adding an assertion such asself.assertEqual(configurations["sampling_ratio"], 1.0)
to verify fallback behavior.
@patch.dict(
sdk/monitor/azure-monitor-opentelemetry-exporter/azure/monitor/opentelemetry/exporter/export/trace/_utils.py:330
- No tests cover
_round_down_to_nearest
or edge cases of_get_djb2_sample_score
. Adding unit tests would improve confidence in these utility functions.
def _get_djb2_sample_score(trace_id_hex: str) -> float:
sdk/monitor/azure-monitor-opentelemetry/azure/monitor/opentelemetry/_utils/configurations.py
Outdated
Show resolved
Hide resolved
sdk/monitor/azure-monitor-opentelemetry/azure/monitor/opentelemetry/_configure.py
Outdated
Show resolved
Hide resolved
sdk/monitor/azure-monitor-opentelemetry/tests/utils/test_configurations.py
Outdated
Show resolved
Hide resolved
sdk/monitor/azure-monitor-opentelemetry/azure/monitor/opentelemetry/_utils/configurations.py
Outdated
Show resolved
Hide resolved
d3c8014
to
de65c50
Compare
…ted-sampler-distro
…ted-sampler-distro
@@ -3,6 +3,8 @@ | |||
## 1.6.14 (Unreleased) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should bump to 1.7.0 because of new features
sdk/monitor/azure-monitor-opentelemetry/azure/monitor/opentelemetry/_constants.py
Outdated
Show resolved
Hide resolved
sdk/monitor/azure-monitor-opentelemetry/azure/monitor/opentelemetry/_utils/configurations.py
Outdated
Show resolved
Hide resolved
…b.com/rads-1996/azure-sdk-for-python into rads-1996/rate-limited-sampler-distro
…into rads-1996/rate-limited-sampler-distro
Description
Adding support for RateLimitedSampler, inspired by Java Application Insights sampler
https://github.com/microsoft/ApplicationInsights-Java/blob/main/agent/agent-tooling/src/main/java/com/microsoft/applicationinsights/agent/internal/sampling/RateLimitedSamplingPercentage.java
This PR contains changes related to the configurations in the azure-distro only. The changes related to implementation of the rate limited sampler are in the azure-exporter in the PR - #41954.
All SDK Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines