-
Notifications
You must be signed in to change notification settings - Fork 3.2k
fix(azure-monitor-opentelemetry): configure_azure_monitor args were overwritten #44131
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?
Conversation
|
Thank you for your contribution @temporaer! We will review the pull request and get back to you soon. |
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 addresses a bug where user-provided configuration values passed to configure_azure_monitor() were being overwritten by default values during the configuration process. The fix changes three instances of direct dictionary assignment to use setdefault(), ensuring that user-provided values are preserved.
Key changes:
- Changed error-handling code paths to use
setdefault()instead of direct assignment forSAMPLING_TRACES_PER_SECOND_ARGandSAMPLING_RATIO_ARG - Applied the same pattern to the default case when no sampler type is specified
- This ensures user-provided kwargs take precedence over both environment variables (in error cases) and hard-coded defaults
| e, | ||
| ) | ||
| configurations[SAMPLING_TRACES_PER_SECOND_ARG] = default_value | ||
| configurations.setdefault(SAMPLING_TRACES_PER_SECOND_ARG, default_value) |
Copilot
AI
Nov 21, 2025
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.
The error message logged here states "Defaulting to %s" (where %s is default_value), but with the change to use setdefault(), the actual behavior may not match this message. If the user provided a value via configure_azure_monitor() kwargs, that user-provided value will be preserved rather than defaulting to default_value. The log message should be updated to reflect this, or the value that will actually be used should be logged.
Consider logging the actual value that will be used after calling setdefault():
configurations.setdefault(SAMPLING_TRACES_PER_SECOND_ARG, default_value)
_logger.error(
"Value of %s must be a positive number for traces per second. Using value: %s (exception: %s)",
OTEL_TRACES_SAMPLER_ARG,
configurations[SAMPLING_TRACES_PER_SECOND_ARG],
e,
)| e, | ||
| ) | ||
| configurations[SAMPLING_RATIO_ARG] = default_value | ||
| configurations.setdefault(SAMPLING_RATIO_ARG, default_value) |
Copilot
AI
Nov 21, 2025
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.
The error message logged here states "Defaulting to %s" (where %s is default_value), but with the change to use setdefault(), the actual behavior may not match this message. If the user provided a value via configure_azure_monitor() kwargs, that user-provided value will be preserved rather than defaulting to default_value. The log message should be updated to reflect this, or the value that will actually be used should be logged.
Consider logging the actual value that will be used after calling setdefault():
configurations.setdefault(SAMPLING_RATIO_ARG, default_value)
_logger.error(
"Value of %s must be a float. Using value: %s (exception: %s)",
OTEL_TRACES_SAMPLER_ARG,
configurations[SAMPLING_RATIO_ARG],
e,
)| _logger.error( # pylint: disable=C | ||
| "Invalid argument for the sampler to be used for tracing. " |
Copilot
AI
Nov 21, 2025
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.
The error message is misleading. It says "Defaulting to %s: %s" and substitutes environment variable names (OTEL_TRACES_SAMPLER and OTEL_TRACES_SAMPLER_ARG) rather than the actual value that will be used.
Consider revising to show the actual value being used:
_logger.error(
"Invalid value '%s' for %s. Supported values are '%s' and '%s'. Using sampling_ratio=%s",
sampler_type,
OTEL_TRACES_SAMPLER,
RATE_LIMITED_SAMPLER,
FIXED_PERCENTAGE_SAMPLER,
configurations.get(SAMPLING_RATIO_ARG, default_value),
)|
@temporaer This behavior is by design, we check the sampler type provided by the user and if the arguments are not valid only then we assign the default values. Is there a case you can provide where this logic is being violated. Also, for the |
Description
After calling
configure_azure_monitor, all kwargs are saved in aconfigurationsobject. This configurations object is then amended with default values, before the actual configuration begins.During this configuration, some user-provided values get overwritten by hard-coded defaults.
This PR fixes at least some obvious cases of this happening.
Cc'ing @rads-1996, who introduced the breaking change.
All SDK Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines