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

Adding the Resource API #61

Merged
merged 6 commits into from
Jul 29, 2019
Merged
Show file tree
Hide file tree
Changes from 2 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
53 changes: 53 additions & 0 deletions opentelemetry-api/src/opentelemetry/resources/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,56 @@
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
from typing import Any, Dict, Union


class Resource:
def __init__(self, labels: Dict[str, str]):
toumorokoshi marked this conversation as resolved.
Show resolved Hide resolved
"""
toumorokoshi marked this conversation as resolved.
Show resolved Hide resolved
Construct a resource. Direct calling of the
toumorokoshi marked this conversation as resolved.
Show resolved Hide resolved
constructor is discouraged, as it cannot
take advantage of caching and restricts
to the type of object that can be returned.
"""
self._labels = labels

@staticmethod
def create(labels: Dict[str, str]) -> "Resource":
Copy link
Member

Choose a reason for hiding this comment

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

I don't know that we want to force implementers to have a static constructor; the arguments for java might not apply here.

It would be great to have a static empty resource on this class 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.

There was a discussion in opentelemetry-specifications about this, and right now I'm a little convinced toward using a factory.

There's a lot of functionality we can augment without adding significant code complexity (e.g. caching and a more flexible type signature. I think it is theoretically possible to accomplish something similar with constructors, but that requires some fairly complex concepts like metaclasses to pull off.

DefaultResource doesn't take strong advantage of this, but I wonder if other resource implementations may.

Copy link
Member

Choose a reason for hiding this comment

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

That sounds fine to me, and maybe we should have factories for more of these classes.

Any reason to keep the constructor impl here?

Copy link
Member Author

Choose a reason for hiding this comment

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

no, sorry, that was a mistake. as was keeping some methods non-abstract. I'll address that.

"""
create a new resource. This is the recommended
Copy link
Member

Choose a reason for hiding this comment

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

minor: capitalize "Create"?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes! will do.

method to use to create a new resource.
"""
return Resource(labels)

def merge(self, other: Union["Resource", None]) -> "Resource":
"""
Copy link
Member

Choose a reason for hiding this comment

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

No blank lines here? I'm surprised lint doesn't complain about this one either.

Copy link
Member Author

Choose a reason for hiding this comment

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

will add blank lines

Perform a merge of the resources, resulting
in a union of the resource objects.

labels that exist in the main Resource take
precedence unless the label value is empty.
"""
if other is None:
return self
if not self._labels:
return other
merged_labels = self.get_labels().copy()
toumorokoshi marked this conversation as resolved.
Show resolved Hide resolved
for key, value in other.get_labels().items():
if key not in merged_labels or merged_labels[key] == "":
merged_labels[key] = value
return Resource(merged_labels)

def get_labels(self) -> Dict[str, str]:
"""
Return the labels associated with this resource.

get_labels exposes the raw internal dictionary,
and as such it is not recommended to copy the
result if it is desired to mutate the result.
"""
return self._labels
Copy link
Member

Choose a reason for hiding this comment

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

Why give _labels the underscore treatment if this method just returns it directly?

Copy link
Member Author

Choose a reason for hiding this comment

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

The desire was to match the API specification, which calls for GetLabels.

https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-resources.md#getlabels

That said now that I understand that the idea is to provide capabilities rather than specific method names / etc, I can just remove this and make labels public.

Copy link
Member Author

Choose a reason for hiding this comment

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

I switched this over to a property. I was hoping that I could assign values directly and have that satisfy and ABC's property interface, but unfortunately that didn't work as expected.

I've tried to reach a middle ground in the code, which still has a private _labels dict. But it does expose labels in a more Pythonic fashion, while ensuring that any class extending the ABC will still have a labels property.


def __eq__(self, other: Any) -> bool:
toumorokoshi marked this conversation as resolved.
Show resolved Hide resolved
if not isinstance(other, Resource):
return False
return self.get_labels() == other.get_labels()
Empty file.
24 changes: 24 additions & 0 deletions opentelemetry-api/tests/resources/test_init.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import unittest
from opentelemetry.resources import Resource


class TestResources(unittest.TestCase):
@staticmethod
toumorokoshi marked this conversation as resolved.
Show resolved Hide resolved
def test_resource_merge():
left = Resource({"service": "ui"})
right = Resource({"host": "service-host"})
assert left.merge(right) == Resource(
{"service": "ui", "host": "service-host"}
)

@staticmethod
def test_resource_merge_empty_string():
"""
labels from the source Resource take
precedence, with the exception of the empty string.
"""
left = Resource({"service": "ui", "host": ""})
right = Resource({"host": "service-host", "service": "not-ui"})
assert left.merge(right) == Resource(
{"service": "ui", "host": "service-host"}
)