Skip to content

Add several configuration related fixes #583

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

Merged
merged 3 commits into from
Apr 27, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 12 additions & 10 deletions opentelemetry-api/src/opentelemetry/configuration/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,27 +19,27 @@
Simple configuration manager

This is a configuration manager for OpenTelemetry. It reads configuration
values from environment variables prefixed with
``OPENTELEMETRY_PYTHON_`` whose characters are only all caps and underscores.
The first character after ``OPENTELEMETRY_PYTHON_`` must be an uppercase
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 :)


For example, these environment variables will be read:

1. ``OPENTELEMETRY_PYTHON_SOMETHING``
2. ``OPENTELEMETRY_PYTHON_SOMETHING_ELSE_``
3. ``OPENTELEMETRY_PYTHON_SOMETHING_ELSE_AND__ELSE``
4. ``OPENTELEMETRY_PYTHON_SOMETHING_ELSE_AND_else``
4. ``OPENTELEMETRY_PYTHON_SOMETHING_ELSE_AND_else2``

These won't:

1. ``OPENTELEMETRY_PYTH_SOMETHING``
2. ``OPENTELEMETRY_PYTHON_something``
3. ``OPENTELEMETRY_PYTHON_SOMETHING_2_AND__ELSE``
4. ``OPENTELEMETRY_PYTHON_SOMETHING_%_ELSE``
2. ``OPENTELEMETRY_PYTHON_2_SOMETHING_AND__ELSE``
3. ``OPENTELEMETRY_PYTHON_SOMETHING_%_ELSE``

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.


For example, if the environment variable
``OPENTELEMETRY_PYTHON_METER_PROVIDER`` value is ``my_meter_provider``, then
Expand Down Expand Up @@ -93,11 +93,13 @@ def __new__(cls) -> "Configuration":

for key, value in environ.items():

match = fullmatch("OPENTELEMETRY_PYTHON_([A-Z][A-Z_]*)", key)
match = fullmatch(
r"OPENTELEMETRY_PYTHON_([A-Za-z_][\w_]*)", key
)

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.


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?

setattr(
Expand Down
23 changes: 18 additions & 5 deletions opentelemetry-api/tests/configuration/test_configuration.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.


def tearDown(self):
Expand All @@ -35,27 +38,37 @@ def test_singleton(self):
{
"OPENTELEMETRY_PYTHON_METER_PROVIDER": "meter_provider",
"OPENTELEMETRY_PYTHON_TRACER_PROVIDER": "tracer_provider",
"OPENTELEMETRY_PYTHON_OThER": "other",
"OPENTELEMETRY_PYTHON_OTHER_7": "other_7",
"OPENTELEMETRY_PTHON_TRACEX_PROVIDER": "tracex_provider",
},
)
def test_environment_variables(self): # type: ignore
self.assertEqual(
Configuration().meter_provider, "meter_provider"
Configuration().METER_PROVIDER, "meter_provider"
) # pylint: disable=no-member
self.assertEqual(
Configuration().tracer_provider, "tracer_provider"
Configuration().TRACER_PROVIDER, "tracer_provider"
) # pylint: disable=no-member
self.assertEqual(
Configuration().OThER, "other"
) # pylint: disable=no-member
self.assertEqual(
Configuration().OTHER_7, "other_7"
) # pylint: disable=no-member
self.assertIsNone(Configuration().TRACEX_PROVIDER)

@patch.dict(
"os.environ", # type: ignore
{"OPENTELEMETRY_PYTHON_TRACER_PROVIDER": "tracer_provider"},
)
def test_property(self):
with self.assertRaises(AttributeError):
Configuration().tracer_provider = "new_tracer_provider"
Configuration().TRACER_PROVIDER = "new_tracer_provider"

def test_slots(self):
with self.assertRaises(AttributeError):
Configuration().xyz = "xyz" # pylint: disable=assigning-non-slot
Configuration().XYZ = "xyz" # pylint: disable=assigning-non-slot

def test_getattr(self):
Configuration().xyz is None
self.assertIsNone(Configuration().XYZ)