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.

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

Add several configuration related fixes #583

Merged
merged 3 commits into from
Apr 27, 2020

Conversation

ocelotl
Copy link
Contributor

@ocelotl ocelotl commented Apr 15, 2020

Fixes #582

This makes the attributes of the configuration object case sensitive in order to match better the environment variables that are too.

This makes the attributes of the configuration object accept any legal Python variable name (Configuration().some_2_attribute) is valid now.

This makes non-defined attributes return None when queried instead of raising an AttributeError. This makes it easier to use the configuration object because the user won't have to check for the existence of the attribute beforehand:

if Configuration().some_attribute == "value":
    # do stuff

instead of

if hasattr(Configuration(), "some_attribute") and Configuration().some_attribute == "value":
    # do stuff

@ocelotl ocelotl added the bug Something isn't working label Apr 15, 2020
@ocelotl ocelotl requested a review from a team April 15, 2020 07:43
@ocelotl ocelotl self-assigned this Apr 15, 2020

if match is not None:

key = match.group(1).lower()
key = match.group(1)

Choose a reason for hiding this comment

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

Why to keep the casing in the python attributes? I think the previous lower() was better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Environment variables are case sensitive, so this needs to be too.

Copy link
Member

Choose a reason for hiding this comment

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

maybe add a comment as such? I think it might also be good to call out which environments have case sensitive environment variables. I believe windows is case insensitive.

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.

I still have some doubts of the possible problems of having a case sensitive configuration. I don't want to delay it, approving.


if match is not None:

key = match.group(1).lower()
key = match.group(1)

setattr(Configuration, "_{}".format(key), value)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need a private attribute AND a property attribute?

character.
values from environment variables prefixed with ``OPENTELEMETRY_PYTHON_`` whose
characters are only alphanumeric characters and unserscores, except for the
first character after ``OPENTELEMETRY_PYTHON_`` which must not be a number.
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, why must the first character after OPENTELEMETRY_PYTHON_ not be a number?

Choose a reason for hiding this comment

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

Those env variables are mapped to Python attributes, it's not possible to have a Python attribute name starting by a number :)

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! Some non-blocking questions.


The values stored in the environment variables can be found in an instance of
``opentelemetry.configuration.Configuration``. This class can be instantiated
freely because instantiating it returns a singleton.
freely because instantiating it returns always the same object.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
freely because instantiating it returns always the same object.
freely because instantiating it always returns the same object.

That said, why the rephrasing here? singleton is a fairly common software term.

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.

Cool! Don't see any blockers, a couple nitpicks.


if match is not None:

key = match.group(1).lower()
key = match.group(1)
Copy link
Member

Choose a reason for hiding this comment

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

maybe add a comment as such? I think it might also be good to call out which environments have case sensitive environment variables. I believe windows is case insensitive.

@@ -21,6 +21,9 @@

class TestConfiguration(TestCase):
def setUp(self):
# This is added here to force a reload of the whole Configuration
# class, resetting its internal attributes so that each tests starts
# with a clean class.
from opentelemetry.configuration import Configuration # type: ignore
Copy link
Member

Choose a reason for hiding this comment

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

is this true? typically python will cache found modules in sys.modules, and as a result you won't re-instantiate a Configuration object.

This is probably working today because this is the first time the Configuration module is loaded into memory, and as such it honors your variables.

I might consider using importlib.reload instead: https://docs.python.org/3/library/importlib.html. This needs to be paired with the local import you are doing here, or else the test module itself (test_configuration) will hold onto the old copy of Configuration.

@toumorokoshi toumorokoshi merged commit 5d675ee into open-telemetry:master Apr 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix several configuration issues
4 participants