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

Configuration class should have convenience methods to access common config options #1217

Closed
owais opened this issue Oct 8, 2020 · 4 comments · Fixed by #1426
Closed

Configuration class should have convenience methods to access common config options #1217

owais opened this issue Oct 8, 2020 · 4 comments · Fixed by #1426
Assignees
Labels

Comments

@owais
Copy link
Contributor

owais commented Oct 8, 2020

A lot of instrumentations use very similar configuration options such as OTEL_PYTHON_<instrumentation>_TRACED_REQUEST_ATTRS or OTEL_PYTHON_<instrumentation>_EXCLUDED_URLS. Each instrumentation duplicates code to read and parse these values using the Configuration class. It would be nice if the Configuration class had convenience methods for such common config options. It could look something like:

cfg = Configuration()
traced_method = cfg.traced_methods("django")
excluded_urls = cfg.excluded_urls("django")
@srikanthccv
Copy link
Member

@owais There is an issue on contrib repo about adding exclude urls capability to fastapi. It also requires the reading and splitting or returning empty list if evn var isn't set. I will pick this up.

@owais
Copy link
Contributor Author

owais commented Nov 25, 2020

Thanks @lonewolf3739

@srikanthccv
Copy link
Member

@owais I was thinking it would be better to return an instance of ExcludeList when cfg.excluded_urls(...) is called.

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))

There is a problem here. This util has a dependency on Configuration to load trace provider and meter provider. If I try to import ExcludeList list from here it will introduce a circular dependency. Would there be any concern about moving ExcludeList to Configuraion file? We can make instrumentations depend on cfg.excluded_urls.

@owais
Copy link
Contributor Author

owais commented Nov 26, 2020

I don't see any glaring issues with that. May be configuration is the better module to host ExcludeList in the first place if we add the concept of excluded urls to the configuration class. Only concern would be if there are modules other than instrumentations that use ExcludeList today but don't use configuration. It might still be OK to add configuration as a dependency to such modules.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants