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

Conversation

srikanthccv
Copy link
Member

@srikanthccv srikanthccv commented Nov 26, 2020

Description

Fixes #1217

Type of change

Does This PR Require a Contrib Repo Change?

@@ -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.

@@ -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.


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.

@lzchen lzchen merged commit 268cfbf into open-telemetry:master Dec 4, 2020
@srikanthccv srikanthccv deleted the config-common branch September 24, 2021 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Configuration class should have convenience methods to access common config options
5 participants