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

Make the configuration object universal #563

Merged
merged 11 commits into from
Apr 13, 2020

Conversation

ocelotl
Copy link
Contributor

@ocelotl ocelotl commented Apr 7, 2020

The configuration object used to provide only configuration for the meter and tracer providers. Now it can be used to load any configuration value stored in an environment variable that starts with OPENTELEMETRY_PYTHON_ whose characters match with [A-Z_]. All this is explained with greater detail in the documentation. The documentation also includes a section that gathers and explains all the current environment variables that are meaningful for OpenTelemetry Python. In this way, the end user can have them all listed in one single place. If in the future, more environment variables are used, then they should be added there and documented accordingly.

Fixes #515

@ocelotl ocelotl requested a review from a team April 7, 2020 21:35
@ocelotl ocelotl self-assigned this Apr 7, 2020
@ocelotl ocelotl added api Affects the API package. doc Documentation-related enhancement New feature or request labels Apr 7, 2020
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.

Looks good! I think there's still some rough edges around using environment variables exclusively to drive providers, but maybe we can talk through it more another time?

Also I might look at pydantic: 3.6+ support only, but it does the env variable -> config conversion you have written by hand: https://pydantic-docs.helpmanual.io/usage/settings/

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.

This looks great! I was looking for a way to configure something like blacklisted host names for flask applications and seems like this would be the way to go.

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.

Generally speaking looks good to me.

I'm wondering if we should relax the restrictions on the names, I think a variable called OPENTELEMETRY_PTYHON_MY_VALUE_2 should be valid.

def test_property(self):
with self.assertRaises(AttributeError):
Configuration().tracer_provider = "new_tracer_provider"

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

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

Choose a reason for hiding this comment

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

Is it missing the assertion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Iep, 😅 fixing...

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

Choose a reason for hiding this comment

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

I think this sentence is not needed as the sentence before already implies it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.


1. ``OPENTELEMETRY_PYTH_SOMETHING``
2. ``OPENTELEMETRY_PYTHON_something``
3. ``OPENTELEMETRY_PYTHON_SOMETHING_2_AND__ELSE``

Choose a reason for hiding this comment

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

Is there any specific reason to not support this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, any valid variable name is supported now.


def tearDown(self):
Configuration._instance = None # pylint: disable=protected-access
from opentelemetry.configuration import Configuration # type: ignore

Choose a reason for hiding this comment

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

How is this supposed to work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a comment.

"os.environ", # type: ignore
{
"OPENTELEMETRY_PYTHON_METER_PROVIDER": "meter_provider",
"OPENTELEMETRY_PYTHON_TRACER_PROVIDER": "tracer_provider",

Choose a reason for hiding this comment

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

It'd be nice to have other env variables here to show that it's generic.

@toumorokoshi toumorokoshi merged commit c8b336d into open-telemetry:master Apr 13, 2020
srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020
* feat: implement createMeasure

* fix: linting

* fix: disable broken tests

* fix: labelSet log statement

* fix: absolute always set to true

* fix: linting

* fix: add jsdoc, adjust optionals

* fix: failing tests

* fix: linting

* refactor: rename test handle var

Co-authored-by: Mayur Kale <mayurkale@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Affects the API package. doc Documentation-related enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make the configuration object serve configuration outside the API
5 participants