fix(azure-monitor-opentelemetry): configure_azure_monitor args were overwritten #44131
Conversation
temporaer
requested review from
JacksonWeber,
Karlie-777,
MSNev,
hectorhdzg,
jeremydvoss,
lzchen and
rads-1996
as code owners
temporaer
changed the title
github-actions
bot
added
Community Contribution
|
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.
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) |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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. " |
There was a problem hiding this comment.
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 |
2 participants
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