-
Notifications
You must be signed in to change notification settings - Fork 651
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
Fix get exporter names condition #1707
Conversation
This issue will only happen if |
@@ -52,7 +52,8 @@ def _get_exporter_names() -> Sequence[str]: | |||
|
|||
if ( | |||
trace_exporters is not None | |||
or trace_exporters.lower().strip() != "none" | |||
and trace_exporters != "" |
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.
Can we just do if trace_exporters and trace_exporters.lower().strip() != "none"
?
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.
That's right. 👍
Here, the blank means the following 2 things.
|
CHANGELOG.md
Outdated
@@ -68,6 +68,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
([#1695](https://github.com/open-telemetry/opentelemetry-python/pull/1695)) | |||
- Change Jaeger exporters to obtain service.name from span | |||
([#1703](https://github.com/open-telemetry/opentelemetry-python/pull/1703)) | |||
- Fixed a parse error on no `OTEL_TRACES_EXPORTER` |
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.
can you change the description to be focused on the behavior of the user?
In this case, I would expect something like Fixed an unset OTEL_TRACES_EXPORTER resulting in an error
.
This way a reader can identify whether their behavior is fixed or not.
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.
minor nit on changelog description, all else looks good!
I updated the code, thanks! |
@dtaniwaki |
I think it was a logical oversight. Probably should have been the "and" operator rather than or. If it's none, the first check is false so the OR statement continues, executing the second statement. Here's what the logic does in a similar example:
|
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.
LGTM, thanks!
Description
With the current condition,
_get_exporter_names
always fails with noOTEL_TRACES_EXPORTER
.Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Configurator().configure()
w/ and w/oOTEL_TRACES_EXPORTER
.Does This PR Require a Contrib Repo Change?
Checklist: