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 convenience methods to access common config options in Configuration class #1426

Merged
merged 18 commits into from
Dec 4, 2020
Merged
Show file tree
Hide file tree
Changes from 9 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
2 changes: 1 addition & 1 deletion .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ env:
# Otherwise, set variable to the commit of your branch on
# opentelemetry-python-contrib which is compatible with these Core repo
# changes.
CONTRIB_REPO_SHA: bcec49cf2eccf8da66c9e63b9836ea8a20516efc
CONTRIB_REPO_SHA: 61ae201677be80e333746f06bc9485fa8856f42c
Copy link
Member Author

Choose a reason for hiding this comment

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

Updated to show that tests pass, will revert once approved.

Copy link
Contributor

Choose a reason for hiding this comment

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

As soon as your Contrib repo PR gets merged, you can't revert this. It either has to be master or this commit. I think leaving it as this commit is fine so that you don't have to change anything!

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought the flow was supposed to be the reverse i.e, core repo gets merged first and then contrib.


jobs:
build:
Expand Down
35 changes: 32 additions & 3 deletions opentelemetry-api/src/opentelemetry/configuration/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -93,14 +93,26 @@
to override this value instead of changing it.
"""

import re
from os import environ
from re import fullmatch
from typing import ClassVar, Dict, Optional, TypeVar, Union
from typing import ClassVar, Dict, List, Optional, Sequence, TypeVar, Union

ConfigValue = Union[str, bool, int, float]
_T = TypeVar("_T", ConfigValue, Optional[ConfigValue])


class ExcludeList:
"""Class to exclude certain paths (given as a list of regexes) from tracing requests"""

def __init__(self, excluded_urls: Sequence[str]):
self._non_empty = len(excluded_urls) > 0
if self._non_empty:
self._regex = re.compile("|".join(excluded_urls))

def url_disabled(self, url: str) -> bool:
return bool(self._non_empty and re.search(self._regex, url))


class Configuration:
_instance = None # type: ClassVar[Optional[Configuration]]
_config_map = {} # type: ClassVar[Dict[str, ConfigValue]]
Expand All @@ -113,7 +125,7 @@ def __new__(cls) -> "Configuration":
instance = super().__new__(cls)
for key, value_str in environ.items():

match = fullmatch(r"OTEL_(PYTHON_)?([A-Za-z_][\w_]*)", key)
match = re.fullmatch(r"OTEL_(PYTHON_)?([A-Za-z_][\w_]*)", key)

if match is not None:

Expand Down Expand Up @@ -167,3 +179,20 @@ def _reset(cls) -> None:
if cls._instance:
cls._instance._config_map.clear() # pylint: disable=protected-access
cls._instance = None

def traced_request_attrs(self, instrumentation: str) -> List[str]:
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Can you make these protected? I believe we are only using these internally.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah sure. I was thinking we would let users use this if they need.

"""Returns list of traced request attributes for instrumentation."""
key = "{}_TRACED_REQUEST_ATTRS".format(instrumentation.upper())
value = self._config_map.get(key, "")

request_attrs = (
[attr.strip() for attr in str.split(value, ",")] if value else [] # type: ignore
srikanthccv marked this conversation as resolved.
Show resolved Hide resolved
)
return request_attrs

def excluded_urls(self, instrumentation: str) -> ExcludeList:
key = "{}_EXCLUDED_URLS".format(instrumentation.upper())
value = self._config_map.get(key, "")
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if commas are allowed in URLs, but they can appear meaningfully in a regex like \d{5,10}. This seems more like an issue with passing comma separated regexes in envars than this PR though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Comma is reserved character and is not typically used in URLs. It is normally used in query component for name value pairs.


urls = str.split(value, ",") if value else [] # type: ignore
return ExcludeList(urls)
12 changes: 0 additions & 12 deletions opentelemetry-api/src/opentelemetry/util/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -65,15 +65,3 @@ def _load_meter_provider(provider: str) -> "MeterProvider":

def _load_trace_provider(provider: str) -> "TracerProvider":
return cast("TracerProvider", _load_provider(provider))


class ExcludeList:
"""Class to exclude certain paths (given as a list of regexes) from tracing requests"""

def __init__(self, excluded_urls: Sequence[str]):
self._non_empty = len(excluded_urls) > 0
if self._non_empty:
self._regex = re.compile("|".join(excluded_urls))

def url_disabled(self, url: str) -> bool:
return bool(self._non_empty and re.search(self._regex, url))
29 changes: 29 additions & 0 deletions opentelemetry-api/tests/configuration/test_configuration.py
Original file line number Diff line number Diff line change
Expand Up @@ -145,3 +145,32 @@ def test_float(self) -> None:
self.assertEqual(
Configuration().NON_FLOAT, "-12z3.123"
) # pylint: disable=no-member

@patch.dict(
"os.environ", # type: ignore
{
"OTEL_PYTHON_WEBFRAMEWORK_TRACED_REQUEST_ATTRS": "content_type,keep_alive",
},
)
def test_traced_request_attrs(self) -> None:
cfg = Configuration()
request_attrs = cfg.traced_request_attrs("webframework")
self.assertEqual(len(request_attrs), 2)
self.assertIn("content_type", request_attrs)
self.assertIn("keep_alive", request_attrs)
self.assertNotIn("authorization", request_attrs)

@patch.dict(
"os.environ", # type: ignore
{
"OTEL_PYTHON_WEBFRAMEWORK_EXCLUDED_URLS": "/healthzz,path,/issues/.*/view",
},
)
def test_excluded_urls(self) -> None:
cfg = Configuration()
excluded_urls = cfg.excluded_urls("webframework")
self.assertTrue(excluded_urls.url_disabled("/healthzz"))
self.assertTrue(excluded_urls.url_disabled("/path"))
self.assertTrue(excluded_urls.url_disabled("/issues/123/view"))
self.assertFalse(excluded_urls.url_disabled("/issues"))
self.assertFalse(excluded_urls.url_disabled("/hello"))
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,8 @@
# limitations under the License.

import unittest
from unittest import mock

from opentelemetry.util import ExcludeList
from opentelemetry.configuration import ExcludeList


class TestExcludeList(unittest.TestCase):
Expand Down