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

Align SDK with resource detection spec #1063

Closed
aabmass opened this issue Sep 1, 2020 · 5 comments · Fixed by #1096
Closed

Align SDK with resource detection spec #1063

aabmass opened this issue Sep 1, 2020 · 5 comments · Fixed by #1096
Assignees
Labels
release:required-for-ga To be resolved before GA release sdk Affects the SDK package.

Comments

@aabmass
Copy link
Member

aabmass commented Sep 1, 2020

Resource detection has been merged into the spec. The SDK should be aligned with the spec. This may already be the case.

Is your feature request related to a problem?
No

Describe the solution you'd like
The resource detection spec is flexible and doesn't prescribe anything. Currently, we have this code:

class ResourceDetector(abc.ABC):
def __init__(self, raise_on_error=False):
self.raise_on_error = raise_on_error
@abc.abstractmethod
def detect(self) -> "Resource":
raise NotImplementedError()
class OTELResourceDetector(ResourceDetector):
# pylint: disable=no-self-use
def detect(self) -> "Resource":
env_resources_items = os.environ.get("OTEL_RESOURCE_ATTRIBUTES")
env_resource_map = {}
if env_resources_items:
env_resource_map = {
key.strip(): value.strip()
for key, value in (
item.split("=") for item in env_resources_items.split(",")
)
}
return Resource(env_resource_map)
def get_aggregated_resources(
detectors: typing.List["ResourceDetector"],
initial_resource: typing.Optional[Resource] = None,
timeout=5,
) -> "Resource":
""" Retrieves resources from detectors in the order that they were passed
:param detectors: List of resources in order of priority
:param initial_resource: Static resource. This has highest priority
:param timeout: Number of seconds to wait for each detector to return
:return:
"""
final_resource = initial_resource or _EMPTY_RESOURCE
detectors = [OTELResourceDetector()] + detectors
with concurrent.futures.ThreadPoolExecutor(max_workers=4) as executor:
futures = [executor.submit(detector.detect) for detector in detectors]
for detector_ind, future in enumerate(futures):
detector = detectors[detector_ind]
try:
detected_resources = future.result(timeout=timeout)
# pylint: disable=broad-except
except Exception as ex:
if detector.raise_on_error:
raise ex
logger.warning(
"Exception %s in detector %s, ignoring", ex, detector
)
detected_resources = _EMPTY_RESOURCE
finally:
final_resource = final_resource.merge(detected_resources)
return final_resource

We could:

  • Leave everything as it is. The spec doesn't say we need a ResourceDetector interface or get_aggregated_resources() nor does it say it shouldn't be there.
  • Remove the ResourceDetector interface and get_aggregated_resources()
  • Replace the ResourceDetector interface with a Callable[[bool], Resource] function param, since the spec only says

    Resource detector packages MUST provide a method that returns a resource.

Thoughts?

@aabmass aabmass self-assigned this Sep 1, 2020
@lzchen
Copy link
Contributor

lzchen commented Sep 1, 2020

I like having the ResourceDetector interface, it gives users an idea of how to implement their own detector.
Is get_aggregated_resources() something we expect users to call explictly? As a user, I'd expect to just instantiate my own ResourceDetector (s) and pass them into the TracerProvider. So instead of doing something like this, maybe we can change TracerProvider to accept a sequence of ResoureDetectors, and then we merge them upon instantiation of the provider underneath (so we don't have to expose get_aggregated_resources() to the user.

@lzchen
Copy link
Contributor

lzchen commented Sep 1, 2020

The Callable[[bool], Resource] is a cool idea. I'd think they would have to depend on the SDK since they would be returning a Resource?

@aabmass
Copy link
Member Author

aabmass commented Sep 1, 2020

I'd think they would have to depend on the SDK since they would be returning a Resource?

Oh ya, my bad. Updated the issue description.

maybe we can change TracerProvider to accept a sequence of ResoureDetectors, and then we merge them upon instantiation of the provider underneath

I like this interface better, not sure if that would be taking too much liberty interpreting the spec though. I think having a util to run the detectors in parallel is worth having, once way or another.

@lzchen lzchen added release:required-for-ga To be resolved before GA release sdk Affects the SDK package. labels Sep 2, 2020
@codeboten
Copy link
Contributor

codeboten commented Sep 3, 2020

As discussed in the SIG meeting get_aggregated_resources can stay but the recommendation is to pass in merged resources, for which users can use get_aggregated_resources

@aabmass
Copy link
Member Author

aabmass commented Sep 3, 2020

Cool, so the only thing remaining is calling and using the OTELResourceDetector in the providers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:required-for-ga To be resolved before GA release sdk Affects the SDK package.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants