Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Handle boolean, integer and float values in Configuration #662

Merged
merged 3 commits into from
May 20, 2020
Merged

Handle boolean, integer and float values in Configuration #662

merged 3 commits into from
May 20, 2020

Conversation

ocelotl
Copy link
Contributor

@ocelotl ocelotl commented May 8, 2020

Fixes #647

@ocelotl ocelotl added the api Affects the API package. label May 8, 2020
@ocelotl ocelotl requested review from mauriciovasquezbernal and a team May 8, 2020 16:54
@ocelotl ocelotl self-assigned this May 8, 2020
@ocelotl ocelotl added enhancement New feature or request needs reviewers PRs with this label are ready for review and needs people to review to move forward. labels May 8, 2020
@ocelotl ocelotl changed the title Convert "True" or "False" to its boolean values Handle boolean, integer and float values in Configuration May 8, 2020
mauriciovasquezbernal
mauriciovasquezbernal approved these changes May 8, 2020
View reviewed changes
Copy link
Member

@mauriciovasquezbernal mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

lzchen
lzchen approved these changes May 18, 2020
View reviewed changes
Copy link
Contributor

@lzchen lzchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

toumorokoshi
toumorokoshi approved these changes May 19, 2020
View reviewed changes
Copy link
Member

@toumorokoshi toumorokoshi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice! I do have a thought on accepting more than just capital T/F for bool, but that could be relaxed later on. Thanks!

opentelemetry-api/src/opentelemetry/configuration/__init__.py
@@ -77,6 +77,11 @@
"default_meter_provider" (this is not actually necessary since the
OpenTelemetry API provided providers are the default ones used if no
configuration is found in the environment variables).

Configuration values that are exactly ``"True"`` or ``"False"`` will be
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typically lowercase "true" or "false" work here as well. Thoughts on accepting those?

@toumorokoshi toumorokoshi merged commit 9e58b8a into open-telemetry:master May 20, 2020
srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020
@mayurkale22
feat: add pg-pool to default list of instrumented plugins (open-telem…
854256a 
…etry#662)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mauriciovasquezbernal mauriciovasquezbernal mauriciovasquezbernal approved these changes

@lzchen lzchen lzchen approved these changes

@toumorokoshi toumorokoshi toumorokoshi approved these changes

Assignees

@ocelotl ocelotl

Labels
api Affects the API package. enhancement New feature or request needs reviewers PRs with this label are ready for review and needs people to review to move forward.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make Configuration object return boolean values
4 participants
@ocelotl @toumorokoshi @lzchen @mauriciovasquezbernal