Add several configuration related fixes#583
Add several configuration related fixes#583toumorokoshi merged 3 commits intoopen-telemetry:masterfrom
Conversation
| if match is not None: | ||
|
|
||
| key = match.group(1).lower() | ||
| key = match.group(1) |
There was a problem hiding this comment.
Why to keep the casing in the python attributes? I think the previous lower() was better.
There was a problem hiding this comment.
Environment variables are case sensitive, so this needs to be too.
There was a problem hiding this comment.
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.
mauriciovasquezbernal
left a comment
mauriciovasquezbernal
left a comment
There was a problem hiding this comment.
I still have some doubts of the possible problems of having a case sensitive configuration. I don't want to delay it, approving.
| key = match.group(1).lower() | ||
| key = match.group(1) | ||
|
|
||
| setattr(Configuration, "_{}".format(key), value) |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
Just curious, why must the first character after OPENTELEMETRY_PYTHON_ not be a number?
There was a problem hiding this comment.
Those env variables are mapped to Python attributes, it's not possible to have a Python attribute name starting by a number :)
lzchen
left a comment
lzchen
left a comment
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
| 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.
toumorokoshi
left a comment
toumorokoshi
left a comment
There was a problem hiding this comment.
Cool! Don't see any blockers, a couple nitpicks.
| if match is not None: | ||
|
|
||
| key = match.group(1).lower() | ||
| key = match.group(1) |
There was a problem hiding this comment.
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.
| # 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 |
There was a problem hiding this comment.
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.
4 participants
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
Nonewhen queried instead of raising anAttributeError. This makes it easier to use the configuration object because the user won't have to check for the existence of the attribute beforehand:instead of